Re: [PATCH] leds: pca9532: Fix phantom device registration on missing hardware

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

Pre-existing issues:
- [Critical] Race condition in driver teardown leading to a use-after-free on the `data` struct due to delayed unregistration of the devres-managed input device.
- [High] Runtime I2C read errors are truncated and incorrectly treated as valid hardware state, leading to silent hardware state corruption.
- [Low] The driver uses the unmanaged `led_classdev_register()` API instead of the subsystem-mandated managed alternative.
- [Low] Reliance on the deprecated `label` Device Tree property instead of `color` and `function`.
- [Low] The driver prints explicit success/info messages to the kernel log upon successful probing and GPIO initialization.
- [Low] Failure to use `dev_err_probe` for error paths in the probe routine.
--

--- Patch [1]: [PATCH] leds: pca9532: Fix phantom device registration on missing hardware ---
commit 3a305c67b3d567810075afd8ccec5a5e5fff8780
Author: Cosmo Chou <[email protected]>

leds: pca9532: Fix phantom device registration on missing hardware

The initial PWM and PSC register writes in pca9532_configure() do not
check the return values of i2c_smbus_write_byte_data(). If the I2C
device is physically absent from the bus, the write fails with -ENXIO.
However, the driver ignores this error and allows probe() to complete
successfully.

This results in the registration of phantom LED class devices and
gpiochips backed by non-existent hardware. Subsequent GPIO reads from
these phantom chips return bogus values (due to -ENXIO being truncated
to an unsigned char in pca9532_gpio_get_value()), silently corrupting
hardware state tracking in userspace.

Propagate the I2C write failures back to probe() so the driver core
can gracefully abort binding and release devres-managed resources.

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index 2d37e00e459de..b2e081c8f1393 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -397,10 +397,14 @@ static int pca9532_configure(struct i2c_client *client,
>  	for (i = 0; i < 2; i++)	{
>  		data->pwm[i] = pdata->pwm[i];
>  		data->psc[i] = pdata->psc[i];
> -		i2c_smbus_write_byte_data(client, PCA9532_REG_PWM(maxleds, i),
> -			data->pwm[i]);
> -		i2c_smbus_write_byte_data(client, PCA9532_REG_PSC(maxleds, i),
> -			data->psc[i]);
> +		err = i2c_smbus_write_byte_data(client, PCA9532_REG_PWM(maxleds, i),
> +						data->pwm[i]);
> +		if (err < 0)
> +			return err;
> +		err = i2c_smbus_write_byte_data(client, PCA9532_REG_PSC(maxleds, i),
> +						data->psc[i]);
> +		if (err < 0)
> +			return err;
>  	}
>  
>  	data->hw_blink = true;

[Severity: Critical]
This isn't a bug introduced by this patch, but could this driver teardown
sequence lead to a use-after-free?

In pca9532_destroy_devices(), the workqueue is canceled:

pca9532_destroy_devices() {
    [ ... ]
    cancel_work_sync(&data->work);
    data->idev = NULL;
    [ ... ]
}

However, the input device is devm-managed and remains active until devres
unwinds later. If a user-space process sends an event (like EV_SND) during
this window, pca9532_event() can still be called:

pca9532_event() {
    [ ... ]
    schedule_work(&data->work);
    [ ... ]
}

This schedules work on the data structure just before devres frees it.
When the delayed work executes, will it dereference the freed data structure?

[Severity: High]
This is a pre-existing issue, but does the driver mishandle hardware read
errors here?

In pca9532_setled(), the code reads from the hardware register:

pca9532_setled() {
    [ ... ]
    reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
    /* zero led bits */
    reg = reg & ~LED_MASK(led->id);
    [ ... ]
}

If a transient I2C error occurs, i2c_smbus_read_byte_data() can return a
negative error code like -ENXIO. This signed error code is then used as
valid data, masked, modified, and written back to the device.

A similar situation occurs in pca9532_gpio_get_value():

pca9532_gpio_get_value() {
    [ ... ]
    reg = i2c_smbus_read_byte_data(data->client, PCA9532_REG_INPUT(offset));
    return !!(reg & (1 << (offset % 8)));
}

The negative error code is truncated, which might lead to false positive
or negative GPIO bit readings. Could this silently corrupt the hardware
state of other LEDs sharing the same register?

[Severity: Low]
This isn't a bug introduced by this patch, but does this registration comply
with the LED subsystem guidelines?

pca9532_configure() {
    [ ... ]
    err = led_classdev_register(&client->dev, &led->ldev);
    [ ... ]
}

The guidelines dictate that drivers should use managed APIs like
devm_led_classdev_register() to prevent resource leaks and simplify
lifecycle management.

[Severity: Low]
This is a pre-existing issue, but does the device tree parsing rely on
deprecated properties?

pca9532_of_populate_pdata() {
    [ ... ]
    if (of_property_read_string(child, "label",
                                &pdata->leds[i].name))
    [ ... ]
}

The LED subsystem deprecates the label property in favor of color and
function bindings. Extracting the legacy label property directly ignores
the standard naming conventions.

[Severity: Low]
This isn't a bug introduced by this patch, but do we need these explicit
success messages in the kernel log?

pca9532_probe() {
    [ ... ]
    dev_info(&client->dev, "setting platform data\n");
    [ ... ]
}

pca9532_configure() {
    [ ... ]
    dev_info(&client->dev, "gpios %i...%i\n",
    [ ... ]
}

The LED subsystem guidelines mention that drivers should not print
success log messages upon successful probing or GPIO initialization.

[Severity: Low]
This is a pre-existing issue, but shouldn't this error path use
dev_err_probe()?

pca9532_probe() {
    [ ... ]
    dev_err(&client->dev, "no platform data\n");
    return -EINVAL;
    [ ... ]
}

The LED subsystem guidelines mandate using dev_err_probe() to handle
probe failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1