Re: [PATCH v2 3/4] leds: menf21bmc: convert to dev_err_probe()

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

New issues:
- [Low] The commit message description is not capitalized.

Pre-existing issues:
- [Critical] The driver manages LED instances using a shared static global array `leds`, causing kernel list corruption and use-after-free crashes if multiple devices probe or unbind.
- [High] The LED brightness control callback `menf21bmc_led_set` sleeps but is incorrectly assigned to the non-blocking `brightness_set` hook.
--

--- Patch [3]: [PATCH v2 3/4] leds: menf21bmc: convert to dev_err_probe() ---
commit 256232e59da7fa918a47e5b2a0fabb6d2df03b37
Author: Arunachalam <[email protected]>

leds: menf21bmc: convert to dev_err_probe()
    
Replace open-coded dev_err() + return pattern with dev_err_probe(),
which also handles -EPROBE_DEFER correctly.

> leds: menf21bmc: convert to dev_err_probe()

[Severity: Low]
This isn't a bug introduced by this patch, but subsystem guidelines mandate
that the description after the subsystem prefix must be capitalized for LED
drivers. Should this be "Convert to dev_err_probe()"?

> diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
> index 6b1b471606026..a7e4db4879d81 100644
> --- a/drivers/leds/leds-menf21bmc.c
> +++ b/drivers/leds/leds-menf21bmc.c
> @@ -84,10 +84,8 @@ static int menf21bmc_led_probe(struct platform_device *pdev)
>  		leds[i].cdev.brightness_set = menf21bmc_led_set;

[Severity: High]
This is a pre-existing issue, but does assigning menf21bmc_led_set to the
non-blocking brightness_set hook risk a scheduling while atomic panic?

If the LED is set to blink via sysfs, led_timer_function executes in atomic
softirq context and calls this brightness_set callback. However,
menf21bmc_led_set() takes a mutex and performs I2C transactions which can
sleep.

Should this be assigned to brightness_set_blocking instead?

>  		leds[i].i2c_client = i2c_client;
>  		ret = devm_led_classdev_register(&pdev->dev, &leds[i].cdev);

[Severity: Critical]
This is a pre-existing issue, but does registering a globally shared static
cdev object risk linked list corruption?

The driver manages LED instances using a single static global array (leds).
If multiple devices are probed or if a user unbinds and rebinds the device,
devm_led_classdev_register() will be called on the same leds[i].cdev
structure.

The LED core calls list_add_tail() on a node already in a linked list, which
corrupts the global leds_list.

> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "failed to register LED device\n");
> -			return ret;
> -		}
> +		if (ret < 0)
> +			return dev_err_probe(&pdev->dev, ret, "failed to register LED device\n");
>  	}
>  	dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");

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