Re: [PATCH v6 9/9] leds: st1202: correct and extend hw_pattern documentation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver violates the LED subsystem API contract by registering a sleeping function (`st1202_brightness_set`) as the non-blocking `brightness_set` callback. This function acquires a mutex and calls I2C functions, which will cause a 'scheduling while atomic' kernel panic when called from atomic context.
--

commit dab687d256f9aaff49be14575dcf7a85acc3dd4a
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: correct and extend hw_pattern documentation
    
Fix the repeat section: -1 is a valid value meaning infinite repeat,
as accepted by the ledtrig-pattern sysfs interface; only 0 and values
below -1 are rejected. The previous text incorrectly stated all negative
numbers were invalid. Also remove the redundant trailing sentence since
the behaviour is now covered inline.
    
Add the brightness range (0-255) to the hw_pattern section, which was
previously undocumented.

> diff --git a/Documentation/leds/leds-st1202.rst b/Documentation/leds/leds-st1202.rst
> index a2353549469ee..ed32eb3a27d45 100644
> --- a/Documentation/leds/leds-st1202.rst
> +++ b/Documentation/leds/leds-st1202.rst

[ ... ]

While this patch is a documentation update, there is a pre-existing issue in the
corresponding C file:

drivers/leds/leds-st1202.c:

> static void st1202_brightness_set(struct led_classdev *led_cdev,
> 				enum led_brightness value)
> {
> 	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
> 	struct st1202_chip *chip = led->chip;
> 
> 	guard(mutex)(&chip->lock);
> 
> 	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
> 		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
> 	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
> 	__st1202_channel_set(chip, led->led_num, !!value);
> }

[Severity: High]
This isn't a bug introduced by this patch, but does the driver violate the LED
subsystem API contract by registering a sleeping function
(st1202_brightness_set) as the non-blocking brightness_set callback?

This function acquires a mutex and calls I2C functions, which will cause a
'scheduling while atomic' kernel panic when called from atomic context.

Could this crash the kernel if a user configures an LED trigger like
ledtrig-timer or ledtrig-pattern that runs in softirq (atomic) context?

The trigger's timer callback calls led_set_brightness(), which is guaranteed
not to sleep, and synchronously invokes the driver's brightness_set callback.
Because st1202_brightness_set uses guard(mutex) and performs I2C transactions,
it sleeps in an atomic context, triggering a crash. Any user with sysfs access
to configure LED triggers can trigger this path.

[ ... ]

> static int st1202_dt_init(struct st1202_chip *chip)
> {
[ ... ]
> 		led->led_cdev.max_brightness = U8_MAX;
> 		led->led_cdev.brightness_set_blocking = st1202_led_set;
> 		led->led_cdev.pattern_set = st1202_led_pattern_set;
> 		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
> 		led->led_cdev.default_trigger = "pattern";
> 		led->led_cdev.brightness_set = st1202_brightness_set;
> 		led->led_cdev.brightness_get = st1202_brightness_get;
> 	}

[Severity: High]
This is a pre-existing issue, but by assigning st1202_brightness_set to the
non-blocking callback pointer, we risk atomic sleep panics.

Should this be assigned to brightness_set_blocking instead, and brightness_set
set to NULL so the LED core can correctly defer blocking operations to a
workqueue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB84972A16ED557BFB96D63F57C5F52@GV1PR08MB8497.eurprd08.prod.outlook.com?part=9