* Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name
@ 2014-02-24 13:44 Philippe Rétornaz
2014-02-24 14:51 ` Sachin Kamat
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Rétornaz @ 2014-02-24 13:44 UTC (permalink / raw)
To: Sachin Kamat, Mark Brown; +Cc: linux-kernel, lgirdwood
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name 2014-02-24 13:44 [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name Philippe Rétornaz @ 2014-02-24 14:51 ` Sachin Kamat 2014-02-24 15:07 ` Philippe Rétornaz 0 siblings, 1 reply; 9+ messages in thread From: Sachin Kamat @ 2014-02-24 14:51 UTC (permalink / raw) To: Philippe Rétornaz; +Cc: Mark Brown, LKML, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name 2014-02-24 14:51 ` Sachin Kamat @ 2014-02-24 15:07 ` Philippe Rétornaz 2014-02-24 15:10 ` Sachin Kamat 0 siblings, 1 reply; 9+ messages in thread From: Philippe Rétornaz @ 2014-02-24 15:07 UTC (permalink / raw) To: Sachin Kamat; +Cc: Mark Brown, LKML, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name 2014-02-24 15:07 ` Philippe Rétornaz @ 2014-02-24 15:10 ` Sachin Kamat 2014-02-25 8:47 ` [PATCH 1/1] regulator: mc13xxx: check if DT is enabled Philippe Rétornaz 0 siblings, 1 reply; 9+ messages in thread From: Sachin Kamat @ 2014-02-24 15:10 UTC (permalink / raw) To: Philippe Rétornaz; +Cc: Mark Brown, LKML, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] regulator: mc13xxx: check if DT is enabled 2014-02-24 15:10 ` Sachin Kamat @ 2014-02-25 8:47 ` Philippe Rétornaz 2014-02-25 9:12 ` Sachin Kamat 2014-02-25 11:58 ` Mark Brown 0 siblings, 2 replies; 9+ messages in thread From: Philippe Rétornaz @ 2014-02-25 8:47 UTC (permalink / raw) To: sachin.kamat, broonie; +Cc: linux-kernel, lgirdwood, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled 2014-02-25 8:47 ` [PATCH 1/1] regulator: mc13xxx: check if DT is enabled Philippe Rétornaz @ 2014-02-25 9:12 ` Sachin Kamat 2014-02-25 10:09 ` Philippe Rétornaz 2014-02-25 11:58 ` Mark Brown 1 sibling, 1 reply; 9+ messages in thread From: Sachin Kamat @ 2014-02-25 9:12 UTC (permalink / raw) To: Philippe Rétornaz; +Cc: Mark Brown, LKML, Liam Girdwood 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled 2014-02-25 9:12 ` Sachin Kamat @ 2014-02-25 10:09 ` Philippe Rétornaz 2014-02-25 10:12 ` Sachin Kamat 0 siblings, 1 reply; 9+ messages in thread From: Philippe Rétornaz @ 2014-02-25 10:09 UTC (permalink / raw) To: Sachin Kamat; +Cc: Mark Brown, LKML, Liam Girdwood 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; > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled 2014-02-25 10:09 ` Philippe Rétornaz @ 2014-02-25 10:12 ` Sachin Kamat 0 siblings, 0 replies; 9+ messages in thread From: Sachin Kamat @ 2014-02-25 10:12 UTC (permalink / raw) To: Philippe Rétornaz; +Cc: Mark Brown, LKML, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] regulator: mc13xxx: check if DT is enabled 2014-02-25 8:47 ` [PATCH 1/1] regulator: mc13xxx: check if DT is enabled Philippe Rétornaz 2014-02-25 9:12 ` Sachin Kamat @ 2014-02-25 11:58 ` Mark Brown 1 sibling, 0 replies; 9+ messages in thread From: Mark Brown @ 2014-02-25 11:58 UTC (permalink / raw) To: Philippe Rétornaz; +Cc: sachin.kamat, linux-kernel, lgirdwood [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-02-25 11:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-24 13:44 [PATCH 13/16] regulator: mc13xxx: Use of_get_child_by_name Philippe Rétornaz 2014-02-24 14:51 ` Sachin Kamat 2014-02-24 15:07 ` Philippe Rétornaz 2014-02-24 15:10 ` Sachin Kamat 2014-02-25 8:47 ` [PATCH 1/1] regulator: mc13xxx: check if DT is enabled Philippe Rétornaz 2014-02-25 9:12 ` Sachin Kamat 2014-02-25 10:09 ` Philippe Rétornaz 2014-02-25 10:12 ` Sachin Kamat 2014-02-25 11:58 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.