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

> -----Original Message-----
> From: Lee Jones <[email protected]>
> Sent: 2026年8月6日 22:22
> To: [email protected]
> Cc: Bough Chen (OSS) <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: 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.

Hi Lee, 
Thanks for reviewing the patches.
I'll take care of the review comments and prepare a new revision. Unfortunately, I may not be able to get back to this for another 2-3 weeks due to other commitments.
Thanks for your patience.

Best regards,
Bough
> 
> > 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/20260729-led-fix-v2-0-09ad218457bd@nxp.
> > com?part=2
> 
> --
> Lee Jones