Re: [PATCH v7 2/3] leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

[sashiko-bot@kernel.org]

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

New issues:
- [High] Regulator is not enabled when requesting maximum torch brightness, preventing the LED from turning on.
- [High] Replacing mod_timer with schedule_delayed_work silently drops updates to the strobe timeout and causes premature LED power down.
- [Medium] Unbalanced regulator disable due to reliance on the global/physical regulator state instead of the driver's consumer enable count.

Pre-existing issues:
- [Critical] Premature destruction of mutex and cancellation of workqueue during device removal while the LED class device is still registered, leading to a Use-After-Free.
--

commit 6a964f82a818f57f5a1a31366f614609bc9c5008
Author: Rudraksha Gupta <guptarud@gmail.com>

leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

This commit extends the RT8515 driver to support flash ICs that use
only a single GPIO for both flash and torch modes, alongside an
optional vin regulator that gates power to the flash IC.

> diff --git a/drivers/leds/flash/leds-rt8515.c b/drivers/leds/flash/leds-rt8515.c
> index 00904cc90ed6e..541f61e13f018 100644
> --- a/drivers/leds/flash/leds-rt8515.c
> +++ b/drivers/leds/flash/leds-rt8515.c
[ ... ]
> @@ -62,27 +66,50 @@ static struct rt8515 *to_rt8515(struct led_classdev_flash *fled)
>  	return container_of(fled, struct rt8515, fled);
>  }
>  
> -static void rt8515_gpio_led_off(struct rt8515 *rt)
> +static int rt8515_gpio_led_off(struct rt8515 *rt)
>  {
> -	gpiod_set_value(rt->enable_flash, 0);
> -	gpiod_set_value(rt->enable_torch, 0);
> +	gpiod_set_value(rt->enable_flash, RT8515_OFF);
> +	gpiod_set_value(rt->enable_torch, RT8515_OFF);
> +
> +	if (!rt->reg)
> +		return 0;
> +
> +	return regulator_is_enabled(rt->reg) > 0 ? regulator_disable(rt->reg) : 0;

[Severity: Medium]
Since regulator_is_enabled() returns the global physical state rather than
the per-consumer enable count, could this lead to an unbalanced disable if
the regulator is marked always-on or shared with another consumer?

Because rt8515_gpio_led_off() is reachable from user-controllable sysfs
attributes via rt8515_led_brightness_set(), repeated calls when the driver
has not enabled the regulator might cause the regulator core to throw a
warning and return -EIO. Should the driver maintain its own boolean state
to track whether it has enabled the regulator?

>  }
[ ... ]
> @@ -91,23 +118,37 @@ static int rt8515_led_brightness_set(struct led_classdev *led,
>  {
>  	struct led_classdev_flash *fled = lcdev_to_flcdev(led);
>  	struct rt8515 *rt = to_rt8515(fled);
> +	int ret = 0;
>  
>  	mutex_lock(&rt->lock);
>  
>  	if (brightness == LED_OFF) {
> -		/* Off */
> -		rt8515_gpio_led_off(rt);
> +		ret = rt8515_gpio_led_off(rt);
> +		if (ret)
> +			goto out;
>  	} else if (brightness < RT8515_TORCH_MAX) {
> -		/* Step it up to movie mode brightness using the flash pin */
> -		rt8515_gpio_brightness_commit(rt->enable_torch, brightness);
> +		/*
> +		 * Step it up to movie mode brightness.
> +		 * If there is no separate torch pin, use the flash pin
> +		 * for torch as well.
> +		 */
> +		ret = rt8515_gpio_brightness_commit(rt,
> +				rt->enable_torch ?: rt->enable_flash, brightness);
> +		if (ret)
> +			goto out;
>  	} else {
> -		/* Max torch brightness requested */
> -		gpiod_set_value(rt->enable_torch, 1);
> +		/*
> +		 * Max torch brightness requested.
> +		 * If there is no separate torch pin, use the flash pin
> +		 * for torch as well.
> +		 */
> +		gpiod_set_value(rt->enable_torch ?: rt->enable_flash, RT8515_ON);

[Severity: High]
If a transition is made from LED_OFF directly to max torch brightness, the
optional vin regulator remains disabled because
rt8515_gpio_brightness_commit() is completely bypassed here. Does this mean
the max torch state might silently fail to power the IC?

>  	}
>  
> +out:
>  	mutex_unlock(&rt->lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,
> @@ -116,27 +157,35 @@ static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,
[ ... ]
> -	if (state) {
> -		/* Enable LED flash mode and set brightness */
> -		rt8515_gpio_brightness_commit(rt->enable_flash, brightness);
> -		/* Set timeout */
> -		mod_timer(&rt->powerdown_timer,
> -			  jiffies + usecs_to_jiffies(timeout->val));
> -	} else {
> -		timer_delete_sync(&rt->powerdown_timer);
> -		/* Turn the LED off */
> -		rt8515_gpio_led_off(rt);
> -	}
> +	/* Enable LED flash mode and set brightness */
> +	ret = rt8515_gpio_brightness_commit(rt, rt->enable_flash, brightness);
> +	if (ret)
> +		goto out;
> +
> +	/* Set timeout */
> +	schedule_delayed_work(&rt->powerdown_work, usecs_to_jiffies(timeout->val));

[Severity: High]
Does schedule_delayed_work() correctly handle consecutive flash strobe
commands? Unlike mod_timer(), schedule_delayed_work() silently ignores
updates if the work is already pending. This could cause consecutive V4L2
flash strobes to fail to extend the timeout, shutting down the flash
prematurely.

Additionally, if the delayed work is actively executing and blocked waiting
on rt->lock, its pending bit is cleared. In that scenario, could
schedule_delayed_work() queue a duplicate instance of the work that
immediately shuts down the LED as soon as the lock is released? Would using
mod_delayed_work() resolve these state management issues?

>  
>  	fled->led_cdev.brightness = LED_OFF;
>  	/* After this the torch LED will be disabled */
>  
> +out:
>  	mutex_unlock(&rt->lock);
>  
> -	return 0;
> +	return ret;
>  }
[ ... ]
> @@ -327,12 +389,17 @@ static int rt8515_probe(struct platform_device *pdev)
[ ... ]
>  	platform_set_drvdata(pdev, rt);
>  
>  	ret = devm_led_classdev_flash_register_ext(dev, fled, &init_data);

[Severity: Critical]
This is a pre-existing issue, but registering the LED class device via the
devm API schedules its sysfs interfaces to be unregistered after
rt8515_remove() returns.

[ ... ]
> @@ -371,7 +438,7 @@ static void rt8515_remove(struct platform_device *pdev)
>  	struct rt8515 *rt = platform_get_drvdata(pdev);
>  
>  	rt8515_v4l2_flash_release(rt);
> -	timer_delete_sync(&rt->powerdown_timer);
> +	cancel_delayed_work_sync(&rt->powerdown_work);
>  	mutex_destroy(&rt->lock);

Because devres destroys the sysfs interfaces after this function returns,
could a concurrent user-space write to sysfs (like triggering a flash
strobe) occur during this window? This would cause the sysfs handler to
attempt to lock the destroyed mutex and queue work on the freed device
structure, leading to a use-after-free. Should devm_add_action_or_reset()
be used here instead to ensure the work and mutex are only destroyed after
the devm-managed LED registration is cleanly torn down?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-expressatt_camera_flash-v7-0-e1fd2edc8d84@gmail.com?part=2