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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Unratelimited warning log in the GPIO read callback can flood the kernel log.
- [Low] The commit message description after the subsystem prefix is not capitalized.

Pre-existing issues:
- [High] Race condition in driver teardown leaves input workqueue active after device data is freed, causing a Use-After-Free.
--

--- Patch [3]: [PATCH v2 3/4] leds: pca9532: check return value in pca9532_gpio_get_value() ---
commit 40e01af05e0100f229b670bf1dcf0b63a92a90f8
Author: Haibo Chen <haibo.chen@nxp.com>

leds: pca9532: check return value in pca9532_gpio_get_value()

pca9532_gpio_get_value() reads the input register with
i2c_smbus_read_byte_data() but stores the result in an unsigned char and
never checks for failure. On an I2C read error the negative error code
(e.g. -EIO) is truncated to an unsigned byte, and a bit is extracted and
returned as a valid GPIO level. This masks hardware errors and lets
kernel or userspace consumers silently receive corrupt data.

> leds: pca9532: check return value in pca9532_gpio_get_value()

[Severity: Low]
This isn't a bug, but the subsystem guidelines request capitalizing the
description after the prefix. Should "check" be capitalized to "Check"?

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index adfb5aa50e8e1..cee398d8275b8 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -345,9 +345,14 @@ static int pca9532_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
>  static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned offset)
>  {
>  	struct pca9532_data *data = gpiochip_get_data(gc);
> -	unsigned char reg;
> +	int reg;
>  
>  	reg = i2c_smbus_read_byte_data(data->client, PCA9532_REG_INPUT(offset));
> +	if (reg < 0) {
> +		dev_warn(&data->client->dev,
> +			 "failed to read input register: %d\n", reg);

[Severity: Medium]
Could this dev_warn() flood the kernel log if the I2C bus fails while
userspace is actively polling the GPIO value? Would it be safer to use
dev_warn_ratelimited() here?

> +		return reg;
> +	}
>  
>  	return !!(reg & (1 << (offset % 8)));
>  }

[Severity: High]
This is a pre-existing issue, but does driver teardown leave the input
workqueue active after device data is freed, causing a use-after-free?

Looking at pca9532_destroy_devices(), cancel_work_sync() is called but
the input device remains registered because it relies on devres for
unregistration:

drivers/leds/leds-pca9532.c:pca9532_destroy_devices() {
    ...
    cancel_work_sync(&data->work);
    data->idev = NULL;
    ...
}

If an event comes in after pca9532_remove() finishes but before devres
cleans up the input device, could pca9532_event() blindly reschedule
the work?

drivers/leds/leds-pca9532.c:pca9532_event() {
    ...
    schedule_work(&data->work);
    ...
}

Since devres unregisters the device and frees data, the scheduled work
could be left dangling. When pca9532_input_work() finally executes,
would it dereference the freed data pointer?

drivers/leds/leds-pca9532.c:pca9532_input_work() {
    ...
    mutex_lock(&data->update_lock);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-led-fix-v2-0-09ad218457bd@nxp.com?part=3