Re: [PATCH v2 2/4] leds: pca9532: check return value in pca9532_setled()

On Wed, 29 Jul 2026, [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Medium] The newly added error return values from `pca9532_setled()` and `pca9532_gpio_set_value()` are ignored in the configuration and GPIO direction paths.
> - [Low] The commit message violates the LED subsystem capitalization rule.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> Pre-existing issues:
> - [High] Incorrect teardown ordering of a devm-managed input device and a custom workqueue leads to a Use-After-Free during device removal.
> --
> 
> --- Patch [2]: [PATCH v2 2/4] leds: pca9532: check return value in pca9532_setled() ---
> commit 3e826891d5b6286d8b08ddc7c668e35465352554
> Author: Haibo Chen <[email protected]>
> 
> leds: pca9532: check return value in pca9532_setled()
> 
> This commit updates pca9532_setled() to check the return value of
> i2c_smbus_read_byte_data(). By checking for read failures, it prevents
> writing bogus bitwise-modified negative error codes back to the device.
> 
> > leds: pca9532: check return value in pca9532_setled()
> 
> [Severity: Low]
> This isn't a bug, but the commit description after the subsystem prefix isn't 
> capitalized. It should probably be "Check return value in pca9532_setled()".
> 
> > diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> > index e8ee17b8bd547..adfb5aa50e8e1 100644
> > --- a/drivers/leds/leds-pca9532.c
> > +++ b/drivers/leds/leds-pca9532.c
> [ ... ]
> > @@ -159,21 +159,29 @@ static int pca9532_setpwm(struct i2c_client *client, int pwm)
> >  }
> >  
> >  /* Set LED routing */
> > -static void pca9532_setled(struct pca9532_led *led)
> > +static int pca9532_setled(struct pca9532_led *led)
> 
> [Severity: Medium]
> Since this function now returns an error code, do we also need to update
> other callers that still ignore it? 
> 
> For example, pca9532_gpio_direction_input() calls pca9532_gpio_set_value()
> (which returns the error from pca9532_setled()), but the error is discarded:
> 
> drivers/leds/leds-pca9532.c:pca9532_gpio_direction_input() {
>     ...
>     pca9532_gpio_set_value(gc, offset, 1);
>     return 0;
> }
> 
> Similarly, pca9532_configure() invokes pca9532_setled() during initialization
> without checking the result, which might leave the hardware in an undefined
> state if the I2C bus fails.
> 
> [ ... ]
> > @@ -257,9 +264,7 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
> >  	if (err)
> >  		return err;
> >  
> > -	pca9532_setled(led);
> > -
> > -	return 0;
> > +	return pca9532_setled(led);
> >  }
> >  
> >  static int pca9532_event(struct input_dev *dev, unsigned int type,
> 
> [Severity: High]
> This is a pre-existing issue, but can this event callback fire after the
> workqueue is cancelled during device removal, causing a use-after-free?
> 
> During driver unbind, pca9532_remove() calls pca9532_destroy_devices()
> which manually cancels the work:
> 
> drivers/leds/leds-pca9532.c:pca9532_destroy_devices() {
>     ...
>     cancel_work_sync(&data->work);
>     data->idev = NULL;
>     ...
> }
> 
> However, because the input device is allocated with
> devm_input_allocate_device(), it remains active and registered until
> devres cleanup runs after .remove() finishes. 
> 
> If userspace injects an EV_SND event concurrently, the input core could
> invoke pca9532_event(), rescheduling the work item via
> schedule_work(&data->work). When the scheduled work executes later, the data
> structure will have already been freed by devres.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
Lee Jones