[PATCH] pwm: imx27: fix overflow for bigger periods

[PATCH] pwm: imx27: fix overflow for bigger periods

[Uwe Kleine-König]

The second parameter of do_div is an u32 and NSEC_PER_SEC * prescale
overflows this for bigger periods. Assuming the usual pwm input clk rate
of 66 MHz this happens starting at requested period > 606060 ns.

Splitting the division into two operations doesn't loose any precision.
It doesn't need to be feared that c / NSEC_PER_SEC doesn't fit into the
unsigned long variable "duty_cycles" because in this case the assignment
above to period_cycles would already have been overflowing as
period >= duty_cycle and then the calculation is moot anyhow.

Fixes: aef1a3799b5c ("pwm: imx27: Fix rounding behavior")
Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
Hello,

for a similar patch I said "that looses more precision than I thought at
first", but I think this was wrong. And if it looses precision the same
applies to the calculation of period_cycles which uses the same
operations.

I'm a bit at doubt how urgent this patch is. The regression was
introduced in 5.8-rc1.

Best regards
Uwe

 drivers/pwm/pwm-imx27.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c50d453552bd..86bcafd23e4f 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -235,8 +235,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	period_cycles /= prescale;
 	c = clkrate * state->duty_cycle;
-	do_div(c, NSEC_PER_SEC * prescale);
+	do_div(c, NSEC_PER_SEC);
 	duty_cycles = c;
+	duty_cycles /= prescale;
 
 	/*
 	 * according to imx pwm RM, the real period value should be PERIOD
-- 
2.29.2

Re: [PATCH] pwm: imx27: fix overflow for bigger periods

[Johannes Pointner]

Hello Uwe,

On Mon, Dec 7, 2020 at 3:13 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> The second parameter of do_div is an u32 and NSEC_PER_SEC * prescale
> overflows this for bigger periods. Assuming the usual pwm input clk rate
> of 66 MHz this happens starting at requested period > 606060 ns.
>
> Splitting the division into two operations doesn't loose any precision.
> It doesn't need to be feared that c / NSEC_PER_SEC doesn't fit into the
> unsigned long variable "duty_cycles" because in this case the assignment
> above to period_cycles would already have been overflowing as
> period >= duty_cycle and then the calculation is moot anyhow.
>
> Fixes: aef1a3799b5c ("pwm: imx27: Fix rounding behavior")
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>

This fixes my issue with the pwm-backlight, thanks.

Tested-by: Johannes Pointner <johannes.pointner@br-automation.com>

> ---
> Hello,
>
> for a similar patch I said "that looses more precision than I thought at
> first", but I think this was wrong. And if it looses precision the same
> applies to the calculation of period_cycles which uses the same
> operations.
>
> I'm a bit at doubt how urgent this patch is. The regression was
> introduced in 5.8-rc1.
>
> Best regards
> Uwe
>
>  drivers/pwm/pwm-imx27.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index c50d453552bd..86bcafd23e4f 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -235,8 +235,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>
>         period_cycles /= prescale;
>         c = clkrate * state->duty_cycle;
> -       do_div(c, NSEC_PER_SEC * prescale);
> +       do_div(c, NSEC_PER_SEC);
>         duty_cycles = c;
> +       duty_cycles /= prescale;
>
>         /*
>          * according to imx pwm RM, the real period value should be PERIOD
> --
> 2.29.2
>

Re: [PATCH] pwm: imx27: fix overflow for bigger periods

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

On Mon, Dec 07, 2020 at 03:13:24PM +0100, Uwe Kleine-König wrote:
> The second parameter of do_div is an u32 and NSEC_PER_SEC * prescale
> overflows this for bigger periods. Assuming the usual pwm input clk rate
> of 66 MHz this happens starting at requested period > 606060 ns.
> 
> Splitting the division into two operations doesn't loose any precision.
> It doesn't need to be feared that c / NSEC_PER_SEC doesn't fit into the
> unsigned long variable "duty_cycles" because in this case the assignment
> above to period_cycles would already have been overflowing as
> period >= duty_cycle and then the calculation is moot anyhow.
> 
> Fixes: aef1a3799b5c ("pwm: imx27: Fix rounding behavior")
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello,
> 
> for a similar patch I said "that looses more precision than I thought at
> first", but I think this was wrong. And if it looses precision the same
> applies to the calculation of period_cycles which uses the same
> operations.
> 
> I'm a bit at doubt how urgent this patch is. The regression was
> introduced in 5.8-rc1.

Well, given that this has been broken in v5.8 and v5.9 and was only
reported a couple of days ago, it doesn't seem like this is very urgent.
At the same time, it's a regression, so we should get this fixed as soon
as possible.

That said, I'm reluctant to send this to Linus without a bit of soak
time in linux-next. So let me put it in linux-next for now and if
there's an rc8 I'll send a PR next week. Otherwise it'll get picked up
into an early stable release and that's probably okay as well.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]