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

[Marion & Christophe JAILLET]

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 at amd.com>
>
> Reviewed-by: Guenter Roeck <linux at roeck-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 at roeck-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);
>