| Message ID | 20230902063232.22620-1-Hari.PrasathGE@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] pwm: atmel: add missing clk_disable_unprepare() | expand |
Le 02/09/2023 à 08:32, Hari Prasath Gujulan Elango a écrit : > Fix the below smatch warning: > > drivers/pwm/pwm-atmel-hlcdc.c:167 atmel_hlcdc_pwm_apply() warn: 'new_clk' from clk_prepare_enable() not released on lines: 112,137,142,149. > > 'Fixes: 2b4984bef47a5 ("pwm: atmel-hlcdc: Convert to the atomic PWM API")' Hi, There shouldn't be ' before Fixes:, neither at the end. Commit id should be 12 chars, not 13. There shouldn't be a blank line between Fixes and Signed-off-by. I think that the Fixes tag should be 2b4984bef47a ("pwm: add support for atmel-hlcdc-pwm device". The commit you point you have touched this code, be part of what you change was already there before that. > > Signed-off-by: Hari Prasath Gujulan Elango <Hari.PrasathGE@microchip.com> > There should be a --- between the signed-of-by and the below changelog, so that the changelog will not be merged in the git history. Also, it is also useful to add the link at lore.kernel.org of previous versions. Here, it would be something like: v1: https://lore.kernel.org/all/20230822070441.22170-1-Hari.PrasathGE@microchip.com/ > changelog of v2: > > - moved the clk_disable_unprepare to single point of return. > - cur_clk set to NULL before return. > --- > drivers/pwm/pwm-atmel-hlcdc.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c > index 96a709a9d49a..4d35b838203f 100644 > --- a/drivers/pwm/pwm-atmel-hlcdc.c > +++ b/drivers/pwm/pwm-atmel-hlcdc.c > @@ -44,7 +44,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm, > struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c); > struct atmel_hlcdc *hlcdc = chip->hlcdc; > unsigned int status; > - int ret; > + int ret = 0; This initialization looks un-needed and un-related to your changes. > > if (state->enabled) { > struct clk *new_clk = hlcdc->slow_clk; > @@ -109,7 +109,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm, > ATMEL_HLCDC_CLKPWMSEL, > gencfg); > if (ret) > - return ret; > + goto disable_new_clk; > } > > do_div(pwmcval, state->period); > @@ -134,18 +134,20 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm, > ATMEL_HLCDC_PWMPOL, > pwmcfg); > if (ret) > - return ret; > + goto disable_new_clk; > > ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, > ATMEL_HLCDC_PWM); > if (ret) > - return ret; > + goto disable_new_clk; > > ret = regmap_read_poll_timeout(hlcdc->regmap, ATMEL_HLCDC_SR, > status, > status & ATMEL_HLCDC_PWM, > 10, 0); > - if (ret) Removing this test looks wrong. > +disable_new_clk: > + clk_disable_unprepare(new_clk); > + chip->cur_clk = NULL; > return ret; This is a really unusual pattern. Usually, an error handling path is added at the end of the function, not in the middle. CJ > } else { > ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS,
Hello Christophe, On 02/09/23 9:57 pm, Christophe JAILLET wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know > the content is safe > > Le 02/09/2023 à 08:32, Hari Prasath Gujulan Elango a écrit : >> Fix the below smatch warning: >> >> drivers/pwm/pwm-atmel-hlcdc.c:167 atmel_hlcdc_pwm_apply() warn: >> 'new_clk' from clk_prepare_enable() not released on lines: >> 112,137,142,149. >> >> 'Fixes: 2b4984bef47a5 ("pwm: atmel-hlcdc: Convert to the atomic PWM >> API")' > > Hi, > > There shouldn't be ' before Fixes:, neither at the end. > Commit id should be 12 chars, not 13. > There shouldn't be a blank line between Fixes and Signed-off-by. > > I think that the Fixes tag should be 2b4984bef47a ("pwm: add support for > atmel-hlcdc-pwm device". > The commit you point you have touched this code, be part of what you > change was already there before that. > Thank you, I admit that I have messes up this part. Its been quite a while sending patches upstream and I seem to have forgotten the basics. I will take time to send the v3 paying more attention to these small details. >> >> Signed-off-by: Hari Prasath Gujulan Elango <Hari.PrasathGE@microchip.com> >> > > There should be a --- between the signed-of-by and the below changelog, > so that the changelog will not be merged in the git history. > > Also, it is also useful to add the link at lore.kernel.org of previous > versions. > > Here, it would be something like: > v1: > https://lore.kernel.org/all/20230822070441.22170-1-Hari.PrasathGE@microchip.com/ > >> changelog of v2: >> >> - moved the clk_disable_unprepare to single point of return. >> - cur_clk set to NULL before return. >> --- >> drivers/pwm/pwm-atmel-hlcdc.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c >> b/drivers/pwm/pwm-atmel-hlcdc.c >> index 96a709a9d49a..4d35b838203f 100644 >> --- a/drivers/pwm/pwm-atmel-hlcdc.c >> +++ b/drivers/pwm/pwm-atmel-hlcdc.c >> @@ -44,7 +44,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, >> struct pwm_device *pwm, >> struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c); >> struct atmel_hlcdc *hlcdc = chip->hlcdc; >> unsigned int status; >> - int ret; >> + int ret = 0; > > This initialization looks un-needed and un-related to your changes. > Though the kernel API's used below return 0 upon success but just thought I will initialize it to 0. >> >> if (state->enabled) { >> struct clk *new_clk = hlcdc->slow_clk; >> @@ -109,7 +109,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip >> *c, struct pwm_device *pwm, >> ATMEL_HLCDC_CLKPWMSEL, >> gencfg); >> if (ret) >> - return ret; >> + goto disable_new_clk; >> } >> >> do_div(pwmcval, state->period); >> @@ -134,18 +134,20 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip >> *c, struct pwm_device *pwm, >> ATMEL_HLCDC_PWMPOL, >> pwmcfg); >> if (ret) >> - return ret; >> + goto disable_new_clk; >> >> ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, >> ATMEL_HLCDC_PWM); >> if (ret) >> - return ret; >> + goto disable_new_clk; >> >> ret = regmap_read_poll_timeout(hlcdc->regmap, >> ATMEL_HLCDC_SR, >> status, >> status & ATMEL_HLCDC_PWM, >> 10, 0); >> - if (ret) > > Removing this test looks wrong. Will add it back and include a 'goto' > >> +disable_new_clk: >> + clk_disable_unprepare(new_clk); >> + chip->cur_clk = NULL; >> return ret; > > This is a really unusual pattern. > Usually, an error handling path is added at the end of the function, not > in the middle. > > CJ I will move this towards the end as it's done usually. -Hari > >> } else { >> ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS, >
Hello Hari, On Wed, Sep 06, 2023 at 06:15:36AM +0000, Hari.PrasathGE@microchip.com wrote: > Thank you, I admit that I have messes up this part. Its been quite a > while sending patches upstream and I seem to have forgotten the basics. > I will take time to send the v3 paying more attention to these small > details. You never followed up with the promised v3. Is this still on your radar? Would be great to get this addressed as it fixes a clk imbalance, right? Best regards Uwe
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c index 96a709a9d49a..4d35b838203f 100644 --- a/drivers/pwm/pwm-atmel-hlcdc.c +++ b/drivers/pwm/pwm-atmel-hlcdc.c @@ -44,7 +44,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm, struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c); struct atmel_hlcdc *hlcdc = chip->hlcdc; unsigned int status; - int ret; + int ret = 0; if (state->enabled) { struct clk *new_clk = hlcdc->slow_clk; @@ -109,7 +109,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm, ATMEL_HLCDC_CLKPWMSEL, gencfg); if (ret) - return ret; + goto disable_new_clk; } do_div(pwmcval, state->period); @@ -134,18 +134,20 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm, ATMEL_HLCDC_PWMPOL, pwmcfg); if (ret) - return ret; + goto disable_new_clk; ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM); if (ret) - return ret; + goto disable_new_clk; ret = regmap_read_poll_timeout(hlcdc->regmap, ATMEL_HLCDC_SR, status, status & ATMEL_HLCDC_PWM, 10, 0); - if (ret) +disable_new_clk: + clk_disable_unprepare(new_clk); + chip->cur_clk = NULL; return ret; } else { ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS,
Fix the below smatch warning: drivers/pwm/pwm-atmel-hlcdc.c:167 atmel_hlcdc_pwm_apply() warn: 'new_clk' from clk_prepare_enable() not released on lines: 112,137,142,149. 'Fixes: 2b4984bef47a5 ("pwm: atmel-hlcdc: Convert to the atomic PWM API")' Signed-off-by: Hari Prasath Gujulan Elango <Hari.PrasathGE@microchip.com> changelog of v2: - moved the clk_disable_unprepare to single point of return. - cur_clk set to NULL before return. --- drivers/pwm/pwm-atmel-hlcdc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)