Re: [PATCH] Input: lm8323 - cancel PWM work before releasing device data

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

New issues:
- [High] Canceling the PWM work synchronously during teardown leaves the LED permanently ON.

Pre-existing issues:
- [High] The LED brightness_set callback unconditionally acquires a mutex, causing a kernel panic when triggered from an atomic context.
--

--- Patch [1]: [PATCH] Input: lm8323 - cancel PWM work before releasing device data ---
commit c5dd51a9a738c81ec9f9a4442ce237316cd3f671
Author: Hongyan Xu <[email protected]>

Input: lm8323 - cancel PWM work before releasing device data

LED class device teardown sets the LED brightness to off. The LM8323
brightness callback responds by scheduling its private PWM work, while
LED core only flushes the class device's own work item.

> diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
> index 8eac63b2d3046..88981de9a90fb 100644
> --- a/drivers/input/keyboard/lm8323.c
> +++ b/drivers/input/keyboard/lm8323.c
> @@ -548,6 +548,13 @@ static struct attribute *lm8323_pwm_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(lm8323_pwm);
>  
> +static void lm8323_cancel_pwm_work(void *data)
> +{
> +	struct lm8323_pwm *pwm = data;
> +
> +	cancel_work_sync(&pwm->work);

[Severity: High]
During device unbind, devres executes teardown actions in LIFO order.
led_classdev_unregister() attempts to turn off the LED by calling the
driver's brightness_set callback, which asynchronously schedules pwm->work.

Immediately after, this newly added devm action calls
cancel_work_sync(&pwm->work). Because the work was just queued, it is
synchronously canceled without executing, and the I2C commands to shut off the
hardware LED are never dispatched. This leaves the hardware LED powered on
indefinitely after the driver is unbound.

Should flush_work(&pwm->work) be used instead of cancel_work_sync() here to
ensure the shutdown commands reach the hardware?

> +}
> +
>  static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
>  		    const char *name)
>  {
> @@ -568,6 +575,10 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
>  	mutex_init(&pwm->lock);
>  	pwm->chip = lm;
>  
> +	err = devm_add_action_or_reset(dev, lm8323_cancel_pwm_work, pwm);
> +	if (err)
> +		return err;
> +
>  	if (name) {
>  		pwm->cdev.name = name;
>  		pwm->cdev.brightness_set = lm8323_pwm_set_brightness;

[Severity: High]
This is a pre-existing issue, but this callback implementation appears
problematic for atomic contexts.

The LED core allows brightness_set to be called directly from atomic contexts
(e.g., by the timer or heartbeat triggers running in softirq context).
However, lm8323_pwm_set_brightness() unconditionally acquires a mutex at the
start of the function:

drivers/input/keyboard/lm8323.c:lm8323_pwm_set_brightness() {
    ...
    scoped_guard(mutex, &pwm->lock) {
        pwm->desired_brightness = brightness;
    }
    ...
}

Does acquiring this mutex unconditionally before checking in_interrupt() cause
a 'scheduling while atomic' panic if used with a trigger like timer?

>  		pwm->cdev.groups = lm8323_pwm_groups;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1