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

[sashiko-bot@kernel.org]

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

--- Patch [9]: [PATCH v4 09/12] leds: trigger: Add led_trigger_notify_hw_control_changed() interface ---
commit 21d51858b3771f2be6da6b9704657f9342549cfc
Author: Rong Zhang <i@rong.moe>

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/20260802-leds-trigger-hw-changed-v4-0-f97e2ca976fe@rong.moe?part=9