* [PATCH] pwm: iqs620a: Fix overflow and optimize calculations
@ 2020-11-27 14:14 Uwe Kleine-König
2020-11-27 20:10 ` Jeff LaBundy
0 siblings, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2020-11-27 14:14 UTC (permalink / raw)
To: Jeff LaBundy, Thierry Reding, Lee Jones; +Cc: linux-pwm, kernel
If state->duty_cycle is 0x100000000000000, the previous calculation of
duty_scale overflows and yields a duty cycle ratio of 0% instead of
100%. Fix this by comparing the requested duty cycle against the maximal
possible duty cycle first. This way it is possible to use a native
integer division instead of a (depending on the architecture) more
expensive 64bit division. Also duty_val cannot be bigger than 0xff which
allows to simplify the code a bit further down.
Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,
even though this is a fix, I don't consider it critical enough to apply
it before v5.10.
Best regards
Uwe
drivers/pwm/pwm-iqs620a.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
index 7d33e3646436..0c9e2a876a05 100644
--- a/drivers/pwm/pwm-iqs620a.c
+++ b/drivers/pwm/pwm-iqs620a.c
@@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct iqs620_pwm_private *iqs620_pwm;
struct iqs62x_core *iqs62x;
- u64 duty_scale;
+ u8 duty_val;
int ret;
if (state->polarity != PWM_POLARITY_NORMAL)
@@ -70,20 +70,22 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* For lower duty cycles (e.g. 0), the PWM output is simply disabled to
* allow an external pull-down resistor to hold the GPIO3/LTX pin low.
*/
- duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
+
+ if (state->duty_cycle < IQS620_PWM_PERIOD_NS)
+ duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS;
+ else
+ duty_val = 0xff;
mutex_lock(&iqs620_pwm->lock);
- if (!state->enabled || !duty_scale) {
+ if (!state->enabled || !duty_val) {
ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
IQS620_PWR_SETTINGS_PWM_OUT, 0);
if (ret)
goto err_mutex;
}
- if (duty_scale) {
- u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
-
+ if (duty_val) {
ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
duty_val);
if (ret)
@@ -92,7 +94,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
iqs620_pwm->duty_val = duty_val;
}
- if (state->enabled && duty_scale) {
+ if (state->enabled && duty_val) {
ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
if (ret)
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] pwm: iqs620a: Fix overflow and optimize calculations
2020-11-27 14:14 [PATCH] pwm: iqs620a: Fix overflow and optimize calculations Uwe Kleine-König
@ 2020-11-27 20:10 ` Jeff LaBundy
2020-11-27 20:32 ` [PATCH v2] " Uwe Kleine-König
0 siblings, 1 reply; 6+ messages in thread
From: Jeff LaBundy @ 2020-11-27 20:10 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Thierry Reding, Lee Jones, linux-pwm, kernel
Hi Uwe,
I tested this patch on actual hardware but the newly calculated register
values are incorrect. We used to get:
duty_cycle reg. 0xd2 bit 7 reg. 0xd8 pulse width
---------- --------------- --------- ----------------
1000 0 N/A constant off
4000 1 0x00 3900 us
8000 1 0x01 7800 us
1000000 1 0xff constant on
Now we get:
duty_cycle reg. 0xd2 bit 7 reg. 0xd8 pulse width
---------- --------------- --------- ----------------
1000 0 N/A constant off
4000 1 0x01 7800 us (x)
8000 1 0x02 11800 us (x)
1000000 1 0xff constant on
A comment below.
On Fri, Nov 27, 2020 at 03:14:04PM +0100, Uwe Kleine-König wrote:
> If state->duty_cycle is 0x100000000000000, the previous calculation of
> duty_scale overflows and yields a duty cycle ratio of 0% instead of
> 100%. Fix this by comparing the requested duty cycle against the maximal
> possible duty cycle first. This way it is possible to use a native
> integer division instead of a (depending on the architecture) more
> expensive 64bit division. Also duty_val cannot be bigger than 0xff which
> allows to simplify the code a bit further down.
>
> Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> even though this is a fix, I don't consider it critical enough to apply
> it before v5.10.
>
> Best regards
> Uwe
>
> drivers/pwm/pwm-iqs620a.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> index 7d33e3646436..0c9e2a876a05 100644
> --- a/drivers/pwm/pwm-iqs620a.c
> +++ b/drivers/pwm/pwm-iqs620a.c
> @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct iqs620_pwm_private *iqs620_pwm;
> struct iqs62x_core *iqs62x;
> - u64 duty_scale;
> + u8 duty_val;
> int ret;
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> @@ -70,20 +70,22 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
> * allow an external pull-down resistor to hold the GPIO3/LTX pin low.
> */
> - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
> +
> + if (state->duty_cycle < IQS620_PWM_PERIOD_NS)
> + duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS;
> + else
> + duty_val = 0xff;
>
> mutex_lock(&iqs620_pwm->lock);
>
> - if (!state->enabled || !duty_scale) {
> + if (!state->enabled || !duty_val) {
> ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> IQS620_PWR_SETTINGS_PWM_OUT, 0);
> if (ret)
> goto err_mutex;
> }
>
> - if (duty_scale) {
> - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> -
> + if (duty_val) {
This is part of the problem; the device's formula for duty cycle has a
plus one that is getting dropped now (see comments in iqs620_pwm_apply).
But since you're clamping duty_val to 0xff ahead of the zero check now,
simply writing duty_val - 1 instead of duty_val would prevent you from
getting constant-on.
> ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
> duty_val);
> if (ret)
> @@ -92,7 +94,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> iqs620_pwm->duty_val = duty_val;
> }
>
> - if (state->enabled && duty_scale) {
> + if (state->enabled && duty_val) {
> ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
> if (ret)
> --
> 2.29.2
>
Kind regards,
Jeff LaBundy
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] pwm: iqs620a: Fix overflow and optimize calculations
2020-11-27 20:10 ` Jeff LaBundy
@ 2020-11-27 20:32 ` Uwe Kleine-König
2020-11-28 1:00 ` Jeff LaBundy
0 siblings, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2020-11-27 20:32 UTC (permalink / raw)
To: Jeff LaBundy, Thierry Reding
Cc: linux-pwm, Lee Jones, kernel, Uwe Kleine-König
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
If state->duty_cycle is 0x100000000000000, the previous calculation of
duty_scale overflows and yields a duty cycle ratio of 0% instead of
100%. Fix this by comparing the requested duty cycle against the maximal
possible duty cycle first. This way it is possible to use a native
integer division instead of a (depending on the architecture) more
expensive 64bit division. Also duty_val cannot be bigger than 0xff which
allows to simplify the code a bit further down.
Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello Jeff,
On Fri, Nov 27, 2020 at 02:10:51PM -0600, Jeff LaBundy wrote:
> I tested this patch on actual hardware but the newly calculated register
> values are incorrect. We used to get:
>
> [...]
> > goto err_mutex;
> > }
> >
> > - if (duty_scale) {
> > - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> > -
> > + if (duty_val) {
>
> This is part of the problem; the device's formula for duty cycle has a
> plus one that is getting dropped now (see comments in iqs620_pwm_apply).
Good catch, I indeed missed that - 1.
This patch should be better in this regard.
Thanks for particular attention and testing,
Uwe
drivers/pwm/pwm-iqs620a.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
index 7d33e3646436..6789e117f123 100644
--- a/drivers/pwm/pwm-iqs620a.c
+++ b/drivers/pwm/pwm-iqs620a.c
@@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct iqs620_pwm_private *iqs620_pwm;
struct iqs62x_core *iqs62x;
- u64 duty_scale;
+ u8 duty_val;
int ret;
if (state->polarity != PWM_POLARITY_NORMAL)
@@ -70,29 +70,31 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* For lower duty cycles (e.g. 0), the PWM output is simply disabled to
* allow an external pull-down resistor to hold the GPIO3/LTX pin low.
*/
- duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
+
+ if (state->duty_cycle < IQS620_PWM_PERIOD_NS)
+ duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS;
+ else
+ duty_val = 256;
mutex_lock(&iqs620_pwm->lock);
- if (!state->enabled || !duty_scale) {
+ if (!state->enabled || !duty_val) {
ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
IQS620_PWR_SETTINGS_PWM_OUT, 0);
if (ret)
goto err_mutex;
}
- if (duty_scale) {
- u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
-
+ if (duty_val) {
ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
- duty_val);
+ duty_val - 1);
if (ret)
goto err_mutex;
iqs620_pwm->duty_val = duty_val;
}
- if (state->enabled && duty_scale) {
+ if (state->enabled && duty_val) {
ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
if (ret)
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pwm: iqs620a: Fix overflow and optimize calculations
2020-11-27 20:32 ` [PATCH v2] " Uwe Kleine-König
@ 2020-11-28 1:00 ` Jeff LaBundy
2020-11-29 12:03 ` Uwe Kleine-König
0 siblings, 1 reply; 6+ messages in thread
From: Jeff LaBundy @ 2020-11-28 1:00 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Thierry Reding, linux-pwm, Lee Jones, kernel, Uwe Kleine-König
Hi Uwe,
Thank you for your work on this; a few comments below.
On Fri, Nov 27, 2020 at 09:32:44PM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> If state->duty_cycle is 0x100000000000000, the previous calculation of
> duty_scale overflows and yields a duty cycle ratio of 0% instead of
> 100%. Fix this by comparing the requested duty cycle against the maximal
> possible duty cycle first. This way it is possible to use a native
> integer division instead of a (depending on the architecture) more
> expensive 64bit division. Also duty_val cannot be bigger than 0xff which
> allows to simplify the code a bit further down.
>
> Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello Jeff,
>
> On Fri, Nov 27, 2020 at 02:10:51PM -0600, Jeff LaBundy wrote:
> > I tested this patch on actual hardware but the newly calculated register
> > values are incorrect. We used to get:
> >
> > [...]
> > > goto err_mutex;
> > > }
> > >
> > > - if (duty_scale) {
> > > - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> > > -
> > > + if (duty_val) {
> >
> > This is part of the problem; the device's formula for duty cycle has a
> > plus one that is getting dropped now (see comments in iqs620_pwm_apply).
>
> Good catch, I indeed missed that - 1.
>
> This patch should be better in this regard.
>
> Thanks for particular attention and testing,
> Uwe
>
> drivers/pwm/pwm-iqs620a.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> index 7d33e3646436..6789e117f123 100644
> --- a/drivers/pwm/pwm-iqs620a.c
> +++ b/drivers/pwm/pwm-iqs620a.c
> @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct iqs620_pwm_private *iqs620_pwm;
> struct iqs62x_core *iqs62x;
> - u64 duty_scale;
> + u8 duty_val;
> int ret;
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> @@ -70,29 +70,31 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
> * allow an external pull-down resistor to hold the GPIO3/LTX pin low.
> */
> - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
> +
> + if (state->duty_cycle < IQS620_PWM_PERIOD_NS)
> + duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS;
> + else
> + duty_val = 256;
The build gives a warning here since duty_val is a u8. Actually, I'm not
a fan of calling this 'duty_val' since it has a different meaning than
iqs620_pwm->duty_val (the cached version restored if the watchdog bites).
>
> mutex_lock(&iqs620_pwm->lock);
>
> - if (!state->enabled || !duty_scale) {
> + if (!state->enabled || !duty_val) {
> ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> IQS620_PWR_SETTINGS_PWM_OUT, 0);
> if (ret)
> goto err_mutex;
> }
>
> - if (duty_scale) {
> - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> -
> + if (duty_val) {
> ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
> - duty_val);
> + duty_val - 1);
> if (ret)
> goto err_mutex;
>
> iqs620_pwm->duty_val = duty_val;
> }
This would need to change to iqs620_pwm->duty_val = duty_val - 1, otherwise
the wrong duty cycle will get restored in iqs620_pwm_notifier. However this
starts to look confusing.
>
> - if (state->enabled && duty_scale) {
> + if (state->enabled && duty_val) {
> ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
> if (ret)
> --
> 2.29.2
>
How about the patch below instead? It solves the overflow problem you found,
is minimally invasive and preserves the original intent. We still avoid the
64-bit division which is unnecessary anyway given this device's fixed period
of only 1 ms.
Kind regards,
Jeff LaBundy
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
index 7d33e36..a15a2aa 100644
--- a/drivers/pwm/pwm-iqs620a.c
+++ b/drivers/pwm/pwm-iqs620a.c
@@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct iqs620_pwm_private *iqs620_pwm;
struct iqs62x_core *iqs62x;
- u64 duty_scale;
+ unsigned int duty_scale;
int ret;
if (state->polarity != PWM_POLARITY_NORMAL)
@@ -70,7 +70,9 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* For lower duty cycles (e.g. 0), the PWM output is simply disabled to
* allow an external pull-down resistor to hold the GPIO3/LTX pin low.
*/
- duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
+ duty_scale = (unsigned int)min_t(u64, state->duty_cycle,
+ IQS620_PWM_PERIOD_NS) * 256 /
+ IQS620_PWM_PERIOD_NS;
mutex_lock(&iqs620_pwm->lock);
@@ -82,7 +84,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
}
if (duty_scale) {
- u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
+ u8 duty_val = min_t(unsigned int, duty_scale - 1, 0xff);
ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
duty_val);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pwm: iqs620a: Fix overflow and optimize calculations
2020-11-28 1:00 ` Jeff LaBundy
@ 2020-11-29 12:03 ` Uwe Kleine-König
2020-11-29 21:15 ` Jeff LaBundy
0 siblings, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2020-11-29 12:03 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Uwe Kleine-König, linux-pwm, Thierry Reding, Lee Jones, kernel
[-- Attachment #1: Type: text/plain, Size: 6214 bytes --]
On Fri, Nov 27, 2020 at 07:00:40PM -0600, Jeff LaBundy wrote:
> Hi Uwe,
>
> Thank you for your work on this; a few comments below.
>
> On Fri, Nov 27, 2020 at 09:32:44PM +0100, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >
> > If state->duty_cycle is 0x100000000000000, the previous calculation of
> > duty_scale overflows and yields a duty cycle ratio of 0% instead of
> > 100%. Fix this by comparing the requested duty cycle against the maximal
> > possible duty cycle first. This way it is possible to use a native
> > integer division instead of a (depending on the architecture) more
> > expensive 64bit division. Also duty_val cannot be bigger than 0xff which
> > allows to simplify the code a bit further down.
> >
> > Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello Jeff,
> >
> > On Fri, Nov 27, 2020 at 02:10:51PM -0600, Jeff LaBundy wrote:
> > > I tested this patch on actual hardware but the newly calculated register
> > > values are incorrect. We used to get:
> > >
> > > [...]
> > > > goto err_mutex;
> > > > }
> > > >
> > > > - if (duty_scale) {
> > > > - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> > > > -
> > > > + if (duty_val) {
> > >
> > > This is part of the problem; the device's formula for duty cycle has a
> > > plus one that is getting dropped now (see comments in iqs620_pwm_apply).
> >
> > Good catch, I indeed missed that - 1.
> >
> > This patch should be better in this regard.
> >
> > Thanks for particular attention and testing,
> > Uwe
> >
> > drivers/pwm/pwm-iqs620a.c | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> > index 7d33e3646436..6789e117f123 100644
> > --- a/drivers/pwm/pwm-iqs620a.c
> > +++ b/drivers/pwm/pwm-iqs620a.c
> > @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > {
> > struct iqs620_pwm_private *iqs620_pwm;
> > struct iqs62x_core *iqs62x;
> > - u64 duty_scale;
> > + u8 duty_val;
> > int ret;
> >
> > if (state->polarity != PWM_POLARITY_NORMAL)
> > @@ -70,29 +70,31 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
> > * allow an external pull-down resistor to hold the GPIO3/LTX pin low.
> > */
> > - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
> > +
> > + if (state->duty_cycle < IQS620_PWM_PERIOD_NS)
> > + duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS;
> > + else
> > + duty_val = 256;
>
> The build gives a warning here since duty_val is a u8. Actually, I'm not
> a fan of calling this 'duty_val' since it has a different meaning than
> iqs620_pwm->duty_val (the cached version restored if the watchdog bites).
Argh, that's what you get if you want to just quickly get out a patch
:-\ Thanks again for your attention.
> > mutex_lock(&iqs620_pwm->lock);
> >
> > - if (!state->enabled || !duty_scale) {
> > + if (!state->enabled || !duty_val) {
> > ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> > IQS620_PWR_SETTINGS_PWM_OUT, 0);
> > if (ret)
> > goto err_mutex;
> > }
> >
> > - if (duty_scale) {
> > - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> > -
> > + if (duty_val) {
> > ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
> > - duty_val);
> > + duty_val - 1);
> > if (ret)
> > goto err_mutex;
> >
> > iqs620_pwm->duty_val = duty_val;
> > }
>
> This would need to change to iqs620_pwm->duty_val = duty_val - 1, otherwise
> the wrong duty cycle will get restored in iqs620_pwm_notifier. However this
> starts to look confusing.
>
> >
> > - if (state->enabled && duty_scale) {
> > + if (state->enabled && duty_val) {
> > ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> > IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
> > if (ret)
> > --
> > 2.29.2
> >
>
> How about the patch below instead? It solves the overflow problem you found,
> is minimally invasive and preserves the original intent. We still avoid the
> 64-bit division which is unnecessary anyway given this device's fixed period
> of only 1 ms.
>
> Kind regards,
> Jeff LaBundy
>
> diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> index 7d33e36..a15a2aa 100644
> --- a/drivers/pwm/pwm-iqs620a.c
> +++ b/drivers/pwm/pwm-iqs620a.c
> @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct iqs620_pwm_private *iqs620_pwm;
> struct iqs62x_core *iqs62x;
> - u64 duty_scale;
> + unsigned int duty_scale;
> int ret;
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> @@ -70,7 +70,9 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
> * allow an external pull-down resistor to hold the GPIO3/LTX pin low.
> */
> - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
> + duty_scale = (unsigned int)min_t(u64, state->duty_cycle,
> + IQS620_PWM_PERIOD_NS) * 256 /
> + IQS620_PWM_PERIOD_NS;
I had problems parsing this. I'd prefer:
if (state->duty_cycle < IQS620_PWM_PERIOD_NS) {
unsigned int duty_cycle = state->duty_cycle;
duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS;
} else {
duty_scale = 256;
}
or maybe:
unsigned int duty_cycle = min_t(u64, state->duty_cycle, IQS620_PWM_PERIOD_NS);
duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS;
(which depending on how clever the compiler is might be less effective
than the first suggestion).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pwm: iqs620a: Fix overflow and optimize calculations
2020-11-29 12:03 ` Uwe Kleine-König
@ 2020-11-29 21:15 ` Jeff LaBundy
0 siblings, 0 replies; 6+ messages in thread
From: Jeff LaBundy @ 2020-11-29 21:15 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Uwe Kleine-König, linux-pwm, Thierry Reding, Lee Jones, kernel
Hi Uwe,
On Sun, Nov 29, 2020 at 01:03:18PM +0100, Uwe Kleine-König wrote:
> On Fri, Nov 27, 2020 at 07:00:40PM -0600, Jeff LaBundy wrote:
> > Hi Uwe,
> >
> > Thank you for your work on this; a few comments below.
> >
> > On Fri, Nov 27, 2020 at 09:32:44PM +0100, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >
> > > If state->duty_cycle is 0x100000000000000, the previous calculation of
> > > duty_scale overflows and yields a duty cycle ratio of 0% instead of
> > > 100%. Fix this by comparing the requested duty cycle against the maximal
> > > possible duty cycle first. This way it is possible to use a native
> > > integer division instead of a (depending on the architecture) more
> > > expensive 64bit division. Also duty_val cannot be bigger than 0xff which
> > > allows to simplify the code a bit further down.
> > >
> > > Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator")
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello Jeff,
> > >
> > > On Fri, Nov 27, 2020 at 02:10:51PM -0600, Jeff LaBundy wrote:
> > > > I tested this patch on actual hardware but the newly calculated register
> > > > values are incorrect. We used to get:
> > > >
> > > > [...]
> > > > > goto err_mutex;
> > > > > }
> > > > >
> > > > > - if (duty_scale) {
> > > > > - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> > > > > -
> > > > > + if (duty_val) {
> > > >
> > > > This is part of the problem; the device's formula for duty cycle has a
> > > > plus one that is getting dropped now (see comments in iqs620_pwm_apply).
> > >
> > > Good catch, I indeed missed that - 1.
> > >
> > > This patch should be better in this regard.
> > >
> > > Thanks for particular attention and testing,
> > > Uwe
> > >
> > > drivers/pwm/pwm-iqs620a.c | 18 ++++++++++--------
> > > 1 file changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> > > index 7d33e3646436..6789e117f123 100644
> > > --- a/drivers/pwm/pwm-iqs620a.c
> > > +++ b/drivers/pwm/pwm-iqs620a.c
> > > @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > {
> > > struct iqs620_pwm_private *iqs620_pwm;
> > > struct iqs62x_core *iqs62x;
> > > - u64 duty_scale;
> > > + u8 duty_val;
> > > int ret;
> > >
> > > if (state->polarity != PWM_POLARITY_NORMAL)
> > > @@ -70,29 +70,31 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
> > > * allow an external pull-down resistor to hold the GPIO3/LTX pin low.
> > > */
> > > - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
> > > +
> > > + if (state->duty_cycle < IQS620_PWM_PERIOD_NS)
> > > + duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS;
> > > + else
> > > + duty_val = 256;
> >
> > The build gives a warning here since duty_val is a u8. Actually, I'm not
> > a fan of calling this 'duty_val' since it has a different meaning than
> > iqs620_pwm->duty_val (the cached version restored if the watchdog bites).
>
> Argh, that's what you get if you want to just quickly get out a patch
> :-\ Thanks again for your attention.
>
> > > mutex_lock(&iqs620_pwm->lock);
> > >
> > > - if (!state->enabled || !duty_scale) {
> > > + if (!state->enabled || !duty_val) {
> > > ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> > > IQS620_PWR_SETTINGS_PWM_OUT, 0);
> > > if (ret)
> > > goto err_mutex;
> > > }
> > >
> > > - if (duty_scale) {
> > > - u8 duty_val = min_t(u64, duty_scale - 1, 0xff);
> > > -
> > > + if (duty_val) {
> > > ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
> > > - duty_val);
> > > + duty_val - 1);
> > > if (ret)
> > > goto err_mutex;
> > >
> > > iqs620_pwm->duty_val = duty_val;
> > > }
> >
> > This would need to change to iqs620_pwm->duty_val = duty_val - 1, otherwise
> > the wrong duty cycle will get restored in iqs620_pwm_notifier. However this
> > starts to look confusing.
> >
> > >
> > > - if (state->enabled && duty_scale) {
> > > + if (state->enabled && duty_val) {
> > > ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> > > IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
> > > if (ret)
> > > --
> > > 2.29.2
> > >
> >
> > How about the patch below instead? It solves the overflow problem you found,
> > is minimally invasive and preserves the original intent. We still avoid the
> > 64-bit division which is unnecessary anyway given this device's fixed period
> > of only 1 ms.
> >
> > Kind regards,
> > Jeff LaBundy
> >
> > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> > index 7d33e36..a15a2aa 100644
> > --- a/drivers/pwm/pwm-iqs620a.c
> > +++ b/drivers/pwm/pwm-iqs620a.c
> > @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > {
> > struct iqs620_pwm_private *iqs620_pwm;
> > struct iqs62x_core *iqs62x;
> > - u64 duty_scale;
> > + unsigned int duty_scale;
> > int ret;
> >
> > if (state->polarity != PWM_POLARITY_NORMAL)
> > @@ -70,7 +70,9 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
> > * allow an external pull-down resistor to hold the GPIO3/LTX pin low.
> > */
> > - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS);
> > + duty_scale = (unsigned int)min_t(u64, state->duty_cycle,
> > + IQS620_PWM_PERIOD_NS) * 256 /
> > + IQS620_PWM_PERIOD_NS;
>
> I had problems parsing this. I'd prefer:
>
> if (state->duty_cycle < IQS620_PWM_PERIOD_NS) {
> unsigned int duty_cycle = state->duty_cycle;
> duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS;
> } else {
> duty_scale = 256;
> }
>
> or maybe:
>
> unsigned int duty_cycle = min_t(u64, state->duty_cycle, IQS620_PWM_PERIOD_NS);
> duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS;
>
> (which depending on how clever the compiler is might be less effective
> than the first suggestion).
I prefer the second option because duty_scale is assigned only once. This
way the duty_scale = 256 case falls out of the equation naturally without
having to be hard-coded.
One minor correction: you will want to use the local (clamped) duty_cycle
rather than state->duty_cycle for the second option. It is not needed for
the first option.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Kind regards,
Jeff LaBundy
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-29 21:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-27 14:14 [PATCH] pwm: iqs620a: Fix overflow and optimize calculations Uwe Kleine-König
2020-11-27 20:10 ` Jeff LaBundy
2020-11-27 20:32 ` [PATCH v2] " Uwe Kleine-König
2020-11-28 1:00 ` Jeff LaBundy
2020-11-29 12:03 ` Uwe Kleine-König
2020-11-29 21:15 ` Jeff LaBundy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.