| Message ID | 917e3890-7895-4b1c-bcee-4eecb3b7fe09@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pwm: samsung: Fix a bit test | expand |
Hello Dan, On Tue, Oct 17, 2023 at 05:04:08PM +0300, Dan Carpenter wrote: > This code has two problems. First, it passes the wrong bit parameter to > test_bit(). Second, it mixes using PWMF_REQUESTED in test_bit() and in > open coded bit tests. > > The test_bit() function takes a bit number. In other words, > "if (test_bit(0, &flags))" is the equivalent of "if (flags & (1 << 0))". > Passing (1 << 0) to test_bit() is like writing BIT(BIT(0)). It's a > double shift bug. > > In pwm_samsung_resume() these issues mean that the flag is never set and > the function is essentially a no-op. > > Fixes: 4c9548d24c0d ("pwm: samsung: Put per-channel data into driver data") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > From static analysis and not tested. > > drivers/pwm/pwm-samsung.c | 2 +- > include/linux/pwm.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c > index 10fe2c13cd80..acf4a0d8d990 100644 > --- a/drivers/pwm/pwm-samsung.c > +++ b/drivers/pwm/pwm-samsung.c > @@ -630,7 +630,7 @@ static int pwm_samsung_resume(struct device *dev) > struct pwm_device *pwm = &chip->pwms[i]; > struct samsung_pwm_channel *chan = &our_chip->channel[i]; > > - if (!(pwm->flags & PWMF_REQUESTED)) > + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) > continue; > > if (our_chip->variant.output_mask & BIT(i)) > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index e3b437587b32..3eee5bf367fb 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -41,8 +41,8 @@ struct pwm_args { > }; > > enum { > - PWMF_REQUESTED = 1 << 0, > - PWMF_EXPORTED = 1 << 1, > + PWMF_REQUESTED = 0, > + PWMF_EXPORTED = 1, I'd want s/ / / here. Or even not assign explicit values at all? > }; > > /* I'd say these are two separate issues, with the one in pwm-samsung being bad and the one in <linux/pwm.h> "only" ugly. I wonder how I could get the samsung part wrong. All current usages of PMWF_REQUESTED (and also PWMF_EXPORTED) use test_bit (et al). Grepping through history pwm-pca9685.c got this wrong in a similar way for some time, but otherwise it was always used correctly. The definition of the flags in <linux/pwm.h> is ugly since f051c466cf69 ("pwm: Allow chips to support multiple PWMs") from 2011! @Dan: Would you split the patch in two please? Thanks for catching that! Best regards Uwe
On Tue, Oct 24, 2023 at 11:11:57PM +0200, Uwe Kleine-König wrote: > Hello Dan, > > On Tue, Oct 17, 2023 at 05:04:08PM +0300, Dan Carpenter wrote: > > This code has two problems. First, it passes the wrong bit parameter to > > test_bit(). Second, it mixes using PWMF_REQUESTED in test_bit() and in > > open coded bit tests. > > > > The test_bit() function takes a bit number. In other words, > > "if (test_bit(0, &flags))" is the equivalent of "if (flags & (1 << 0))". > > Passing (1 << 0) to test_bit() is like writing BIT(BIT(0)). It's a > > double shift bug. > > > > In pwm_samsung_resume() these issues mean that the flag is never set and > > the function is essentially a no-op. > > > > Fixes: 4c9548d24c0d ("pwm: samsung: Put per-channel data into driver data") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > From static analysis and not tested. > > > > drivers/pwm/pwm-samsung.c | 2 +- > > include/linux/pwm.h | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c > > index 10fe2c13cd80..acf4a0d8d990 100644 > > --- a/drivers/pwm/pwm-samsung.c > > +++ b/drivers/pwm/pwm-samsung.c > > @@ -630,7 +630,7 @@ static int pwm_samsung_resume(struct device *dev) > > struct pwm_device *pwm = &chip->pwms[i]; > > struct samsung_pwm_channel *chan = &our_chip->channel[i]; > > > > - if (!(pwm->flags & PWMF_REQUESTED)) > > + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) > > continue; > > > > if (our_chip->variant.output_mask & BIT(i)) > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > index e3b437587b32..3eee5bf367fb 100644 > > --- a/include/linux/pwm.h > > +++ b/include/linux/pwm.h > > @@ -41,8 +41,8 @@ struct pwm_args { > > }; > > > > enum { > > - PWMF_REQUESTED = 1 << 0, > > - PWMF_EXPORTED = 1 << 1, > > + PWMF_REQUESTED = 0, > > + PWMF_EXPORTED = 1, > > I'd want s/ / / here. Or even not assign explicit values at all? > I feel like the 0 and 1 add value. But sure, I can remove the extra space. You're right that trying to align stuff is potentially going to cause pain in the future. > > }; > > > > /* > > I'd say these are two separate issues, with the one in pwm-samsung being > bad and the one in <linux/pwm.h> "only" ugly. > > I wonder how I could get the samsung part wrong. All current usages of > PMWF_REQUESTED (and also PWMF_EXPORTED) use test_bit (et al). Grepping > through history pwm-pca9685.c got this wrong in a similar way for some > time, but otherwise it was always used correctly. > > The definition of the flags in <linux/pwm.h> is ugly since > f051c466cf69 ("pwm: Allow chips to support multiple PWMs") from 2011! > > @Dan: Would you split the patch in two please? Sure. regards, dan carpenter
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c index 10fe2c13cd80..acf4a0d8d990 100644 --- a/drivers/pwm/pwm-samsung.c +++ b/drivers/pwm/pwm-samsung.c @@ -630,7 +630,7 @@ static int pwm_samsung_resume(struct device *dev) struct pwm_device *pwm = &chip->pwms[i]; struct samsung_pwm_channel *chan = &our_chip->channel[i]; - if (!(pwm->flags & PWMF_REQUESTED)) + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) continue; if (our_chip->variant.output_mask & BIT(i)) diff --git a/include/linux/pwm.h b/include/linux/pwm.h index e3b437587b32..3eee5bf367fb 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -41,8 +41,8 @@ struct pwm_args { }; enum { - PWMF_REQUESTED = 1 << 0, - PWMF_EXPORTED = 1 << 1, + PWMF_REQUESTED = 0, + PWMF_EXPORTED = 1, }; /*
This code has two problems. First, it passes the wrong bit parameter to test_bit(). Second, it mixes using PWMF_REQUESTED in test_bit() and in open coded bit tests. The test_bit() function takes a bit number. In other words, "if (test_bit(0, &flags))" is the equivalent of "if (flags & (1 << 0))". Passing (1 << 0) to test_bit() is like writing BIT(BIT(0)). It's a double shift bug. In pwm_samsung_resume() these issues mean that the flag is never set and the function is essentially a no-op. Fixes: 4c9548d24c0d ("pwm: samsung: Put per-channel data into driver data") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- From static analysis and not tested. drivers/pwm/pwm-samsung.c | 2 +- include/linux/pwm.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)