Re: [PATCH v2] leds: flash: sgm3140: fix child node reference leak

[Guangshuo Li]

Hi Laurent,

On Mon, 21 Sept 2026 at 19:54, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Why did you submit a v2 ignoring my latest comments on v1 ?
>
> On Mon, Sep 21, 2026 at 05:21:28PM +0800, Guangshuo Li wrote:
> > sgm3140_probe() obtains a reference to the LED child node with
> > device_get_next_child_node(), but the reference is not released after a
> > successful probe.
> >
> > The child node is also passed to devm_led_classdev_flash_register_ext().
> > The LED class device stores the fwnode without taking a reference of its
> > own, so the reference obtained by sgm3140_probe() must remain valid until
> > the LED class device is unregistered.
> >
> > Manage the child node reference with a devm action registered before the
> > LED class device. This ensures that the LED class device is unregistered
> > before the child node reference is dropped, while also handling probe
> > failure and device removal.
> >
> > Fixes: cef8ec8cbd21 ("leds: add sgm3140 driver")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> > ---
> > v2:
> >   - Keep the child node reference alive until the LED class device is
> >     unregistered, as pointed out by Laurent Pinchart.
> >   - Manage the reference with a devm action registered before the LED
> >     class device registration.
> >
> >  drivers/leds/flash/leds-sgm3140.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/leds/flash/leds-sgm3140.c b/drivers/leds/flash/leds-sgm3140.c
> > index dc6840357370..973676b1dbb1 100644
> > --- a/drivers/leds/flash/leds-sgm3140.c
> > +++ b/drivers/leds/flash/leds-sgm3140.c
> > @@ -182,6 +182,11 @@ static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
> >  }
> >  #endif
> >
> > +static void sgm3140_fwnode_put(void *data)
> > +{
> > +     fwnode_handle_put(data);
> > +}
> > +
> >  static int sgm3140_probe(struct platform_device *pdev)
> >  {
> >       struct sgm3140 *priv;
> > @@ -220,6 +225,10 @@ static int sgm3140_probe(struct platform_device *pdev)
> >                       "No fwnode child node found for connected LED.\n");
> >               return -EINVAL;
> >       }
> > +     ret = devm_add_action_or_reset(&pdev->dev, sgm3140_fwnode_put,
> > +                                    child_node);
> > +     if (ret)
> > +             return ret;
> >
> >       ret = fwnode_property_read_u32(child_node, "flash-max-timeout-us",
> >                                      &priv->max_timeout);
> > @@ -276,7 +285,6 @@ static int sgm3140_probe(struct platform_device *pdev)
> >       return ret;
> >
> >  err:
> > -     fwnode_handle_put(child_node);
> >       return ret;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart

You're right. I sent v2 too quickly and failed to take your latest
comment on v1 into account. Sorry about that.

What led me to this issue was the documentation for
device_get_next_child_node(), which says the caller must call
fwnode_handle_put() on the returned fwnode. I focused too narrowly on
balancing that reference and missed the lifetime implications of passing
it to the LED core.

Would it make more sense for the LED core to take its own reference to
init_data->fwnode when associating it with the LED device, and keep that
reference for the lifetime of the LED class device? The driver could then
drop the temporary reference obtained from device_get_next_child_node()
after registration.

Does that sound like the right ownership model?

Thanks,
Guangshuo