Re: [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@xxxxxxx>
>
> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>

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@xxxxxxxxxxxx).
> > ---
> >   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);