Re: [PATCH] leds: multicolor: limit intensity to max_brightness of LED

[Michael Tretter <m.tretter@pengutronix.de>]

On Fri, 06 Mar 2026 13:55:05 +0000, Lee Jones wrote:
> On Thu, 05 Mar 2026, Andy Shevchenko wrote:
> 
> > On Fri, Jan 23, 2026 at 11:13:24AM +0100, Michael Tretter wrote:
> > > According to Documentation/ABI/testing/sysfs-class-led-multicolor, the
> > > intensity should not exceed /sys/class/leds/<led>/max_brightness.
> > > 
> > > The interface doesn't check the values and higher values may lead to
> > > unexpected color changes if the brightness is changed.
> > > 
> > > Clamp the intensity value to max_brightness.
> > 
> > This also brings a regression if somebody doesn't care about wrapping around.
> > It's possible to return an error instead, but still the user space will be
> > broken (in some rare weird cases).

If somebody in user space doesn't care about the wrap around for the
brightness value, the behavior will still differ between systems,
because the value for the wraparound for brightness depends on
max_brightness, which is driver specific and some drivers already
implement clamping on max_brightness.

Handling this in a coherent way at the API level leads to more
consistent behavior between different drivers.

Furthermore, intensity for multicolor LEDs behaves kind of like
brightness for single color LEDs and for these, the documentation
(Documentation/ABI/testing/sysfs-class-led) specifies that

	The [brightness] value is between 0 and
	/sys/class/leds/<led>/max_brightness

and

	Most LEDs don't have hardware brightness support, so will
	just be turned on for non-zero brightness settings.

With a wraparound behavior, writing (max_brightness + 1) to brightness
would lead to a wraparound and turn the LED off, which is against this
specification.

I agree that the behavior of intensity is undefined if it exceeds
max_brightness. Do you have an example of user space (or use case) that
relies on this behavior or is this a hypothetical case?

> > 
> > Again, I care even less about this change, but be always careful,
> > the main rule in the kernel "We do NOT break user space".
> 
> I'm going to remove this patch for now.
> 
> /tmp/next/build/drivers/leds/led-class-multicolor.c: In function 'multi_intensity_store':
> /tmp/next/build/include/linux/compiler_types.h:706:45: error: call to '__compiletime_assert_434' declared with attribute error: min(intensity_value[i], led_cdev->max_brightness) signedness error
>   706 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |                                             ^
> /tmp/next/build/include/linux/compiler_types.h:687:25: note: in definition of macro '__compiletime_assert'
>   687 |                         prefix ## suffix();                             \
>       |                         ^~~~~~
> /tmp/next/build/include/linux/compiler_types.h:706:9: note: in expansion of macro '_compiletime_assert'
>   706 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |         ^~~~~~~~~~~~~~~~~~~
> /tmp/next/build/include/linux/build_bug.h:40:37: note: in expansion of macro 'compiletime_assert'
>    40 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>       |                                     ^~~~~~~~~~~~~~~~~~
> /tmp/next/build/include/linux/minmax.h:93:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>    93 |         BUILD_BUG_ON_MSG(!__types_ok(ux, uy),           \
>       |         ^~~~~~~~~~~~~~~~
> /tmp/next/build/include/linux/minmax.h:98:9: note: in expansion of macro '__careful_cmp_once'
>    98 |         __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>       |         ^~~~~~~~~~~~~~~~~~
> /tmp/next/build/include/linux/minmax.h:105:25: note: in expansion of macro '__careful_cmp'
>   105 | #define min(x, y)       __careful_cmp(min, x, y)
>       |                         ^~~~~~~~~~~~~
> /tmp/next/build/drivers/leds/led-class-multicolor.c:51:38: note: in expansion of macro 'min'
>    51 |                 intensity_value[i] = min(intensity_value[i],
>       |               

I'm confused because you already applied [0] my patch that fixes this
exact error.

[0] https://lore.kernel.org/all/177273014112.321702.15159156714467402540.b4-ty@kernel.org/

Michael