Re: [PATCH 1/2] leds: lm3530: use brightness_set_blocking for sleepable callback

On Mon, 15 Jun 2026, Runyu Xiao wrote:

> lm3530_brightness_set() talks to the device over I2C and can sleep, but
> the driver registers it as brightness_set.  That leaves the LED core
> free to invoke the callback from atomic contexts.
> 
> This issue was found by our static analysis tool and then manually
> reviewed against the current tree.
> 
> A minimal Lockdep reproducer that keeps the original registration and
> call chain is enough to trigger the warning: lm3530_probe() still
> publishes lm3530_brightness_set() as brightness_set,
> led_trigger_event_atomic() invokes it under spin_lock_irqsave(), and
> the callback reaches i2c_smbus_write_byte_data() as its first
> sleepable edge.
> 
> Lockdep reports:
> 
>   BUG: sleeping function called from invalid context
>   i2c_smbus_write_byte_data.constprop.0+0x14/0x30 [vuln_msv]
>   lm3530_brightness_set+0x4e/0x66 [vuln_msv]
>   led_trigger_event_atomic.constprop.0+0x2b/0x40 [vuln_msv]
> 
> Convert the callback to brightness_set_blocking so the LED core only
> invokes it from a sleepable context.
> 
> Fixes: b1e6b7068f02 ("leds: add driver for LM3530 ALS")
> Cc: [email protected]
> Signed-off-by: Runyu Xiao <[email protected]>
> ---
> Notes:
>   - Validated with a grounded Lockdep PoC that preserves the
>     lm3530_probe() brightness_set registration and the
>     led_trigger_event_atomic() -> lm3530_brightness_set() ->
>     i2c_smbus_write_byte_data() path.
>   - checkpatch.pl --strict: clean.
>   - Not tested on LM3530 hardware.
> 
>  drivers/leds/leds-lm3530.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/leds-lm3530.c b/drivers/leds/leds-lm3530.c
> index e44a3db106c3..ba744961ebcd 100644
> --- a/drivers/leds/leds-lm3530.c
> +++ b/drivers/leds/leds-lm3530.c
> @@ -301,10 +301,11 @@ static int lm3530_init_registers(struct lm3530_data *drvdata)
>  	return ret;
>  }
>  
> -static void lm3530_brightness_set(struct led_classdev *led_cdev,
> -				     enum led_brightness brt_val)
> +static int
> +lm3530_brightness_set_blocking(struct led_classdev *led_cdev,
> +			       enum led_brightness brt_val)
>  {
> -	int err;
> +	int err = 0;
>  	struct lm3530_data *drvdata =
>  	    container_of(led_cdev, struct lm3530_data, led_dev);
>  	struct lm3530_platform_data *pdata = drvdata->pdata;
> @@ -344,6 +345,8 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev,
>  	default:
>  		break;
>  	}
> +
> +	return err;

What makes you think that the original author didn't want these failures
to be non-catastrophic?

>  }
>  
>  static ssize_t mode_show(struct device *dev,
> @@ -438,7 +441,8 @@ static int lm3530_probe(struct i2c_client *client)
>  	drvdata->brightness = LED_OFF;
>  	drvdata->enable = false;
>  	drvdata->led_dev.name = LM3530_LED_DEV;
> -	drvdata->led_dev.brightness_set = lm3530_brightness_set;
> +	drvdata->led_dev.brightness_set_blocking =
> +		lm3530_brightness_set_blocking;

We've had no complaints about this in the 15-years it's been
operational.  I think it's safe to conclude that this isn't causes
anyone issues.  I'd be concerned that these changes, although seemingly
correct on the surface, may cause problems for users, which would be
unacceptable.

>  	drvdata->led_dev.max_brightness = MAX_BRIGHTNESS;
>  	drvdata->led_dev.groups = lm3530_groups;
>  
> -- 
> 2.34.1

-- 
Lee Jones