* [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.