Re: [PATCH] leds: bcm63138: Use %pe to print pinctrl error instead of %ld

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

Pre-existing issues:
- [High] The driver ignores `-EPROBE_DEFER` from `devm_pinctrl_get_select_default()`, failing to properly defer probe when the pinctrl driver is not yet ready.
- [High] Hardware initialization occurs without holding the required spinlock after the LED is exposed to user space, causing a data race on shared hardware registers.
--

--- Patch [1]: [PATCH] leds: bcm63138: Use %pe to print pinctrl error instead of %ld ---
commit 11674fcbe14b4d4811e6f7f83c90fb13ea7bf5ac
Author: Ninad Naik <[email protected]>

leds: bcm63138: Use %pe to print pinctrl error instead of %ld

Pass the pinctrl error pointer directly to %pe to print the
symbolic error name. Detected by Coccinelle. Tested by compiling.

> diff --git a/drivers/leds/blink/leds-bcm63138.c b/drivers/leds/blink/leds-bcm63138.c
> index 2cf2761e49145..78a87e1df4d7c 100644
> --- a/drivers/leds/blink/leds-bcm63138.c
> +++ b/drivers/leds/blink/leds-bcm63138.c
> @@ -237,8 +237,8 @@ static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
>  
>  	pinctrl = devm_pinctrl_get_select_default(led->cdev.dev);
>  	if (IS_ERR(pinctrl) && PTR_ERR(pinctrl) != -ENODEV) {
> -		dev_warn(led->cdev.dev, "Failed to select %pOF pinctrl: %ld\n",
> -			 np, PTR_ERR(pinctrl));
> +		dev_warn(led->cdev.dev, "Failed to select %pOF pinctrl: %pe\n",
> +			 np, pinctrl);
>  	}

[Severity: High]
This is a pre-existing issue, but does this code ignore -EPROBE_DEFER from
devm_pinctrl_get_select_default()?

If the pinctrl provider is probed after this LED driver, it would return
-EPROBE_DEFER. Because the driver merely logs a warning and continues instead
of returning the error (e.g. via dev_err_probe), the LED device registers
successfully but remains physically non-functional since its pins are never
multiplexed.

>  
>  	bit = BIT(led->pin);
>  	bcm63138_leds_update_bits(leds, BCM63138_PARALLEL_LED_POLARITY, bit,
>  				  led->active_low ? 0 : bit);
>  	bcm63138_leds_update_bits(leds, BCM63138_HW_LED_EN, bit, 0);
>  	bcm63138_leds_set_flash_rate(leds, led, 0);
>  	bcm63138_leds_enable_led(leds, led, led->cdev.brightness);

[Severity: High]
This is also a pre-existing issue, but is it safe to initialize the hardware
without holding the required spinlock after the LED is exposed to user space?

bcm63138_leds_update_bits() performs a non-atomic read-modify-write on shared
hardware registers. Calling it locklessly here can race with
bcm63138_leds_brightness_set() from another CPU, which locks leds->lock and
performs its own read-modify-write on the same registers, corrupting the
hardware state of previously registered LEDs.

User space can concurrently write to sysfs attributes of this or other LEDs
during this lockless initialization sequence because
devm_led_classdev_register_ext() was already called earlier in this function.

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