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

[Lee Jones <lee@kernel.org>]

Andreas, any thoughts?

On Mon, 15 Jun 2026, Runyu Xiao wrote:

> menf21bmc_led_set() serializes access with a mutex and performs I2C
> transfers, but the driver exposes it through brightness_set.  That
> mismatches the LED core callback contract for atomic callers.
> 
> 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: menf21bmc_led_probe()
> still publishes menf21bmc_led_set() as brightness_set,
> led_trigger_event_atomic() invokes it under spin_lock_irqsave(), and
> the callback immediately tries mutex_lock(&led_lock) before reaching
> its I2C accesses.
> 
> Lockdep reports:
> 
>   BUG: sleeping function called from invalid context
>   __mutex_lock+0x4f/0xd20
>   menf21bmc_led_set+0x15/0x49 [vuln_msv]
>   [ BUG: Invalid wait context ]
>   ... (led_lock#2) ... at: menf21bmc_led_set+0x15/0x49 [vuln_msv]
> 
> Convert the driver to brightness_set_blocking and return the transport
> status to the LED core.
> 
> Fixes: 38433639af91 ("leds: leds-menf21bmc: Introduce MEN 14F021P00 BMC LED driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
> Notes:
>   - Validated with a grounded Lockdep PoC that preserves the
>     menf21bmc_led_probe() brightness_set registration and the
>     led_trigger_event_atomic() -> menf21bmc_led_set() ->
>     mutex_lock(&led_lock) path.
>   - checkpatch.pl --strict: clean.
>   - Not tested on MEN 14F021P00 BMC hardware.
> 
>  drivers/leds/leds-menf21bmc.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
> index 6b1b47160602..da476fe6ed2c 100644
> --- a/drivers/leds/leds-menf21bmc.c
> +++ b/drivers/leds/leds-menf21bmc.c
> @@ -49,28 +49,33 @@ static struct menf21bmc_led leds[] = {
>  
>  static DEFINE_MUTEX(led_lock);
>  
> -static void
> -menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +static int
> +menf21bmc_led_set_blocking(struct led_classdev *led_cdev,
> +			   enum led_brightness value)
>  {
>  	int led_val;
> +	int ret = 0;
>  	struct menf21bmc_led *led = container_of(led_cdev,
>  					struct menf21bmc_led, cdev);
>  
>  	mutex_lock(&led_lock);
>  	led_val = i2c_smbus_read_byte_data(led->i2c_client,
>  					   BMC_CMD_LED_GET_SET);
> -	if (led_val < 0)
> +	if (led_val < 0) {
> +		ret = led_val;
>  		goto err_out;
> +	}
>  
>  	if (value == LED_OFF)
>  		led_val &= ~led->led_bit;
>  	else
>  		led_val |= led->led_bit;
>  
> -	i2c_smbus_write_byte_data(led->i2c_client,
> -				  BMC_CMD_LED_GET_SET, led_val);
> +	ret = i2c_smbus_write_byte_data(led->i2c_client,
> +					BMC_CMD_LED_GET_SET, led_val);
>  err_out:
>  	mutex_unlock(&led_lock);
> +	return ret;
>  }
>  
>  static int menf21bmc_led_probe(struct platform_device *pdev)
> @@ -81,7 +86,8 @@ static int menf21bmc_led_probe(struct platform_device *pdev)
>  
>  	for (i = 0; i < ARRAY_SIZE(leds); i++) {
>  		leds[i].cdev.name = leds[i].name;
> -		leds[i].cdev.brightness_set = menf21bmc_led_set;
> +		leds[i].cdev.brightness_set_blocking =
> +			menf21bmc_led_set_blocking;
>  		leds[i].i2c_client = i2c_client;
>  		ret = devm_led_classdev_register(&pdev->dev, &leds[i].cdev);
>  		if (ret < 0) {
> -- 
> 2.34.1

-- 
Lee Jones