Re: [PATCH 2/2] backlight: Add Awinic AW99703 backlight driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] build: Incompatible pointer type passed to device_property_read_u32()
- [Medium] gpio: Hardware left powered on upon early probe failure
- [Low] backlight: TOCTOU race in aw99703_shutdown() missing update_lock
--

commit b010e2c6526192785d6b65bcadf761f3928c7acd
Author: Luka Panio <lukapanio@gmail.com>

backlight: Add Awinic AW99703 backlight driver

Add a driver for the Awinic AW99703 3-string WLED backlight controller.

> diff --git a/drivers/video/backlight/aw99703.c b/drivers/video/backlight/aw99703.c
> new file mode 100644
> index 0000000000000..b0a46753d1765
> --- /dev/null
> +++ b/drivers/video/backlight/aw99703.c
[ ... ]
> +static void aw99703_dt_parse(struct aw99703_device *aw,
> +			     struct backlight_properties *props)
> +{
[ ... ]
> +	props->max_brightness = AW99703_MAX_BRT_LVL;
> +	device_property_read_u32(aw->dev, "max-brightness",
> +				 &props->max_brightness);

[Severity: High]
Does this code trigger a -Wincompatible-pointer-types build failure?

device_property_read_u32() expects a u32 * as its third argument, but
&props->max_brightness is a pointer to int. In recent kernel builds, this
warning is promoted to a fatal error.

> +	if (props->max_brightness > AW99703_MAX_BRT_LVL)
> +		props->max_brightness = AW99703_MAX_BRT_LVL;
> +
> +	props->brightness = props->max_brightness >> 1;
> +	device_property_read_u32(aw->dev, "default-brightness",
> +				 &props->brightness);

[Severity: High]
Similarly, &props->brightness is also a pointer to int.

[ ... ]
> +static void aw99703_power_off(void *data)
> +{
> +	struct aw99703_device *aw = data;
> +
> +	aw99703_update_brightness(aw, 0);
> +	gpiod_set_value_cansleep(aw->hwen_gpio, 0);
> +}
> +
> +static int aw99703_probe(struct i2c_client *client)
> +{
[ ... ]
> +	if (aw->hwen_gpio) {
> +		ret = gpiod_direction_output(aw->hwen_gpio, 1);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to drive enable gpio\n");
> +		fsleep(AW99703_HWEN_ON_DELAY_US);
> +	}
> +
> +	ret = aw99703_chip_id_read(aw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "chip id check failed\n");

[Severity: Medium]
If the chip ID read fails, does this return without de-asserting the GPIO?

The probe function sets the hwen_gpio output to 1 to power on the chip,
but if aw99703_chip_id_read() or aw99703_hw_init() fail, it returns an
error code immediately. Since the devm_add_action_or_reset() for
aw99703_power_off is registered later in the function, the cleanup
action won't execute on early probe failure, leaving the hardware
permanently enabled.

> +
> +	props.type = BACKLIGHT_RAW;
> +	aw99703_dt_parse(aw, &props);
> +	aw99703_read_boot_brightness(aw, &props);
> +
> +	ret = aw99703_hw_init(aw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize chip\n");
> +
> +	ret = devm_add_action_or_reset(dev, aw99703_power_off, aw);
[ ... ]
> +static void aw99703_shutdown(struct i2c_client *client)
> +{
> +	aw99703_power_off(i2c_get_clientdata(client));
> +}

[Severity: Low]
Is there a data race between this shutdown callback and concurrent sysfs
writes to the brightness attribute?

aw99703_shutdown() invokes aw99703_power_off(), which modifies the
aw->bl_enable flag and writes to I2C registers via aw99703_update_brightness()
without acquiring the backlight bd->update_lock mutex. Since userspace tasks
are not frozen during system shutdown, a concurrent sysfs operation could
attempt to communicate with the chip while its registers are being written
or after the power pin is de-asserted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016162912.64581-1-lukapanio@gmail.com?part=2