linux-kernel - Re: [PATCH] pwm: atmel: add missing clk_disable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2aab2c4f-cff5-f13d-e879-a5b1b00ef9c7@microchip.com>
Date:   Fri, 25 Aug 2023 03:04:07 +0000
From:   <Hari.PrasathGE@...rochip.com>
To:     <claudiu.beznea@...on.dev>, <thierry.reding@...il.com>,
        <u.kleine-koenig@...gutronix.de>, <Nicolas.Ferre@...rochip.com>,
        <alexandre.belloni@...tlin.com>
CC:     <linux-pwm@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pwm: atmel: add missing clk_disable_unprepare()

Hello Claudiu,

On 23/08/23 10:29 am, claudiu beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 8/22/23 10:04, Hari Prasath Gujulan Elango wrote:
>> 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.
>>
> 
> Can you add a fixes tag?
> 

yes I will add it.

>> Signed-off-by: Hari Prasath Gujulan Elango <Hari.PrasathGE@...rochip.com>
>> ---
>>   drivers/pwm/pwm-atmel-hlcdc.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
>> index 96a709a9d49a..ce46f6c74a14 100644
>> --- a/drivers/pwm/pwm-atmel-hlcdc.c
>> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
>> @@ -108,8 +108,10 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>>                                                 ATMEL_HLCDC_CFG(0),
>>                                                 ATMEL_HLCDC_CLKPWMSEL,
>>                                                 gencfg);
>> -                     if (ret)
>> +                     if (ret) {
>> +                             clk_disable_unprepare(new_clk);
>>                                return ret;
>> +                     }
>>                }
>>
>>                do_div(pwmcval, state->period);
>> @@ -133,20 +135,27 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>>                                         ATMEL_HLCDC_PWMPS_MASK |
>>                                         ATMEL_HLCDC_PWMPOL,
>>                                         pwmcfg);
>> -             if (ret)
>> +             if (ret) {
>> +                     clk_disable_unprepare(new_clk);
>>                        return ret;
>> +             }
>>
>>                ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN,
>>                                   ATMEL_HLCDC_PWM);
>> -             if (ret)
>> +             if (ret) {
>> +                     clk_disable_unprepare(new_clk);
>>                        return ret;
>> +             }
>>
>>                ret = regmap_read_poll_timeout(hlcdc->regmap, ATMEL_HLCDC_SR,
>>                                               status,
>>                                               status & ATMEL_HLCDC_PWM,
>>                                               10, 0);
>> -             if (ret)
>> +             if (ret) {
>> +                     clk_disable_unprepare(new_clk);
> 
> Can you keep a single failure point for all these?
> 
> Also, you have to set chip->cur_clk = NULL otherwise next time your apply
> will get executed the new_clk will not be enabled.
> 

I see that new_clk is assigned to cur_clk in the if (state->enabled) 
block and clk_disable_unprepare() is invoked only in the else block for 
cur_clk and its made NULL. I will cleanup all of this at a single point 
and resend v2.

Thanks,
Hari


> Thank you,
> Claudiu Beznea
> 
>>                        return ret;
>> +             }
>> +
>>        } else {
>>                ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS,
>>                                   ATMEL_HLCDC_PWM);