Re: [PATCH 3/3] backlight: lp8864: Convert from LED to backlight class driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hardcoded backlight device name prevents probing multiple device instances.
- [High] Unclamped device tree brightness properties bypass limits and cause hardware wraparound.
- [High] The LED class interface completely ignores DT backlight limits, allowing userspace to overdrive the display.
- [Medium] Concurrent sysfs writes from the backlight and LED subsystems cause a data race on `priv->led_status_mask`.
--

commit 3a876b5316e78acf44b2217cdc076c3285471e8a
Author: Alexander Sverdlin <alexander.sverdlin@gmail.com>

backlight: lp8864: Convert from LED to backlight class driver
    
Move the TI LP8864/LP8866 driver from drivers/leds/ to drivers/video/backlight/
and convert it to register a backlight class device. The driver preserves
backward-compatible LED class device registration if the "led" child node is
present.

> diff --git a/drivers/video/backlight/lp8864_bl.c b/drivers/video/backlight/lp8864_bl.c
> index d05211b970c94..67b28f7daedd2 100644
> --- a/drivers/leds/leds-lp8864.c
> +++ b/drivers/video/backlight/lp8864_bl.c

[ ... ]

> @@ -157,28 +162,59 @@ static int lp8864_fault_check(struct lp8864 *priv)
>  	return ret;
>  }
>  
> -static int lp8864_brightness_set(struct led_classdev *led_cdev,
> -				 enum led_brightness brt_val)
> +static int lp8864_brightness_set(struct lp8864 *priv, unsigned int brightness)
>  {
> -	struct lp8864 *priv = container_of(led_cdev, struct lp8864, led_dev);
> -	/* Scale 0..LED_FULL into 16-bit HW brightness */
> -	unsigned int val = brt_val * 0xffff / LED_FULL;
>  	int ret;
>  
>  	ret = lp8864_fault_check(priv);

[Severity: Medium]
Since the driver registers both a backlight device and an LED class
device, can concurrent sysfs writes to both interfaces cause a data race?

The LED and Backlight subsystems use independent locks (led_access mutex
and update_lock mutex). Parallel writes will result in concurrent calls to
lp8864_brightness_set(), which calls lp8864_fault_check().

Inside lp8864_fault_check(), the driver performs an unprotected
read-modify-write on priv->led_status_mask. Could this concurrent access
cause fault status updates to be lost?

>  	if (ret)
>  		return ret;

[ ... ]

> +static int lp8864_led_brightness_set(struct led_classdev *led_cdev,
> +				     enum led_brightness brt_val)
> +{
> +	struct lp8864 *priv = dev_get_drvdata(led_cdev->dev->parent);
> +
> +	/* Scale 0..LED_FULL into 16-bit HW brightness */
> +	return lp8864_brightness_set(priv, brt_val * 0xffff / LED_FULL);
> +}

[Severity: High]
Does the LED class interface ignore the device tree backlight limits?

The backlight device respects props.max_brightness, but the parallel LED
interface scales the requested brightness using a hardcoded LED_FULL divisor
(brt_val * 0xffff / LED_FULL).

If a custom max-brightness is specified in the device tree, writing to
the LED sysfs interface could bypass this limit and write arbitrary high
values to the hardware. Could this overdrive and potentially damage the
display?

[ ... ]

> @@ -258,11 +290,46 @@ static int lp8864_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	/* Register backlight class device */
> +	memset(&props, 0, sizeof(props));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = LP8864_MAX_BRIGHTNESS;
> +	props.brightness = LP8864_MAX_BRIGHTNESS;
> +	props.scale = BACKLIGHT_SCALE_LINEAR;
> +
> +	if (!device_property_read_u32(&client->dev, "max-brightness", &val))
> +		props.max_brightness = val;
> +
> +	if (!device_property_read_u32(&client->dev, "default-brightness", &val))
> +		props.brightness = val;

[Severity: High]
Could reading max-brightness without default-brightness bypass the
protective limits during boot?

If max-brightness is provided in the device tree but default-brightness
is not, props.brightness retains its initial 0xFFFF value. When
backlight_update_status() is called shortly after, it will write this
unconstrained 0xFFFF to the hardware.

Additionally, since device_property_read_u32() is used without upper bound
checks, would values greater than 65535 in the device tree wrap around
when passed to the 16-bit regmap_write() in lp8864_brightness_set()?

> +
> +	bl = devm_backlight_device_register(&client->dev, "lp8864-backlight",
> +					    &client->dev, priv,
> +					    &lp8864_backlight_ops, &props);

[Severity: High]
Will using a fixed string for the backlight device name prevent probing
multiple instances?

The commit message mentions a use case on a hot-pluggable segment of an
I2C bus. If there are multiple LP8864 instances on the same system, the
second instance will fail to probe because the sysfs core mandates unique
names for devices in the same class.

Should this use a dynamically unique name, such as dev_name(&client->dev),
to avoid device_add() returning -EEXIST?

> +	if (IS_ERR(bl))
> +		return dev_err_probe(&client->dev, PTR_ERR(bl),
> +				     "Failed to register backlight device\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615120353.3409035-1-alexander.sverdlin@siemens.com?part=3