| Message ID | 20221216113013.126881-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
On Fri, Dec 16, 2022 at 12:30:03PM +0100, Hans de Goede wrote: > led_put() is used to "undo" a successful of_led_get() call, > of_led_get() uses class_find_device_by_of_node() which returns > a reference to the device which must be free-ed with put_device() > when the caller is done with it. > > Add a put_device() call to led_put() to free the reference returned > by class_find_device_by_of_node(). > > And also add a put_device() in the error-exit case of try_module_get() > failing. ... > led_cdev = dev_get_drvdata(led_dev); > > - if (!try_module_get(led_cdev->dev->parent->driver->owner)) > + if (!try_module_get(led_cdev->dev->parent->driver->owner)) { > + put_device(led_cdev->dev); > return ERR_PTR(-ENODEV); > + } > > return led_cdev; ... > void led_put(struct led_classdev *led_cdev) > { > module_put(led_cdev->dev->parent->driver->owner); > + put_device(led_cdev->dev); Hmm... It was in the original submission. https://lore.kernel.org/linux-leds/1443605522-1118-2-git-send-email-tomi.valkeinen@ti.com/ Nevertheless, shouldn't you put device before putting module? (It may need to save the owner of the driver, I think.) > }
On Fri, Dec 16, 2022 at 03:35:29PM +0200, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:03PM +0100, Hans de Goede wrote: ... > > led_cdev = dev_get_drvdata(led_dev); > > > > - if (!try_module_get(led_cdev->dev->parent->driver->owner)) > > + if (!try_module_get(led_cdev->dev->parent->driver->owner)) { > > + put_device(led_cdev->dev); > > return ERR_PTR(-ENODEV); > > + } > > > > return led_cdev; > > ... > > > void led_put(struct led_classdev *led_cdev) > > { > > module_put(led_cdev->dev->parent->driver->owner); > > + put_device(led_cdev->dev); > > Hmm... It was in the original submission. > > https://lore.kernel.org/linux-leds/1443605522-1118-2-git-send-email-tomi.valkeinen@ti.com/ ... > Nevertheless, shouldn't you put device before putting module? (It may need to > save the owner of the driver, I think.) I think this is wrong, the symmetry is kept correct in your patch.
Hi, On 12/16/22 14:55, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 03:35:29PM +0200, Andy Shevchenko wrote: >> On Fri, Dec 16, 2022 at 12:30:03PM +0100, Hans de Goede wrote: > > ... > >>> led_cdev = dev_get_drvdata(led_dev); >>> >>> - if (!try_module_get(led_cdev->dev->parent->driver->owner)) >>> + if (!try_module_get(led_cdev->dev->parent->driver->owner)) { >>> + put_device(led_cdev->dev); >>> return ERR_PTR(-ENODEV); >>> + } >>> >>> return led_cdev; >> >> ... >> >>> void led_put(struct led_classdev *led_cdev) >>> { >>> module_put(led_cdev->dev->parent->driver->owner); >>> + put_device(led_cdev->dev); >> >> Hmm... It was in the original submission. >> >> https://lore.kernel.org/linux-leds/1443605522-1118-2-git-send-email-tomi.valkeinen@ti.com/ > > ... > >> Nevertheless, shouldn't you put device before putting module? (It may need to >> save the owner of the driver, I think.) > > I think this is wrong, the symmetry is kept correct in your patch. Right, the line above dereferences led_cdev->dev, so the put() must be done after that line. Regards, Hans
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 6a8ea94834fa..7391d2cf1370 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -241,8 +241,10 @@ struct led_classdev *of_led_get(struct device_node *np, int index) led_cdev = dev_get_drvdata(led_dev); - if (!try_module_get(led_cdev->dev->parent->driver->owner)) + if (!try_module_get(led_cdev->dev->parent->driver->owner)) { + put_device(led_cdev->dev); return ERR_PTR(-ENODEV); + } return led_cdev; } @@ -255,6 +257,7 @@ EXPORT_SYMBOL_GPL(of_led_get); void led_put(struct led_classdev *led_cdev) { module_put(led_cdev->dev->parent->driver->owner); + put_device(led_cdev->dev); } EXPORT_SYMBOL_GPL(led_put);
led_put() is used to "undo" a successful of_led_get() call, of_led_get() uses class_find_device_by_of_node() which returns a reference to the device which must be free-ed with put_device() when the caller is done with it. Add a put_device() call to led_put() to free the reference returned by class_find_device_by_of_node(). And also add a put_device() in the error-exit case of try_module_get() failing. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/leds/led-class.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)