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

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.

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