Re: [PATCH 3/6] powerpc/fsl-pci: Determine primary bus by looking for ISA node

[Scott Wood <scottwood@freescale.com>]

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