Re: [PATCH 14/21] leds: flash: Avoid divide-by-zero in led_clamp_align()

[David Laight]

On Mon, 26 Oct 2026 17:31:29 +0000
Lee Jones <lee@xxxxxxxxxx> wrote:

> On Thu, 22 Oct 2026, David Laight wrote:
> 
> > On Wed, 21 Oct 2026 12:32:23 +0000
> > Lee Jones <lee@xxxxxxxxxx> wrote:
> >   
> > > If a flash LED driver leaves a setting's step size as zero, writing to
> > > the corresponding sysfs attribute invokes led_clamp_align() and triggers
> > > an integer division by zero when aligning the offset to s->step.
> > > 
> > > Clamp s->val to [s->min, s->max] and return early when s->step is zero.
> > > 
> > > Fixes: 7aea8389a77a ("leds: Add LED Flash class extension to the LED subsystem")
> > > Closes: https://sashiko.dev/#/bug/linux-9d06d7fe-310b-4aee-8b16-0fb49550c287
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
> > > ---
> > >  drivers/leds/led-class-flash.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
> > > index 181dfd49c6ad..16b3b6b9365b 100644
> > > --- a/drivers/leds/led-class-flash.c
> > > +++ b/drivers/leds/led-class-flash.c
> > > @@ -375,6 +375,11 @@ static void led_clamp_align(struct led_flash_setting *s)
> > >  {
> > >  	u32 v, offset;
> > >  
> > > +	if (!s->step) {
> > > +		s->val = clamp(s->val, s->min, s->max);
> > > +		return;
> > > +	}  
> > 
> > Why not just:
> > 	u32 step = s->step ?: 1;
> > and then use step instead of s->step.  
> 
> I can see where you're coming from, but I don't think this is better.
> 
> With `step = 1`, the reader has to mentally trace `1 / 2 == 0` and `1 *
> (offset / 1) == offset` to see that it reduces to a plain clamp and the
> CPU still executes a redundant integer division.

That happens with a step of one anyway.
All my suggestion does is treat the unexpected 0 as being the same as 1
instead of optimising for it being zero.

David

> 
> IMHO, simple, easy-to-read, maintainable code wins out over clever code.
> 
> > > +
> > >  	v = s->val + s->step / 2;
> > >  	v = clamp(v, s->min, s->max);
> > >  	offset = v - s->min;  
> >   
>