linux-kernel - Re: [PATCH V3] watchdog: of_xilinx_wdt: Remove unnecessary clock disable call in the remove path

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 28 Aug 2023 20:05:27 +0200
From:   Marion & Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Guenter Roeck <linux@...ck-us.net>,
        Srinivas Neeli <srinivas.neeli@....com>,
        shubhrajyoti.datta@....com, michal.simek@....com,
        wim@...ux-watchdog.org
Cc:     linux-watchdog@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        git@....com, neelisrinivas18@...il.com
Subject: Re: [PATCH V3] watchdog: of_xilinx_wdt: Remove unnecessary clock
 disable call in the remove path


Le 28/08/2023 à 12:08, Guenter Roeck a écrit :
> On 8/28/23 02:50, Srinivas Neeli wrote:
>> There is a mismatch in axi clock enable and disable calls.
>> The axi clock is enabled and disabled by the probe function,
>> then it is again disabled in the remove path.
>> So observed the call trace while removing the module.
>> Use the clk_enable() and devm_clk_get_prepared() functions
>> instead of devm_clk_get_enable() to avoid an extra clock disable
>> call from the remove path.
>>
>>   Call trace:
>>    clk_core_disable+0xb0/0xc0
>>    clk_disable+0x30/0x4c
>>    clk_disable_unprepare+0x18/0x30
>>    devm_clk_release+0x24/0x40
>>    devres_release_all+0xc8/0x190
>>    device_unbind_cleanup+0x18/0x6c
>>    device_release_driver_internal+0x20c/0x250
>>    device_release_driver+0x18/0x24
>>    bus_remove_device+0x124/0x130
>>    device_del+0x174/0x440
>>
>> Fixes: 4de0224c6fbe ("watchdog: of_xilinx_wdt: Use 
>> devm_clk_get_enabled() helper")
>> Signed-off-by: Srinivas Neeli <srinivas.neeli@....com>
>
> Reviewed-by: Guenter Roeck <linux@...ck-us.net>
>

Hi, I'm not sure the Fixes tag is correct.

This issue was there before it.
Commit 4de0224c6fbe is just a clean-up and shouldn't change the behavior 
of the code.

I think that the issue was introduced in 2017 in b6bc41645547. (should 
the bellow patch be backported in older stable kernels)

CJ


>> ---
>> Changes in V3:
>> -> Added "clk_disable() in xwdt_selftest() error path.
>> Changes in V2:
>> -> Fixed typo in "To" list(linux@...ck-us.net).
>> ---
>>   drivers/watchdog/of_xilinx_wdt.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/of_xilinx_wdt.c 
>> b/drivers/watchdog/of_xilinx_wdt.c
>> index 05657dc1d36a..352853e6fe71 100644
>> --- a/drivers/watchdog/of_xilinx_wdt.c
>> +++ b/drivers/watchdog/of_xilinx_wdt.c
>> @@ -187,7 +187,7 @@ static int xwdt_probe(struct platform_device *pdev)
>>         watchdog_set_nowayout(xilinx_wdt_wdd, enable_once);
>>   -    xdev->clk = devm_clk_get_enabled(dev, NULL);
>> +    xdev->clk = devm_clk_get_prepared(dev, NULL);
>>       if (IS_ERR(xdev->clk)) {
>>           if (PTR_ERR(xdev->clk) != -ENOENT)
>>               return PTR_ERR(xdev->clk);
>> @@ -218,18 +218,25 @@ static int xwdt_probe(struct platform_device 
>> *pdev)
>>       spin_lock_init(&xdev->spinlock);
>>       watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
>>   +    rc = clk_enable(xdev->clk);
>> +    if (rc) {
>> +        dev_err(dev, "unable to enable clock\n");
>> +        return rc;
>> +    }
>> +
>>       rc = xwdt_selftest(xdev);
>>       if (rc == XWT_TIMER_FAILED) {
>>           dev_err(dev, "SelfTest routine error\n");
>> +        clk_disable(xdev->clk);
>>           return rc;
>>       }
>>   +    clk_disable(xdev->clk);
>> +
>>       rc = devm_watchdog_register_device(dev, xilinx_wdt_wdd);
>>       if (rc)
>>           return rc;
>>   -    clk_disable(xdev->clk);
>> -
>>       dev_info(dev, "Xilinx Watchdog Timer with timeout %ds\n",
>>            xilinx_wdt_wdd->timeout);
>