[PATCH v2 1/2] ARM: OMAP2+: only search for GPMC DT child nodes on probe

[PATCH v2 1/2] ARM: OMAP2+: only search for GPMC DT child nodes on probe

[Javier Martinez Canillas]

The GPMC DT probe function use for_each_node_by_name() to search
child device nodes of the GPMC controller. But this function does
not use the GPMC device node as the root of the search and instead
search across the complete Device Tree.

This means that any device node on the DT that is using any of the
GPMC child nodes names searched for will be returned even if they
are not connected to the GPMC, making the gpmc_probe_xxx_child()
function to fail.

Fix this by using the GPMC device node as the search root so the
search will be restricted to its children.

Reported-by: Lars Poeschel <poeschel@lemonage.de>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes since v1 (suggested by Jon Hunter):
  - Split the search for GPMC child nodes and only warn if a
    child probe fails on two different patches
  - Don't probe all childs unnecesary if a previous matched

 arch/arm/mach-omap2/gpmc.c |   33 ++++++++++-----------------------
 1 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index ed946df..6166847 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1520,32 +1520,19 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 		return ret;
 	}
 
-	for_each_node_by_name(child, "nand") {
-		ret = gpmc_probe_nand_child(pdev, child);
-		if (ret < 0) {
-			of_node_put(child);
-			return ret;
-		}
-	}
+	for_each_child_of_node(pdev->dev.of_node, child) {
 
-	for_each_node_by_name(child, "onenand") {
-		ret = gpmc_probe_onenand_child(pdev, child);
-		if (ret < 0) {
-			of_node_put(child);
-			return ret;
-		}
-	}
+		if (!child->name)
+			continue;
 
-	for_each_node_by_name(child, "nor") {
-		ret = gpmc_probe_generic_child(pdev, child);
-		if (ret < 0) {
-			of_node_put(child);
-			return ret;
-		}
-	}
+		if (of_node_cmp(child->name, "nand") == 0)
+			ret = gpmc_probe_nand_child(pdev, child);
+		else if (of_node_cmp(child->name, "onenand") == 0)
+			ret = gpmc_probe_onenand_child(pdev, child);
+		else if (of_node_cmp(child->name, "ethernet") == 0 ||
+			 of_node_cmp(child->name, "nor") == 0)
+			ret = gpmc_probe_generic_child(pdev, child);
 
-	for_each_node_by_name(child, "ethernet") {
-		ret = gpmc_probe_generic_child(pdev, child);
 		if (ret < 0) {
 			of_node_put(child);
 			return ret;
-- 
1.7.7.6

[PATCH v2 2/2] ARM: OMAP2+: only WARN if a GPMC child probe function fail

[Javier Martinez Canillas]

If any of the GPMC child nodes fails, this shouldn't make the
whole gpmc_probe_dt() function to fail. It is better to just
WARN and allow other devices probe function to succeed.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes since v1 (suggested by Jon Hunter):
  - Split the search for GPMC child nodes and only warn if a
    child probe fails on two different patches
  - Add a warn message to report which GPMC child probe failed

 arch/arm/mach-omap2/gpmc.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 6166847..6c4da12 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1533,10 +1533,9 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 			 of_node_cmp(child->name, "nor") == 0)
 			ret = gpmc_probe_generic_child(pdev, child);
 
-		if (ret < 0) {
+		if (WARN(ret < 0, "%s: probing gpmc child %s failed\n",
+			 __func__, child->full_name))
 			of_node_put(child);
-			return ret;
-		}
 	}
 
 	return 0;
-- 
1.7.7.6

Re: [PATCH v2 1/2] ARM: OMAP2+: only search for GPMC DT child nodes on probe

[Jon Hunter]

On 04/17/2013 03:34 PM, Javier Martinez Canillas wrote:
> The GPMC DT probe function use for_each_node_by_name() to search
> child device nodes of the GPMC controller. But this function does
> not use the GPMC device node as the root of the search and instead
> search across the complete Device Tree.
> 
> This means that any device node on the DT that is using any of the
> GPMC child nodes names searched for will be returned even if they
> are not connected to the GPMC, making the gpmc_probe_xxx_child()
> function to fail.
> 
> Fix this by using the GPMC device node as the search root so the
> search will be restricted to its children.
> 
> Reported-by: Lars Poeschel <poeschel@lemonage.de>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> 
> Changes since v1 (suggested by Jon Hunter):
>   - Split the search for GPMC child nodes and only warn if a
>     child probe fails on two different patches
>   - Don't probe all childs unnecesary if a previous matched
> 
>  arch/arm/mach-omap2/gpmc.c |   33 ++++++++++-----------------------
>  1 files changed, 10 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index ed946df..6166847 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1520,32 +1520,19 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	for_each_node_by_name(child, "nand") {
> -		ret = gpmc_probe_nand_child(pdev, child);
> -		if (ret < 0) {
> -			of_node_put(child);
> -			return ret;
> -		}
> -	}
> +	for_each_child_of_node(pdev->dev.of_node, child) {
>  
> -	for_each_node_by_name(child, "onenand") {
> -		ret = gpmc_probe_onenand_child(pdev, child);
> -		if (ret < 0) {
> -			of_node_put(child);
> -			return ret;
> -		}
> -	}
> +		if (!child->name)
> +			continue;
>  
> -	for_each_node_by_name(child, "nor") {
> -		ret = gpmc_probe_generic_child(pdev, child);
> -		if (ret < 0) {
> -			of_node_put(child);
> -			return ret;
> -		}
> -	}
> +		if (of_node_cmp(child->name, "nand") == 0)
> +			ret = gpmc_probe_nand_child(pdev, child);
> +		else if (of_node_cmp(child->name, "onenand") == 0)
> +			ret = gpmc_probe_onenand_child(pdev, child);
> +		else if (of_node_cmp(child->name, "ethernet") == 0 ||
> +			 of_node_cmp(child->name, "nor") == 0)
> +			ret = gpmc_probe_generic_child(pdev, child);
>  
> -	for_each_node_by_name(child, "ethernet") {
> -		ret = gpmc_probe_generic_child(pdev, child);
>  		if (ret < 0) {

I think that we need to make sure that "ret" is initialised to 0 at the
beginning of the function. We should not have a case where the child
name does not match but who knows. Actually that raises another point,
should we have an "else" clause at the end that WARNs on
"unknown/unsupported child" device?

>  			of_node_put(child);
>  			return ret;
> 

Otherwise looks great.

Cheers
Jon

Re: [PATCH v2 1/2] ARM: OMAP2+: only search for GPMC DT child nodes on probe

[Javier Martinez Canillas]

On 04/17/2013 11:27 PM, Jon Hunter wrote:
> 
> On 04/17/2013 03:34 PM, Javier Martinez Canillas wrote:
>> The GPMC DT probe function use for_each_node_by_name() to search
>> child device nodes of the GPMC controller. But this function does
>> not use the GPMC device node as the root of the search and instead
>> search across the complete Device Tree.
>> 
>> This means that any device node on the DT that is using any of the
>> GPMC child nodes names searched for will be returned even if they
>> are not connected to the GPMC, making the gpmc_probe_xxx_child()
>> function to fail.
>> 
>> Fix this by using the GPMC device node as the search root so the
>> search will be restricted to its children.
>> 
>> Reported-by: Lars Poeschel <poeschel@lemonage.de>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>> 
>> Changes since v1 (suggested by Jon Hunter):
>>   - Split the search for GPMC child nodes and only warn if a
>>     child probe fails on two different patches
>>   - Don't probe all childs unnecesary if a previous matched
>> 
>>  arch/arm/mach-omap2/gpmc.c |   33 ++++++++++-----------------------
>>  1 files changed, 10 insertions(+), 23 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> index ed946df..6166847 100644
>> --- a/arch/arm/mach-omap2/gpmc.c
>> +++ b/arch/arm/mach-omap2/gpmc.c
>> @@ -1520,32 +1520,19 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> -	for_each_node_by_name(child, "nand") {
>> -		ret = gpmc_probe_nand_child(pdev, child);
>> -		if (ret < 0) {
>> -			of_node_put(child);
>> -			return ret;
>> -		}
>> -	}
>> +	for_each_child_of_node(pdev->dev.of_node, child) {
>>  
>> -	for_each_node_by_name(child, "onenand") {
>> -		ret = gpmc_probe_onenand_child(pdev, child);
>> -		if (ret < 0) {
>> -			of_node_put(child);
>> -			return ret;
>> -		}
>> -	}
>> +		if (!child->name)
>> +			continue;
>>  
>> -	for_each_node_by_name(child, "nor") {
>> -		ret = gpmc_probe_generic_child(pdev, child);
>> -		if (ret < 0) {
>> -			of_node_put(child);
>> -			return ret;
>> -		}
>> -	}
>> +		if (of_node_cmp(child->name, "nand") == 0)
>> +			ret = gpmc_probe_nand_child(pdev, child);
>> +		else if (of_node_cmp(child->name, "onenand") == 0)
>> +			ret = gpmc_probe_onenand_child(pdev, child);
>> +		else if (of_node_cmp(child->name, "ethernet") == 0 ||
>> +			 of_node_cmp(child->name, "nor") == 0)
>> +			ret = gpmc_probe_generic_child(pdev, child);
>>  
>> -	for_each_node_by_name(child, "ethernet") {
>> -		ret = gpmc_probe_generic_child(pdev, child);
>>  		if (ret < 0) {
> 
> I think that we need to make sure that "ret" is initialised to 0 at the
> beginning of the function. We should not have a case where the child

Hi Jon,

I didn't set ret  to 0 at the beginning of the function since it is assigned the
return value of a previous call to of_property_read_u32(). So ret should be 0
when execution reaches the for loop.

> name does not match but who knows. Actually that raises another point,
> should we have an "else" clause at the end that WARNs on
> "unknown/unsupported child" device?
>

Actually I thought about it when I was writing that patch and then I decided to
not add a WARN for that case since nothing really fail in that case.

I mean if we want to check that a DT is well formed then I think we will need to
add much more checks and I don't know if is worth it.

But I don't have a strong opinion on this so if you want I can add it an send a v3.

>>  			of_node_put(child);
>>  			return ret;
>> 
> 
> Otherwise looks great.
> 
> Cheers
> Jon
> 

Best regards,
Javier

Re: [PATCH v2 1/2] ARM: OMAP2+: only search for GPMC DT child nodes on probe

[Jon Hunter]

On 04/17/2013 05:10 PM, Javier Martinez Canillas wrote:
> On 04/17/2013 11:27 PM, Jon Hunter wrote:
>>
>> On 04/17/2013 03:34 PM, Javier Martinez Canillas wrote:
>>> The GPMC DT probe function use for_each_node_by_name() to search
>>> child device nodes of the GPMC controller. But this function does
>>> not use the GPMC device node as the root of the search and instead
>>> search across the complete Device Tree.
>>>
>>> This means that any device node on the DT that is using any of the
>>> GPMC child nodes names searched for will be returned even if they
>>> are not connected to the GPMC, making the gpmc_probe_xxx_child()
>>> function to fail.
>>>
>>> Fix this by using the GPMC device node as the search root so the
>>> search will be restricted to its children.
>>>
>>> Reported-by: Lars Poeschel <poeschel@lemonage.de>
>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>> ---
>>>
>>> Changes since v1 (suggested by Jon Hunter):
>>>   - Split the search for GPMC child nodes and only warn if a
>>>     child probe fails on two different patches
>>>   - Don't probe all childs unnecesary if a previous matched
>>>
>>>  arch/arm/mach-omap2/gpmc.c |   33 ++++++++++-----------------------
>>>  1 files changed, 10 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>>> index ed946df..6166847 100644
>>> --- a/arch/arm/mach-omap2/gpmc.c
>>> +++ b/arch/arm/mach-omap2/gpmc.c
>>> @@ -1520,32 +1520,19 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>>>  		return ret;
>>>  	}
>>>  
>>> -	for_each_node_by_name(child, "nand") {
>>> -		ret = gpmc_probe_nand_child(pdev, child);
>>> -		if (ret < 0) {
>>> -			of_node_put(child);
>>> -			return ret;
>>> -		}
>>> -	}
>>> +	for_each_child_of_node(pdev->dev.of_node, child) {
>>>  
>>> -	for_each_node_by_name(child, "onenand") {
>>> -		ret = gpmc_probe_onenand_child(pdev, child);
>>> -		if (ret < 0) {
>>> -			of_node_put(child);
>>> -			return ret;
>>> -		}
>>> -	}
>>> +		if (!child->name)
>>> +			continue;
>>>  
>>> -	for_each_node_by_name(child, "nor") {
>>> -		ret = gpmc_probe_generic_child(pdev, child);
>>> -		if (ret < 0) {
>>> -			of_node_put(child);
>>> -			return ret;
>>> -		}
>>> -	}
>>> +		if (of_node_cmp(child->name, "nand") == 0)
>>> +			ret = gpmc_probe_nand_child(pdev, child);
>>> +		else if (of_node_cmp(child->name, "onenand") == 0)
>>> +			ret = gpmc_probe_onenand_child(pdev, child);
>>> +		else if (of_node_cmp(child->name, "ethernet") == 0 ||
>>> +			 of_node_cmp(child->name, "nor") == 0)
>>> +			ret = gpmc_probe_generic_child(pdev, child);
>>>  
>>> -	for_each_node_by_name(child, "ethernet") {
>>> -		ret = gpmc_probe_generic_child(pdev, child);
>>>  		if (ret < 0) {
>>
>> I think that we need to make sure that "ret" is initialised to 0 at the
>> beginning of the function. We should not have a case where the child
> 
> Hi Jon,
> 
> I didn't set ret  to 0 at the beginning of the function since it is assigned the
> return value of a previous call to of_property_read_u32(). So ret should be 0
> when execution reaches the for loop.

Yes you are right, I overlooked that.

>> name does not match but who knows. Actually that raises another point,
>> should we have an "else" clause at the end that WARNs on
>> "unknown/unsupported child" device?
>>
> 
> Actually I thought about it when I was writing that patch and then I decided to
> not add a WARN for that case since nothing really fail in that case.
> 
> I mean if we want to check that a DT is well formed then I think we will need to
> add much more checks and I don't know if is worth it.
> 
> But I don't have a strong opinion on this so if you want I can add it an send a v3.

Ok, that's fine. I am happy with this version, so no need then to re-do.

Cheers
Jon

Re: [PATCH v2 1/2] ARM: OMAP2+: only search for GPMC DT child nodes on probe

[Javier Martinez Canillas]

On 04/18/2013 12:33 AM, Jon Hunter wrote:
> 
> On 04/17/2013 05:10 PM, Javier Martinez Canillas wrote:
>> On 04/17/2013 11:27 PM, Jon Hunter wrote:
>>>
>>> On 04/17/2013 03:34 PM, Javier Martinez Canillas wrote:
>>>> The GPMC DT probe function use for_each_node_by_name() to search
>>>> child device nodes of the GPMC controller. But this function does
>>>> not use the GPMC device node as the root of the search and instead
>>>> search across the complete Device Tree.
>>>>
>>>> This means that any device node on the DT that is using any of the
>>>> GPMC child nodes names searched for will be returned even if they
>>>> are not connected to the GPMC, making the gpmc_probe_xxx_child()
>>>> function to fail.
>>>>
>>>> Fix this by using the GPMC device node as the search root so the
>>>> search will be restricted to its children.
>>>>
>>>> Reported-by: Lars Poeschel <poeschel@lemonage.de>
>>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>>> ---
>>>>
>>>> Changes since v1 (suggested by Jon Hunter):
>>>>   - Split the search for GPMC child nodes and only warn if a
>>>>     child probe fails on two different patches
>>>>   - Don't probe all childs unnecesary if a previous matched
>>>>
>>>>  arch/arm/mach-omap2/gpmc.c |   33 ++++++++++-----------------------
>>>>  1 files changed, 10 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>>>> index ed946df..6166847 100644
>>>> --- a/arch/arm/mach-omap2/gpmc.c
>>>> +++ b/arch/arm/mach-omap2/gpmc.c
>>>> @@ -1520,32 +1520,19 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	for_each_node_by_name(child, "nand") {
>>>> -		ret = gpmc_probe_nand_child(pdev, child);
>>>> -		if (ret < 0) {
>>>> -			of_node_put(child);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> +	for_each_child_of_node(pdev->dev.of_node, child) {
>>>>  
>>>> -	for_each_node_by_name(child, "onenand") {
>>>> -		ret = gpmc_probe_onenand_child(pdev, child);
>>>> -		if (ret < 0) {
>>>> -			of_node_put(child);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> +		if (!child->name)
>>>> +			continue;
>>>>  
>>>> -	for_each_node_by_name(child, "nor") {
>>>> -		ret = gpmc_probe_generic_child(pdev, child);
>>>> -		if (ret < 0) {
>>>> -			of_node_put(child);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> +		if (of_node_cmp(child->name, "nand") == 0)
>>>> +			ret = gpmc_probe_nand_child(pdev, child);
>>>> +		else if (of_node_cmp(child->name, "onenand") == 0)
>>>> +			ret = gpmc_probe_onenand_child(pdev, child);
>>>> +		else if (of_node_cmp(child->name, "ethernet") == 0 ||
>>>> +			 of_node_cmp(child->name, "nor") == 0)
>>>> +			ret = gpmc_probe_generic_child(pdev, child);
>>>>  
>>>> -	for_each_node_by_name(child, "ethernet") {
>>>> -		ret = gpmc_probe_generic_child(pdev, child);
>>>>  		if (ret < 0) {
>>>
>>> I think that we need to make sure that "ret" is initialised to 0 at the
>>> beginning of the function. We should not have a case where the child
>> 
>> Hi Jon,
>> 
>> I didn't set ret  to 0 at the beginning of the function since it is assigned the
>> return value of a previous call to of_property_read_u32(). So ret should be 0
>> when execution reaches the for loop.
> 
> Yes you are right, I overlooked that.
> 
>>> name does not match but who knows. Actually that raises another point,
>>> should we have an "else" clause at the end that WARNs on
>>> "unknown/unsupported child" device?
>>>
>> 
>> Actually I thought about it when I was writing that patch and then I decided to
>> not add a WARN for that case since nothing really fail in that case.
>> 
>> I mean if we want to check that a DT is well formed then I think we will need to
>> add much more checks and I don't know if is worth it.
>> 
>> But I don't have a strong opinion on this so if you want I can add it an send a v3.
> 
> Ok, that's fine. I am happy with this version, so no need then to re-do.
>

Great, thanks a lot for your feedback!

Best regards,
Javier