Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name

Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name

[Philippe Rétornaz]

Hi

This patch breaks DT-Enabled kernel run on non-DT board:
[    0.822977] Unable to handle kernel NULL pointer dereference at 
virtual address 0000001c
(...)
[    0.880320] [<c02f3e94>] (of_get_next_child) from [<c02f5420>] 
(of_get_child_by_name+0x38/0x50)
[    0.881449] [<c02f5420>] (of_get_child_by_name) from [<c01f6258>] 
(mc13xxx_get_num_regulators_dt+0x18/0x64)
[    0.882707] [<c01f6258>] (mc13xxx_get_num_regulators_dt) from 
[<c01f5d38>] (mc13783_regulator_probe+0x34/0x17c)
[    0.884011] [<c01f5d38>] (mc13783_regulator_probe) from [<c021fed0>] 
(platform_drv_probe+0x20/0x54)
[    0.885182] [<c021fed0>] (platform_drv_probe) from [<c021e674>] 
(driver_probe_device+0x144/0x360)
(...)

Because mc13783-regulator do in its probe:
static int mc13783_regulator_probe(struct platform_device *pdev)
{
(...)
         int i, num_regulators;

         num_regulators = mc13xxx_get_num_regulators_dt(pdev);
         if (num_regulators <= 0 && pdata)
                 num_regulators = pdata->num_regulators


and mc13xxx_get_num_regulators_dt() do, before your patch:

parent = of_find_node_by_name(pdev->dev.parent->of_node, "regulators");
if (!parent)
  		return -ENODEV;

of_find_node_by_name will return NULL if the node passed is NULL and the
DT is non-existant.

But, with your change we use this:
parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");

Which will OOPS as it does not expect to have a NULL node passed as 
argument.

So please revert this patch.

Regards,

Philippe

Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name

[Sachin Kamat]

On 24 February 2014 19:14, Philippe Rétornaz <philippe.retornaz@epfl.ch> wrote:
> Hi
>
> This patch breaks DT-Enabled kernel run on non-DT board:
> [    0.822977] Unable to handle kernel NULL pointer dereference at virtual
> address 0000001c
> (...)
> [    0.880320] [<c02f3e94>] (of_get_next_child) from [<c02f5420>]
> (of_get_child_by_name+0x38/0x50)
> [    0.881449] [<c02f5420>] (of_get_child_by_name) from [<c01f6258>]
> (mc13xxx_get_num_regulators_dt+0x18/0x64)
> [    0.882707] [<c01f6258>] (mc13xxx_get_num_regulators_dt) from
> [<c01f5d38>] (mc13783_regulator_probe+0x34/0x17c)
> [    0.884011] [<c01f5d38>] (mc13783_regulator_probe) from [<c021fed0>]
> (platform_drv_probe+0x20/0x54)
> [    0.885182] [<c021fed0>] (platform_drv_probe) from [<c021e674>]
> (driver_probe_device+0x144/0x360)
> (...)
>
> Because mc13783-regulator do in its probe:
> static int mc13783_regulator_probe(struct platform_device *pdev)
> {
> (...)
>         int i, num_regulators;
>
>         num_regulators = mc13xxx_get_num_regulators_dt(pdev);
>         if (num_regulators <= 0 && pdata)
>                 num_regulators = pdata->num_regulators
>
>
> and mc13xxx_get_num_regulators_dt() do, before your patch:
>
> parent = of_find_node_by_name(pdev->dev.parent->of_node, "regulators");
> if (!parent)
>                 return -ENODEV;
>
> of_find_node_by_name will return NULL if the node passed is NULL and the
> DT is non-existant.
>
> But, with your change we use this:
> parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
>
> Which will OOPS as it does not expect to have a NULL node passed as
> argument.
>
> So please revert this patch.

Sorry for the issue caused due to this patch. I would prefer to try to
have this fixed before reverting.
Can you please try with below change and let me know if it works:

 parent = of_get_child_by_name(pdev->dev->of_node, "regulators");

Also, how about adding NULL check to see it the node exists at all?

-- 
With warm regards,
Sachin

Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name

[Philippe Rétornaz]

Le 24/02/2014 15:51, Sachin Kamat a écrit :
> On 24 February 2014 19:14, Philippe Rétornaz
> <philippe.retornaz@epfl.ch> wrote:
>> Hi
>>
>> This patch breaks DT-Enabled kernel run on non-DT board: [
>> 0.822977] Unable to handle kernel NULL pointer dereference at
>> virtual address 0000001c (...) [    0.880320] [<c02f3e94>]
>> (of_get_next_child) from [<c02f5420>]
>> (of_get_child_by_name+0x38/0x50) [    0.881449] [<c02f5420>]
>> (of_get_child_by_name) from [<c01f6258>]
>> (mc13xxx_get_num_regulators_dt+0x18/0x64) [    0.882707]
>> [<c01f6258>] (mc13xxx_get_num_regulators_dt) from [<c01f5d38>]
>> (mc13783_regulator_probe+0x34/0x17c) [    0.884011] [<c01f5d38>]
>> (mc13783_regulator_probe) from [<c021fed0>]
>> (platform_drv_probe+0x20/0x54) [    0.885182] [<c021fed0>]
>> (platform_drv_probe) from [<c021e674>]
>> (driver_probe_device+0x144/0x360) (...)
>>
>> Because mc13783-regulator do in its probe: static int
>> mc13783_regulator_probe(struct platform_device *pdev) { (...) int
>> i, num_regulators;
>>
>> num_regulators = mc13xxx_get_num_regulators_dt(pdev); if
>> (num_regulators <= 0 && pdata) num_regulators =
>> pdata->num_regulators
>>
>>
>> and mc13xxx_get_num_regulators_dt() do, before your patch:
>>
>> parent = of_find_node_by_name(pdev->dev.parent->of_node,
>> "regulators"); if (!parent) return -ENODEV;
>>
>> of_find_node_by_name will return NULL if the node passed is NULL
>> and the DT is non-existant.
>>
>> But, with your change we use this: parent =
>> of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
>>
>> Which will OOPS as it does not expect to have a NULL node passed as
>> argument.
>>
>> So please revert this patch.
>
> Sorry for the issue caused due to this patch. I would prefer to try
> to have this fixed before reverting. Can you please try with below
> change and let me know if it works:
>
> parent = of_get_child_by_name(pdev->dev->of_node, "regulators");
>

You meant pdev->dev.of_node here I guess ?
This still OOPS as the board is not booting with DT, so all of_node are
NULL.

> Also, how about adding NULL check to see it the node exists at all?
>

Could be an option. BTW, mc13xxx_parse_regulators_dt() has the same problem.

Regards,

Philippe

Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name

[Sachin Kamat]

On 24 February 2014 20:37, Philippe Rétornaz <philippe.retornaz@epfl.ch> wrote:
> Le 24/02/2014 15:51, Sachin Kamat a écrit :
>
>> On 24 February 2014 19:14, Philippe Rétornaz
>> <philippe.retornaz@epfl.ch> wrote:
>>>
>>> Hi
>>>
>>> This patch breaks DT-Enabled kernel run on non-DT board: [
>>> 0.822977] Unable to handle kernel NULL pointer dereference at
>>> virtual address 0000001c (...) [    0.880320] [<c02f3e94>]
>>> (of_get_next_child) from [<c02f5420>]
>>> (of_get_child_by_name+0x38/0x50) [    0.881449] [<c02f5420>]
>>> (of_get_child_by_name) from [<c01f6258>]
>>> (mc13xxx_get_num_regulators_dt+0x18/0x64) [    0.882707]
>>> [<c01f6258>] (mc13xxx_get_num_regulators_dt) from [<c01f5d38>]
>>> (mc13783_regulator_probe+0x34/0x17c) [    0.884011] [<c01f5d38>]
>>> (mc13783_regulator_probe) from [<c021fed0>]
>>> (platform_drv_probe+0x20/0x54) [    0.885182] [<c021fed0>]
>>> (platform_drv_probe) from [<c021e674>]
>>> (driver_probe_device+0x144/0x360) (...)
>>>
>>> Because mc13783-regulator do in its probe: static int
>>> mc13783_regulator_probe(struct platform_device *pdev) { (...) int
>>> i, num_regulators;
>>>
>>> num_regulators = mc13xxx_get_num_regulators_dt(pdev); if
>>> (num_regulators <= 0 && pdata) num_regulators =
>>> pdata->num_regulators
>>>
>>>
>>> and mc13xxx_get_num_regulators_dt() do, before your patch:
>>>
>>> parent = of_find_node_by_name(pdev->dev.parent->of_node,
>>> "regulators"); if (!parent) return -ENODEV;
>>>
>>> of_find_node_by_name will return NULL if the node passed is NULL
>>> and the DT is non-existant.
>>>
>>> But, with your change we use this: parent =
>>> of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
>>>
>>> Which will OOPS as it does not expect to have a NULL node passed as
>>> argument.
>>>
>>> So please revert this patch.
>>
>>
>> Sorry for the issue caused due to this patch. I would prefer to try
>> to have this fixed before reverting. Can you please try with below
>> change and let me know if it works:
>>
>> parent = of_get_child_by_name(pdev->dev->of_node, "regulators");
>>
>
> You meant pdev->dev.of_node here I guess ?

Yes right. Sorry for the typo.

> This still OOPS as the board is not booting with DT, so all of_node are
> NULL.

So this is non-DT mode. Please see further comments below.

>
>> Also, how about adding NULL check to see it the node exists at all?
>>
>
> Could be an option. BTW, mc13xxx_parse_regulators_dt() has the same problem.

I would suggest adding a check at the initial stage itself to see if
it is non-DT and then either
error out or skip appropriately.

-- 
With warm regards,
Sachin

[PATCH 1/1] regulator: mc13xxx: check if DT is enabled

[Philippe Rétornaz]

This fix a regression on non-DT board booted with a DT enabled kernel

Signed-off-by: Philippe Rétornaz <philippe.retornaz@epfl.ch>
---
 drivers/regulator/mc13xxx-regulator-core.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
index 4498a3f..a10c999 100644
--- a/drivers/regulator/mc13xxx-regulator-core.c
+++ b/drivers/regulator/mc13xxx-regulator-core.c
@@ -167,6 +167,9 @@ int mc13xxx_get_num_regulators_dt(struct platform_device *pdev)
 	struct device_node *parent;
 	int num;
 
+	if(!pdev->dev.parent->of_node)
+		return -ENODEV;
+
 	of_node_get(pdev->dev.parent->of_node);
 	parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
 	if (!parent)
@@ -187,6 +190,9 @@ struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
 	struct device_node *parent, *child;
 	int i, parsed = 0;
 
+	if(!pdev->dev.parent->of_node)
+		return NULL;
+
 	of_node_get(pdev->dev.parent->of_node);
 	parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
 	if (!parent)
-- 
1.7.9.5

Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled

[Sachin Kamat]

On 25 February 2014 14:17, Philippe Rétornaz <philippe.retornaz@epfl.ch> wrote:
> This fix a regression on non-DT board booted with a DT enabled kernel
>
> Signed-off-by: Philippe Rétornaz <philippe.retornaz@epfl.ch>
> ---
>  drivers/regulator/mc13xxx-regulator-core.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
> index 4498a3f..a10c999 100644
> --- a/drivers/regulator/mc13xxx-regulator-core.c
> +++ b/drivers/regulator/mc13xxx-regulator-core.c
> @@ -167,6 +167,9 @@ int mc13xxx_get_num_regulators_dt(struct platform_device *pdev)
>         struct device_node *parent;
>         int num;
>
> +       if(!pdev->dev.parent->of_node)
> +               return -ENODEV;
> +
>         of_node_get(pdev->dev.parent->of_node);
>         parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
>         if (!parent)
> @@ -187,6 +190,9 @@ struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
>         struct device_node *parent, *child;
>         int i, parsed = 0;
>
> +       if(!pdev->dev.parent->of_node)
> +               return NULL;
> +
>         of_node_get(pdev->dev.parent->of_node);
>         parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
>         if (!parent)
> --
> 1.7.9.5
>

Thanks for the patch. I was about to send a similar patch which also
does a little more cleanup.
If you could test it in DT and non-DT modes that would be great. I
attach the diff below. You can
even fold it into your patch if you wish.

------------------
diff --git a/drivers/regulator/mc13xxx-regulator-core.c
b/drivers/regulator/mc13xxx-regulator-core.c
index 4498a3f0733d..bf75fcabfa3c 100644
--- a/drivers/regulator/mc13xxx-regulator-core.c
+++ b/drivers/regulator/mc13xxx-regulator-core.c
@@ -167,8 +167,10 @@ int mc13xxx_get_num_regulators_dt(struct
platform_device *pdev)
        struct device_node *parent;
        int num;

-       of_node_get(pdev->dev.parent->of_node);
-       parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
+       if (!pdev->dev.of_node)
+               return -ENODEV;
+
+       parent = of_get_child_by_name(pdev->dev.of_node, "regulators");
        if (!parent)
                return -ENODEV;

@@ -187,8 +189,10 @@ struct mc13xxx_regulator_init_data
*mc13xxx_parse_regulators_dt(
        struct device_node *parent, *child;
        int i, parsed = 0;

-       of_node_get(pdev->dev.parent->of_node);
-       parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
+       if (!pdev->dev.of_node)
+               return NULL;
+
+       parent = of_get_child_by_name(pdev->dev.of_node, "regulators");
        if (!parent)
                return NULL;


-- 
With warm regards,
Sachin

Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled

[Philippe Rétornaz]

Le 25/02/2014 10:12, Sachin Kamat a écrit :
> On 25 February 2014 14:17, Philippe Rétornaz <philippe.retornaz@epfl.ch> wrote:
>> This fix a regression on non-DT board booted with a DT enabled kernel
>>
>> Signed-off-by: Philippe Rétornaz <philippe.retornaz@epfl.ch>
>> ---
>>   drivers/regulator/mc13xxx-regulator-core.c |    6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
>> index 4498a3f..a10c999 100644
>> --- a/drivers/regulator/mc13xxx-regulator-core.c
>> +++ b/drivers/regulator/mc13xxx-regulator-core.c
>> @@ -167,6 +167,9 @@ int mc13xxx_get_num_regulators_dt(struct platform_device *pdev)
>>          struct device_node *parent;
>>          int num;
>>
>> +       if(!pdev->dev.parent->of_node)
>> +               return -ENODEV;
>> +
>>          of_node_get(pdev->dev.parent->of_node);
>>          parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
>>          if (!parent)
>> @@ -187,6 +190,9 @@ struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
>>          struct device_node *parent, *child;
>>          int i, parsed = 0;
>>
>> +       if(!pdev->dev.parent->of_node)
>> +               return NULL;
>> +
>>          of_node_get(pdev->dev.parent->of_node);
>>          parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
>>          if (!parent)
>> --
>> 1.7.9.5
>>
>
> Thanks for the patch. I was about to send a similar patch which also
> does a little more cleanup.
> If you could test it in DT and non-DT modes that would be great. I
> attach the diff below. You can
> even fold it into your patch if you wish.

It does fix the regression on the non-DT board.
I don't have any DT-enabled board with mc13xxx PMIC so I cannot test the
DT case. I will let you test the DT case & submit the final patch.

Regards,

Philippe


>
> ------------------
> diff --git a/drivers/regulator/mc13xxx-regulator-core.c
> b/drivers/regulator/mc13xxx-regulator-core.c
> index 4498a3f0733d..bf75fcabfa3c 100644
> --- a/drivers/regulator/mc13xxx-regulator-core.c
> +++ b/drivers/regulator/mc13xxx-regulator-core.c
> @@ -167,8 +167,10 @@ int mc13xxx_get_num_regulators_dt(struct
> platform_device *pdev)
>          struct device_node *parent;
>          int num;
>
> -       of_node_get(pdev->dev.parent->of_node);
> -       parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
> +       if (!pdev->dev.of_node)
> +               return -ENODEV;
> +
> +       parent = of_get_child_by_name(pdev->dev.of_node, "regulators");
>          if (!parent)
>                  return -ENODEV;
>
> @@ -187,8 +189,10 @@ struct mc13xxx_regulator_init_data
> *mc13xxx_parse_regulators_dt(
>          struct device_node *parent, *child;
>          int i, parsed = 0;
>
> -       of_node_get(pdev->dev.parent->of_node);
> -       parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
> +       if (!pdev->dev.of_node)
> +               return NULL;
> +
> +       parent = of_get_child_by_name(pdev->dev.of_node, "regulators");
>          if (!parent)
>                  return NULL;
>
>

Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled

[Sachin Kamat]

On 25 February 2014 15:39, Philippe Rétornaz <philippe.retornaz@epfl.ch> wrote:
> Le 25/02/2014 10:12, Sachin Kamat a écrit :
>
>> On 25 February 2014 14:17, Philippe Rétornaz <philippe.retornaz@epfl.ch>
>> wrote:
>>
>> Thanks for the patch. I was about to send a similar patch which also
>> does a little more cleanup.
>> If you could test it in DT and non-DT modes that would be great. I
>> attach the diff below. You can
>> even fold it into your patch if you wish.
>
>
> It does fix the regression on the non-DT board.
> I don't have any DT-enabled board with mc13xxx PMIC so I cannot test the
> DT case. I will let you test the DT case & submit the final patch.

Thanks for the confirmation. I will post this patch.

-- 
With warm regards,
Sachin

Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 270 bytes --]

On Tue, Feb 25, 2014 at 09:47:51AM +0100, Philippe Rétornaz wrote:

> +	if(!pdev->dev.parent->of_node)
> +		return -ENODEV;

I know Sachin was going to post a more complete patch here but watch out
for coding style - there should be a space between if and the (.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 01/16] regulator: max77686: Use of_get_child_by_name

[Sachin Kamat]

of_find_node_by_name walks the allnodes list, and can thus walk
outside of the parent node. Use of_get_child_by_name instead.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
Series compile tested. Some max* regulators tested on h/w.
---
 drivers/regulator/max77686.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index ae001ccf26f4..0e725f6ed455 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -400,7 +400,7 @@ static int max77686_pmic_dt_parse_pdata(struct platform_device *pdev,
 	unsigned int i;
 
 	pmic_np = iodev->dev->of_node;
-	regulators_np = of_find_node_by_name(pmic_np, "voltage-regulators");
+	regulators_np = of_get_child_by_name(pmic_np, "voltage-regulators");
 	if (!regulators_np) {
 		dev_err(&pdev->dev, "could not find regulators sub-node\n");
 		return -EINVAL;
-- 
1.7.9.5

[PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name

[Sachin Kamat]

of_find_node_by_name walks the allnodes list, and can thus walk
outside of the parent node. Use of_get_child_by_name instead.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/regulator/mc13xxx-regulator-core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
index da4859282302..4498a3f0733d 100644
--- a/drivers/regulator/mc13xxx-regulator-core.c
+++ b/drivers/regulator/mc13xxx-regulator-core.c
@@ -168,7 +168,7 @@ int mc13xxx_get_num_regulators_dt(struct platform_device *pdev)
 	int num;
 
 	of_node_get(pdev->dev.parent->of_node);
-	parent = of_find_node_by_name(pdev->dev.parent->of_node, "regulators");
+	parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
 	if (!parent)
 		return -ENODEV;
 
@@ -188,7 +188,7 @@ struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
 	int i, parsed = 0;
 
 	of_node_get(pdev->dev.parent->of_node);
-	parent = of_find_node_by_name(pdev->dev.parent->of_node, "regulators");
+	parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
 	if (!parent)
 		return NULL;
 
-- 
1.7.9.5

Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 210 bytes --]

On Fri, Feb 14, 2014 at 05:19:59PM +0530, Sachin Kamat wrote:
> of_find_node_by_name walks the allnodes list, and can thus walk
> outside of the parent node. Use of_get_child_by_name instead.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]