overflow and wrong timeout errors in pwm-atmel

[Uwe Kleine-König]

Hello Alexandre,

On Wed, Apr 21, 2021 at 04:18:37PM +0200, Alexandre Belloni wrote:
> On 21/04/2021 15:48:25+0200, Uwe Kleine-König wrote:
> > On Wed, Apr 21, 2021 at 01:03:36PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> > > > With these three patches PWM_DEBUG is now happy. (At least I couldn't
> > > > trigger a warning any more. I think there are still a few problems with
> > > > integer overflows.)
> > > 
> > > BTW, setting the period to 138350580899 (with a clock rate of 133333333
> > > Hz) results in setting period=0 because
> > > 
> > > 	state->period * clkrate =
> > > 	138350580899 * 133333333 =
> > > 	40254751 (discarded from 18446744073749806367).
> > 
> > As a first remedy the following could be done:
> > 
> > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> > index 38d86340201c..02d69fa5f7d2 100644
> > --- a/drivers/pwm/pwm-atmel.c
> > +++ b/drivers/pwm/pwm-atmel.c
> > @@ -199,6 +199,11 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
> >  	unsigned long long cycles = state->period;
> >  	int shift;
> >  
> > +	if (fls(cycles) + fls(clkrate) > 64) {
> > +		dev_err(chip->dev, "period to big to calculate HW parameters\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* Calculate the period cycles and prescale value */
> >  	cycles *= clkrate;
> >  	do_div(cycles, NSEC_PER_SEC);
> > 
> > Is this sensible? (Actually I'd prefer to just continue with
> > 
> > 	period = (ULL(1) << (64 - fls(clkrate))) - 1
> > 
> > according to the motto to yield the highest possible period, but this
> > function has another error path that returns -EINVAL so this would be
> > inconsistent.)
> 
> Shouldn't that be -ERANGE?

The other exit point a few lines down also uses -EINVAL so I sticked to
that.

> I do think it is better to return an error and let userspace decide
> what is the policy instead of having the policy in the driver.

I agree that the consumer (userspace is just one of them) should have
the choice what happens. IMHO the only sane way to implement this is
using a round_state function that tells the consumer what they can
expect when a certain state is passed to apply. (Otherwise the consumer
might already be unhappy if they request a period of say 652799 ns and
the driver implements 645120 ns (which is BTW what happens with the
pwm-atmel driver when feed by a 133333333 Hz clk).)

And another critical detail of this round_state function is that it
should expose the same behaviour for all lowlevel drivers. I think
first rounding down period and then duty_cycle is a good strategy that
can be worked with. With that in place the next (IMHO) obvious
conclusion is that .apply() should behave in the same way for
consistency and also to keep the drivers simple.

If then a consumer prefers a different rounding strategy (e.g. for the
pwm-ir-tx driver rounding to the nearest values is better), this can be
(more or less) easily and (more important) generically implemented in a
single place.

Does this make sense in your eyes?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20210421/ac92d339/attachment.sig>