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

Hi Lee,

On Tue, 2026-08-11 at 20:13 +0100, Lee Jones wrote:
> 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.

For PATCH 4 & 5, what Sashiko found were pre-existing bugs on probe,
i.e., non-idempotent trigger registration. Since both cros_ec and turris-
omnia are very platform-specific and the relevant platforms never have
more than one such device, the bug is never triggered in reality. Thus,
fixing them is far beyond the scope of the series.

For PATCH 8, I've replied there.

For PATCH 9 (this), please see below.

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

For the very first users of the interface, i.e., ideapad-laptop and
(supposedly) thinkpad_acpi, it's not an issue as ACPI notifier handlers
are executed in work contexts.

To implement an internal workqueue deferral mechanism for atomic
contexts, we will have to introduce:
- a work_struct
- an atomic_t storing the last value of the `activate' argument
- extra synchronization on unregistration

It makes things more unnecessarily tedious just to take care of (yet)
non-existent atomic callers.

In fact, 9 out of 11 callers of the similar interface for hardware
brightness change, i.e., led_classdev_notify_brightness_hw_changed(), are
platform/x86 drivers and call it from sleepable contexts.

Therefore, I personally prefer adding a might_sleep() annotation, and we
could reconsider when an atomic caller emerges. If you prefer the other
way, I am also glad to implement it.

Thanks,
Rong

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