overflow and wrong timeout errors in pwm-atmel
Alexandre Belloni
alexandre.belloni at bootlin.com
Wed Apr 21 15:18:37 BST 2021
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? 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.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
More information about the linux-arm-kernel
mailing list