| Message ID | 20230828095006.3602426-1-srinivas.neeli@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V3] watchdog: of_xilinx_wdt: Remove unnecessary clock disable call in the remove path | expand |
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@amd.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > Changes in V3: > -> Added "clk_disable() in xwdt_selftest() error path. > Changes in V2: > -> Fixed typo in "To" list(linux@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); >
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@amd.com> > > Reviewed-by: Guenter Roeck <linux@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@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); >
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);
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@amd.com> --- Changes in V3: -> Added "clk_disable() in xwdt_selftest() error path. Changes in V2: -> Fixed typo in "To" list(linux@roeck-us.net). --- drivers/watchdog/of_xilinx_wdt.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)