Message ID | 20201127203244.3439478-1-uwe@kleine-koenig.org |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2] pwm: iqs620a: Fix overflow and optimize calculations | expand |
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);
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
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
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)