* [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
@ 2020-09-22 21:05 Christophe JAILLET
2020-09-23 13:27 ` Dan Murphy
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Christophe JAILLET @ 2020-09-22 21:05 UTC (permalink / raw)
To: pavel, dmurphy, jacek.anaszewski
Cc: linux-leds, linux-kernel, kernel-janitors, Christophe JAILLET
In case of memory allocation failure, we must release some resources as
done in all other error handling paths of the function.
'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
is called when we break out of a 'device_for_each_child_node' loop.
Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/leds/leds-lp50xx.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index 47144a37cb94..8178782f2a8a 100644
--- a/drivers/leds/leds-lp50xx.c
+++ b/drivers/leds/leds-lp50xx.c
@@ -487,8 +487,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
*/
mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
sizeof(*mc_led_info), GFP_KERNEL);
- if (!mc_led_info)
- return -ENOMEM;
+ if (!mc_led_info) {
+ ret = -ENOMEM;
+ goto child_out;
+ }
fwnode_for_each_child_node(child, led_node) {
ret = fwnode_property_read_u32(led_node, "color",
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET
@ 2020-09-23 13:27 ` Dan Murphy
2020-09-23 13:35 ` Dan Carpenter
2020-11-25 11:01 ` Pavel Machek
2 siblings, 0 replies; 9+ messages in thread
From: Dan Murphy @ 2020-09-23 13:27 UTC (permalink / raw)
To: Christophe JAILLET, pavel, jacek.anaszewski
Cc: linux-leds, linux-kernel, kernel-janitors
Christophe
On 9/22/20 4:05 PM, Christophe JAILLET wrote:
> In case of memory allocation failure, we must release some resources as
> done in all other error handling paths of the function.
>
> 'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
> is called when we break out of a 'device_for_each_child_node' loop.
>
> Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/leds/leds-lp50xx.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index 47144a37cb94..8178782f2a8a 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -487,8 +487,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
> */
> mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
> sizeof(*mc_led_info), GFP_KERNEL);
> - if (!mc_led_info)
> - return -ENOMEM;
> + if (!mc_led_info) {
> + ret = -ENOMEM;
> + goto child_out;
> + }
>
> fwnode_for_each_child_node(child, led_node) {
> ret = fwnode_property_read_u32(led_node, "color",
Thanks for the patch
Acked-by: Dan Murphy <dmurphy@ti.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET
2020-09-23 13:27 ` Dan Murphy
@ 2020-09-23 13:35 ` Dan Carpenter
2020-09-23 18:49 ` Christophe JAILLET
2020-11-25 11:01 ` Pavel Machek
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2020-09-23 13:35 UTC (permalink / raw)
To: Christophe JAILLET, Heikki Krogerus
Cc: pavel, dmurphy, jacek.anaszewski, linux-leds, linux-kernel,
kernel-janitors
I've added Heikki Krogerus to the CC list because my question is mostly
about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
the firmware node framework").
I have been trying to teach Smatch to understand reference counting so
it can discover these kinds of bugs automatically.
I don't know how software_node_get_next_child() can work when it doesn't
call kobject_get(). This sort of bug would have been caught in testing
because it affects the success path so I must be reading the code wrong.
drivers/leds/leds-lp50xx.c
444 static int lp50xx_probe_dt(struct lp50xx *priv)
445 {
446 struct fwnode_handle *child = NULL;
447 struct fwnode_handle *led_node = NULL;
448 struct led_init_data init_data = {};
449 struct led_classdev *led_cdev;
450 struct mc_subled *mc_led_info;
451 struct lp50xx_led *led;
452 int ret = -EINVAL;
453 int num_colors;
454 u32 color_id;
455 int i = 0;
456
457 priv->enable_gpio = devm_gpiod_get_optional(priv->dev, "enable", GPIOD_OUT_LOW);
458 if (IS_ERR(priv->enable_gpio)) {
459 ret = PTR_ERR(priv->enable_gpio);
460 dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n",
461 ret);
462 return ret;
463 }
464
465 priv->regulator = devm_regulator_get(priv->dev, "vled");
466 if (IS_ERR(priv->regulator))
467 priv->regulator = NULL;
468
469 device_for_each_child_node(priv->dev, child) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^
This iterator is implemented by two possible function pointers. Smatch
understands of_fwnode_get_next_child_node() and that it takes a
reference. The software_node_get_next_child() function does not take
a kobject reference.
470 led = &priv->leds[i];
471 ret = fwnode_property_count_u32(child, "reg");
472 if (ret < 0) {
473 dev_err(&priv->client->dev, "reg property is invalid\n");
474 goto child_out;
475 }
476
477 ret = lp50xx_probe_leds(child, priv, led, ret);
478 if (ret)
479 goto child_out;
480
481 init_data.fwnode = child;
482 num_colors = 0;
483
484 /*
485 * There are only 3 LEDs per module otherwise they should be
486 * banked which also is presented as 3 LEDs.
487 */
488 mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
489 sizeof(*mc_led_info), GFP_KERNEL);
490 if (!mc_led_info)
491 return -ENOMEM;
492
493 fwnode_for_each_child_node(child, led_node) {
494 ret = fwnode_property_read_u32(led_node, "color",
495 &color_id);
496 if (ret) {
497 dev_err(priv->dev, "Cannot read color\n");
498 goto child_out;
499 }
500
501 mc_led_info[num_colors].color_index = color_id;
502 num_colors++;
503 }
504
505 led->priv = priv;
506 led->mc_cdev.num_colors = num_colors;
507 led->mc_cdev.subled_info = mc_led_info;
508 led_cdev = &led->mc_cdev.led_cdev;
509 led_cdev->brightness_set_blocking = lp50xx_brightness_set;
510
511 fwnode_property_read_string(child, "linux,default-trigger",
512 &led_cdev->default_trigger);
513
514 ret = devm_led_classdev_multicolor_register_ext(&priv->client->dev,
515 &led->mc_cdev,
516 &init_data);
517 if (ret) {
518 dev_err(&priv->client->dev, "led register err: %d\n",
519 ret);
520 goto child_out;
521 }
522 i++;
523 fwnode_handle_put(child);
^^^^^^^^^^^^^^^^^^^^^^^^^
This will call software_node_put() which calls kobject_put().
524 }
525
526 return 0;
527
528 child_out:
529 fwnode_handle_put(child);
^^^^^^^^^^^^^^^^^^^^^^^^
Same here.
530 return ret;
531 }
regards,
dan carpenter
On Tue, Sep 22, 2020 at 11:05:15PM +0200, Christophe JAILLET wrote:
> In case of memory allocation failure, we must release some resources as
> done in all other error handling paths of the function.
>
> 'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
> is called when we break out of a 'device_for_each_child_node' loop.
>
> Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/leds/leds-lp50xx.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index 47144a37cb94..8178782f2a8a 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -487,8 +487,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
> */
> mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
> sizeof(*mc_led_info), GFP_KERNEL);
> - if (!mc_led_info)
> - return -ENOMEM;
> + if (!mc_led_info) {
> + ret = -ENOMEM;
> + goto child_out;
> + }
>
> fwnode_for_each_child_node(child, led_node) {
> ret = fwnode_property_read_u32(led_node, "color",
> --
> 2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-09-23 13:35 ` Dan Carpenter
@ 2020-09-23 18:49 ` Christophe JAILLET
2020-09-24 6:49 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Christophe JAILLET @ 2020-09-23 18:49 UTC (permalink / raw)
To: Dan Carpenter, Heikki Krogerus
Cc: pavel, dmurphy, jacek.anaszewski, linux-leds, linux-kernel,
kernel-janitors
Le 23/09/2020 à 15:35, Dan Carpenter a écrit :
> I've added Heikki Krogerus to the CC list because my question is mostly
> about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
> the firmware node framework").
>
> I have been trying to teach Smatch to understand reference counting so
> it can discover these kinds of bugs automatically.
>
> I don't know how software_node_get_next_child() can work when it doesn't
> call kobject_get(). This sort of bug would have been caught in testing
> because it affects the success path so I must be reading the code wrong.
>
I had the same reading of the code and thought that I was missing
something somewhere.
There is the same question about 'acpi_get_next_subnode' which is also a
'.get_next_child_node' function, without any ref counting, if I'm correct.
CJ
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-09-23 18:49 ` Christophe JAILLET
@ 2020-09-24 6:49 ` Dan Carpenter
2020-09-28 11:53 ` Heikki Krogerus
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2020-09-24 6:49 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Heikki Krogerus, pavel, dmurphy, jacek.anaszewski, linux-leds,
linux-kernel, kernel-janitors
On Wed, Sep 23, 2020 at 08:49:56PM +0200, Christophe JAILLET wrote:
> Le 23/09/2020 à 15:35, Dan Carpenter a écrit :
> > I've added Heikki Krogerus to the CC list because my question is mostly
> > about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
> > the firmware node framework").
> >
> > I have been trying to teach Smatch to understand reference counting so
> > it can discover these kinds of bugs automatically.
> >
> > I don't know how software_node_get_next_child() can work when it doesn't
> > call kobject_get(). This sort of bug would have been caught in testing
> > because it affects the success path so I must be reading the code wrong.
> >
>
> I had the same reading of the code and thought that I was missing something
> somewhere.
>
> There is the same question about 'acpi_get_next_subnode' which is also a
> '.get_next_child_node' function, without any ref counting, if I'm correct.
>
Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
stuff so it's not a problem. (Presumably there is some other sort of
refcounting policy there).
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-09-24 6:49 ` Dan Carpenter
@ 2020-09-28 11:53 ` Heikki Krogerus
2020-11-25 10:46 ` Pavel Machek
0 siblings, 1 reply; 9+ messages in thread
From: Heikki Krogerus @ 2020-09-28 11:53 UTC (permalink / raw)
To: Dan Carpenter
Cc: Christophe JAILLET, pavel, dmurphy, jacek.anaszewski, linux-leds,
linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]
On Thu, Sep 24, 2020 at 09:49:32AM +0300, Dan Carpenter wrote:
> On Wed, Sep 23, 2020 at 08:49:56PM +0200, Christophe JAILLET wrote:
> > Le 23/09/2020 à 15:35, Dan Carpenter a écrit :
> > > I've added Heikki Krogerus to the CC list because my question is mostly
> > > about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
> > > the firmware node framework").
> > >
> > > I have been trying to teach Smatch to understand reference counting so
> > > it can discover these kinds of bugs automatically.
> > >
> > > I don't know how software_node_get_next_child() can work when it doesn't
> > > call kobject_get(). This sort of bug would have been caught in testing
> > > because it affects the success path so I must be reading the code wrong.
> > >
> >
> > I had the same reading of the code and thought that I was missing something
> > somewhere.
> >
> > There is the same question about 'acpi_get_next_subnode' which is also a
> > '.get_next_child_node' function, without any ref counting, if I'm correct.
> >
>
> Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
> stuff so it's not a problem. (Presumably there is some other sort of
> refcounting policy there).
OK, so I guess we need to make software_node_get_next_child()
mimic the behaviour of of_get_next_available_child(), and not
acpi_get_next_subnode(). Does the attached patch work?
thanks,
--
heikki
[-- Attachment #2: 0001-software-nodes-Handle-the-refcounting-also-in-softwa.patch --]
[-- Type: text/plain, Size: 1765 bytes --]
From 9b5744450d07b1e6e32e441785b9b69d7e54a7b1 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 28 Sep 2020 14:38:09 +0300
Subject: [PATCH] software nodes: Handle the refcounting also in
software_node_get_next_child()
Incrementing the reference count of the node that is
returned in software_node_get_next_child(), and decrementing
the reference count of the previous node.
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
drivers/base/swnode.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 010828fc785bc..adbaafab3887b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -439,18 +439,26 @@ static struct fwnode_handle *
software_node_get_next_child(const struct fwnode_handle *fwnode,
struct fwnode_handle *child)
{
- struct swnode *p = to_swnode(fwnode);
- struct swnode *c = to_swnode(child);
+ struct swnode *parent = to_swnode(fwnode);
+ struct swnode *prev = to_swnode(child);
+ struct swnode *next;
- if (!p || list_empty(&p->children) ||
- (c && list_is_last(&c->entry, &p->children)))
+ if (!parent || list_empty(&parent->children))
return NULL;
- if (c)
- c = list_next_entry(c, entry);
- else
- c = list_first_entry(&p->children, struct swnode, entry);
- return &c->fwnode;
+ if (prev && list_is_last(&prev->entry, &parent->children)) {
+ kobject_put(&prev->kobj);
+ return NULL;
+ }
+
+ if (prev) {
+ next = list_next_entry(prev, entry);
+ kobject_put(&prev->kobj);
+ } else {
+ next = list_first_entry(&parent->children, struct swnode, entry);
+ }
+
+ return fwnode_handle_get(&next->fwnode);
}
static struct fwnode_handle *
--
2.28.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-09-28 11:53 ` Heikki Krogerus
@ 2020-11-25 10:46 ` Pavel Machek
2020-11-25 12:16 ` Heikki Krogerus
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Machek @ 2020-11-25 10:46 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Dan Carpenter, Christophe JAILLET, dmurphy, jacek.anaszewski,
linux-leds, linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]
Hi!
> > > > I have been trying to teach Smatch to understand reference counting so
> > > > it can discover these kinds of bugs automatically.
> > > >
> > > > I don't know how software_node_get_next_child() can work when it doesn't
> > > > call kobject_get(). This sort of bug would have been caught in testing
> > > > because it affects the success path so I must be reading the code wrong.
> > > >
> > >
> > > I had the same reading of the code and thought that I was missing something
> > > somewhere.
> > >
> > > There is the same question about 'acpi_get_next_subnode' which is also a
> > > '.get_next_child_node' function, without any ref counting, if I'm correct.
> > >
> >
> > Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
> > stuff so it's not a problem. (Presumably there is some other sort of
> > refcounting policy there).
>
> OK, so I guess we need to make software_node_get_next_child()
> mimic the behaviour of of_get_next_available_child(), and not
> acpi_get_next_subnode(). Does the attached patch work?
Does not sound unreasonable. Did it get solved, somehow?
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET
2020-09-23 13:27 ` Dan Murphy
2020-09-23 13:35 ` Dan Carpenter
@ 2020-11-25 11:01 ` Pavel Machek
2 siblings, 0 replies; 9+ messages in thread
From: Pavel Machek @ 2020-11-25 11:01 UTC (permalink / raw)
To: Christophe JAILLET
Cc: dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 607 bytes --]
On Tue 2020-09-22 23:05:15, Christophe JAILLET wrote:
> In case of memory allocation failure, we must release some resources as
> done in all other error handling paths of the function.
>
> 'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
> is called when we break out of a 'device_for_each_child_node' loop.
>
> Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Thanks, applied.
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
2020-11-25 10:46 ` Pavel Machek
@ 2020-11-25 12:16 ` Heikki Krogerus
0 siblings, 0 replies; 9+ messages in thread
From: Heikki Krogerus @ 2020-11-25 12:16 UTC (permalink / raw)
To: Pavel Machek
Cc: Dan Carpenter, Christophe JAILLET, dmurphy, jacek.anaszewski,
linux-leds, linux-kernel, kernel-janitors
On Wed, Nov 25, 2020 at 11:46:29AM +0100, Pavel Machek wrote:
> Hi!
>
> > > > > I have been trying to teach Smatch to understand reference counting so
> > > > > it can discover these kinds of bugs automatically.
> > > > >
> > > > > I don't know how software_node_get_next_child() can work when it doesn't
> > > > > call kobject_get(). This sort of bug would have been caught in testing
> > > > > because it affects the success path so I must be reading the code wrong.
> > > > >
> > > >
> > > > I had the same reading of the code and thought that I was missing something
> > > > somewhere.
> > > >
> > > > There is the same question about 'acpi_get_next_subnode' which is also a
> > > > '.get_next_child_node' function, without any ref counting, if I'm correct.
> > > >
> > >
> > > Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
> > > stuff so it's not a problem. (Presumably there is some other sort of
> > > refcounting policy there).
> >
> > OK, so I guess we need to make software_node_get_next_child()
> > mimic the behaviour of of_get_next_available_child(), and not
> > acpi_get_next_subnode(). Does the attached patch work?
>
> Does not sound unreasonable. Did it get solved, somehow?
Has anybody tested my patch?
thanks,
--
heikki
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-25 12:16 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET
2020-09-23 13:27 ` Dan Murphy
2020-09-23 13:35 ` Dan Carpenter
2020-09-23 18:49 ` Christophe JAILLET
2020-09-24 6:49 ` Dan Carpenter
2020-09-28 11:53 ` Heikki Krogerus
2020-11-25 10:46 ` Pavel Machek
2020-11-25 12:16 ` Heikki Krogerus
2020-11-25 11:01 ` Pavel Machek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).