Re: [PATCH v4 09/12] leds: trigger: Add led_trigger_notify_hw_control_changed() interface

On Sun, 02 Aug 2026, [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The `led_trigger_notify_hw_control_changed()` API performs sleepable operations without an internal deferral mechanism, creating a trap for driver authors who might call it directly from hardware interrupt handlers.

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

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

> --
> 
> --- Patch [9]: [PATCH v4 09/12] leds: trigger: Add led_trigger_notify_hw_control_changed() interface ---
> commit 21d51858b3771f2be6da6b9704657f9342549cfc
> Author: Rong Zhang <[email protected]>
> 
> leds: trigger: Add led_trigger_notify_hw_control_changed() interface
> 
> This commit introduces a new interface, led_trigger_notify_hw_control_changed(),
> allowing LED drivers to notify the LED core about autonomous hardware
> control transitions. The core will then transition the trigger between
> "none" and the device's private trigger.
> 
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index 64bc95d0bfeb7..2db9fcd1350b8 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> [ ... ]
> > @@ -471,6 +492,63 @@ int devm_led_trigger_register(struct device *dev,
> [ ... ]
> > +void led_trigger_notify_hw_control_changed(struct led_classdev *led_cdev, bool activate)
> > +{
> > +	struct led_trigger *trig;
> > +
> > +	/* Restricted to private triggers. */
> > +	if (WARN_ON(!(led_cdev->flags & LED_TRIG_HW_CHANGED) ||
> > +		    !led_cdev->hw_control_trigger || !led_cdev->trigger_type))
> > +		return;
> > +
> > +	scoped_guard(rwsem_read, &triggers_list_lock) {
> 
> [Severity: Medium]
> Will driver authors likely call this API directly from hardware interrupt
> handlers when an autonomous hardware control transition occurs?
> 
> This function acquires a sleepable rw_semaphore (triggers_list_lock) and
> can subsequently call __led_trigger_set() which uses synchronize_rcu().
> Calling this from an atomic context like a hardirq handler would trigger a
> 'sleeping in atomic context' bug or a deadlock.
> 
> Since there is no internal workqueue deferral mechanism (unlike
> led_trigger_event()) and no might_sleep() annotation, could this become a
> trap for driver authors?
> 
> > +		list_for_each_entry(trig, &trigger_list, next_trig) {
> > +			if (trig->trigger_type == led_cdev->trigger_type &&
> > +			    !strcmp(trig->name, led_cdev->hw_control_trigger)) {
> > +				guard(rwsem_write)(&led_cdev->trigger_lock);
> > +
> > +				led_trigger_do_hw_control_transition(led_cdev, activate, trig);
> > +				return;
> > +			}
> > +		}
> > +	}
> > +
> > +	dev_err(led_cdev->dev,
> > +		"%s() is called, but the private trigger (%s) is not properly registered\n",
> > +		__func__, led_cdev->hw_control_trigger);
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
> 

-- 
Lee Jones