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

[Lee Jones <lee@kernel.org>]

On Sun, 13 Sep 2026, sashiko-bot@kernel.org wrote:

> 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.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> 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

-- 
Lee Jones