| Message ID | 20220420121240.67781-7-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pwm: renesas-tpu: Various improvements | expand |
On Wed, Apr 20, 2022 at 2:12 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Dividing by the result of a division looses precision. Consider for example > clk_rate = 33000000 and period_ns = 500001. Then > > clk_rate / (NSEC_PER_SEC / period_ns) > > has the exact value 16500.033, but in C this evaluates to 16508. It gets > worse for even bigger values of period_ns, so with period_ns = 500000001, > the exact result is 16500000.033 while in C we get 33000000. > > For that reason use > > clk_rate * period_ns / NSEC_PER_SEC > > instead which doesn't suffer from this problem. To ensure this doesn't > overflow add a safeguard check for clk_rate. > > Note that duty > period can never happen, so the respective check can be > dropped. > > Incidentally this fixes a division by zero if period_ns > NSEC_PER_SEC. > Another side effect is that values bigger than INT_MAX for period and > duty_cyle are not wrongly discarded any more. > > Fixes: 99b82abb0a35 ("pwm: Add Renesas TPU PWM driver") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index 60ba7cf275c7..9e6978c39788 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -242,20 +242,29 @@ static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) } static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, - int duty_ns, int period_ns, bool enabled) + u64 duty_ns, u64 period_ns, bool enabled) { struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm); struct tpu_device *tpu = to_tpu_device(chip); unsigned int prescaler; bool duty_only = false; u32 clk_rate; - u32 period; + u64 period; u32 duty; int ret; clk_rate = clk_get_rate(tpu->clk); + if (unlikely(clk_rate > NSEC_PER_SEC)) { + /* + * This won't happen in the nearer future, so this is only a + * safeguard to prevent the following calculation from + * overflowing. With this clk_rate * period_ns / NSEC_PER_SEC is + * not greater than period_ns and so fits into an u64. + */ + return -EINVAL; + } - period = clk_rate / (NSEC_PER_SEC / period_ns); + period = mul_u64_u64_div_u64(clk_rate, period_ns, NSEC_PER_SEC); /* * Find the minimal prescaler in [0..3] such that @@ -292,18 +301,15 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, period >>= 2 * prescaler; - if (duty_ns) { - duty = (clk_rate >> 2 * prescaler) - / (NSEC_PER_SEC / duty_ns); - if (duty > period) - return -EINVAL; - } else { + if (duty_ns) + duty = mul_u64_u64_div_u64(clk_rate, duty_ns, + (u64)NSEC_PER_SEC << (2 * prescaler)); + else duty = 0; - } dev_dbg(&tpu->pdev->dev, "rate %u, prescaler %u, period %u, duty %u\n", - clk_rate, 1 << (2 * prescaler), period, duty); + clk_rate, 1 << (2 * prescaler), (u32)period, duty); if (tpd->prescaler == prescaler && tpd->period == period) duty_only = true;
Dividing by the result of a division looses precision. Consider for example clk_rate = 33000000 and period_ns = 500001. Then clk_rate / (NSEC_PER_SEC / period_ns) has the exact value 16500.033, but in C this evaluates to 16508. It gets worse for even bigger values of period_ns, so with period_ns = 500000001, the exact result is 16500000.033 while in C we get 33000000. For that reason use clk_rate * period_ns / NSEC_PER_SEC instead which doesn't suffer from this problem. To ensure this doesn't overflow add a safeguard check for clk_rate. Note that duty > period can never happen, so the respective check can be dropped. Incidentally this fixes a division by zero if period_ns > NSEC_PER_SEC. Another side effect is that values bigger than INT_MAX for period and duty_cyle are not wrongly discarded any more. Fixes: 99b82abb0a35 ("pwm: Add Renesas TPU PWM driver") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-renesas-tpu.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)