Message ID | 20201207141324.25945-1-uwe@kleine-koenig.org |
---|---|
State | Accepted |
Headers | show |
Series | pwm: imx27: fix overflow for bigger periods | expand |
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 >
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
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
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(-)