On 07/25/2012 04:01 AM, Jia Hongtao-B38951 wrote:
>>> +/*
>>> + * Recursively scan all the children nodes of parent and find out if
>> there
>>> + * is "isa" node. Return 1 if parent has isa node otherwise return 0.
>>> + */
>>> +int has_isa_node(struct device_node *parent)
>>> +{
>>> + static int result;
>>> + struct device_node *cur_child;
>>> +
>>> + cur_child = NULL;
>>> + result = 0;
>>> + while (!result && (cur_child = of_get_next_child(parent,
>> cur_child))) {
>>> + /* Get "isa" node and return 1 */
>>> + if (of_node_cmp(cur_child->type, "isa") == 0)
>>> + return result = 1;
>>> + has_isa_node(cur_child);
>>> + }
>>> +
>>> + return result;
>>> +}
>>
>> Why are you reimplementing this? It's already in Linus's tree. See
>> fsl_pci_init().
>>
>> Plus, your version is recursive which is unacceptable in kernel code
>> with a small stack (outside of a few rare examples where the depth has a
>> small fixed upper bound), and once it finds an ISA node, it returns 1
>> forever, regardless of what node you pass in in the future.
>>
>> -Scott
>
> Yes, recursive function is not recommended for kernel but maybe it's not
> unacceptable.
> This function is not so deep stacked and simple. In my opinion this is
> acceptable.
The depth is limited not by code, but by externally provided data.
Granted a bad device tree can mess the kernel up in far worse ways, but
still it's a bad idea and totally unnecessary.
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
