Hi all, On Mon, 2026-08-17 at 17:18 +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) > to consider: > > New issues: > - [High] The devres registration order for the backlight device and the LED > disable action is reversed, creating a TOCTOU race condition on device > removal that allows the hardware to be improperly turned back on. > - [Medium] Hardcoding `dev_name(dev)` as the backlight device name prevents a > single device from registering multiple LED-backed backlights.
make sense! Will be fixed in v2. > Pre-existing issues: > - [High] Dereferencing a potentially NULL `supplier` pointer inside > `dev_name()` during an error path causes a kernel panic. > -- > > --- Patch [1]: [PATCH 1/2] backlight: led_bl: Add > devm_led_backlight_register() helper --- > commit 9866a4a9924f34ae7a659947d01fcc9dc9744ec2 > Author: Alexander Sverdlin <[email protected]> > > backlight: led_bl: Add devm_led_backlight_register() helper > > This commit factors out the backlight registration into a led_bl_register() > helper and adds devm_led_backlight_register(). This allows self-contained > LED providers to expose a backlight interface. It also switches to > devres-managed functions to handle teardown, removing the explicit .remove > callback. > > > diff --git a/drivers/video/backlight/led_bl.c > > b/drivers/video/backlight/led_bl.c > > index f7ab9b3607313..f87a48f74186a 100644 > > --- a/drivers/video/backlight/led_bl.c > > +++ b/drivers/video/backlight/led_bl.c > > [ ... ] > > > @@ -203,24 +200,28 @@ static int led_bl_probe(struct platform_device *pdev) > > props.brightness = priv->default_brightness; > > props.power = (priv->default_brightness > 0) ? BACKLIGHT_POWER_OFF : > > BACKLIGHT_POWER_ON; > > - priv->bl_dev = backlight_device_register(dev_name(&pdev->dev), > > - &pdev->dev, priv, &led_bl_ops, &props); > > - if (IS_ERR(priv->bl_dev)) { > > - dev_err(&pdev->dev, "Failed to register backlight\n"); > > - return PTR_ERR(priv->bl_dev); > > - } > > + priv->bl_dev = devm_backlight_device_register(dev, dev_name(dev), dev, > > + priv, &led_bl_ops, > > &props); > > [Severity: Medium] > Since dev_name(dev) is used for the backlight device name, will this prevent > a single device from registering multiple LED-backed backlights? > > If a single controller driver calls devm_led_backlight_register() more than > once for different LEDs, it seems the second call will fail due to a sysfs > naming collision (-EEXIST) because the name is hardcoded to the parent > device's name. > > > + if (IS_ERR(priv->bl_dev)) > > + return dev_err_probe(dev, PTR_ERR(priv->bl_dev), > > + "Failed to register backlight\n"); > > > > for (i = 0; i < priv->nb_leds; i++) { > > + struct device *supplier = priv->leds[i]->dev->parent; > > struct device_link *link; > > > > - link = device_link_add(&pdev->dev, priv->leds[i]->dev->parent, > > - DL_FLAG_AUTOREMOVE_CONSUMER); > > - if (!link) { > > - dev_err(&pdev->dev, "Failed to add devlink (consumer > > %s, supplier %s)\n", > > - dev_name(&pdev->dev), > > dev_name(priv->leds[i]->dev->parent)); > > - backlight_device_unregister(priv->bl_dev); > > - return -EINVAL; > > - } > > + /* > > + * BL and the LED are the same device if instantiated via > > + * devm_led_backlight_register() > > + */ > > + if (supplier == dev) > > + continue; > > + > > + link = device_link_add(dev, supplier, > > DL_FLAG_AUTOREMOVE_CONSUMER); > > + if (!link) > > + return dev_err_probe(dev, -EINVAL, > > + "Failed to add devlink (consumer > > %s, supplier %s)\n", > > + dev_name(dev), dev_name(supplier)); > > [Severity: High] > This is a pre-existing issue, but could this dev_name(supplier) call trigger a > NULL pointer dereference? > > If an LED classdev without a parent device (where parent = NULL) is passed > to devm_led_backlight_register(), supplier becomes NULL. > > Since device_link_add() unconditionally returns NULL when the supplier is > NULL, the error path will execute and dev_name(supplier) will panic by > dereferencing NULL. > > > } > > > > for (i = 0; i < priv->nb_leds; i++) { > > @@ -229,26 +230,68 @@ static int led_bl_probe(struct platform_device *pdev) > > mutex_unlock(&priv->leds[i]->led_access); > > } > > > > + ret = devm_add_action_or_reset(dev, led_bl_disable, priv); > > + if (ret) > > + return ret; > > [Severity: High] > Does registering the led_bl_disable action here create a race condition > during device removal? > > Because devres actions execute in LIFO order (reverse of registration), > led_bl_disable() will run before the backlight device is unregistered. > > This means led_bl_disable() turns off the LED, but the backlight sysfs nodes > are still active. Could userspace write to the backlight's sysfs brightness > attribute during this window, turning the LED back on just before the driver > is removed, thus leaving the hardware powered on? -- Alexander Sverdlin Siemens AG www.siemens.com
