Re: [PATCH] watchdog: imx2_wdt: Drop .remove callback

[Guenter Roeck]

On 2/24/20 3:44 AM, Anson Huang wrote:
> Hi, Uwe
>
> > Subject: Re: [PATCH] watchdog: imx2_wdt: Drop .remove callback
> >
> > On Mon, Feb 24, 2020 at 10:51:27AM +0800, Anson Huang wrote:
> > > .remove callback implementation doesn' call clk_disable_unprepare()
> > > which is buggy, actually, we can just use
> > > devm_watchdog_register_device() and
> > > devm_add_action_or_reset() to handle all necessary operations for
> > > remove action, then .remove callback can be dropped.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > > ---
> > >   drivers/watchdog/imx2_wdt.c | 37
> > > ++++++++++---------------------------
> > >   1 file changed, 10 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> > > index f8d58bf..1fe472f 100644
> > > --- a/drivers/watchdog/imx2_wdt.c
> > > +++ b/drivers/watchdog/imx2_wdt.c
> > > @@ -244,6 +244,11 @@ static const struct regmap_config
> > imx2_wdt_regmap_config = {
> > >   	.max_register = 0x8,
> > >   };
> > >
> > > +static void imx2_wdt_action(void *data) {
> > > +	clk_disable_unprepare(data);
> >
> > Does this have the effect of stopping the watchdog? Maybe we can have a
> > more expressive function name here (imx2_wdt_stop_clk or similar)?
>
> This action is ONLY called when probe failed or device is removed, and if watchdog
> is running, the core driver will prevent it from being removed.
>
> > Is there some watchdog core policy that tells if the watchdog should be
> > stopped on unload?
>
> watchdog_stop_on_unregister() should be called in .probe function to make core
> policy stop the watchdog before removing it, but I think this driver does NOT call
> it, maybe I should add the API call, need Guenter to help confirm.
The driver doesn't have a stop function, which implies that the watchdog
can not be stopped once started. Calling watchdog_stop_on_unregister()
seems to be pointless.

That also implies that the watchdog can not be unloaded after it has
been started since it can't be stopped. More on that below.

> > > +}
> > > +
> > >   static int __init imx2_wdt_probe(struct platform_device *pdev)  {
> > >   	struct device *dev = &pdev->dev;
> > > @@ -292,6 +297,10 @@ static int __init imx2_wdt_probe(struct
> > platform_device *pdev)
> > >   	if (ret)
> > >   		return ret;
> > >
> > > +	ret = devm_add_action_or_reset(dev, imx2_wdt_action, wdev->clk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >   	regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
> > >   	wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ?
> > WDIOF_CARDRESET : 0;
> > > @@ -315,32 +324,7 @@ static int __init imx2_wdt_probe(struct
> > platform_device *pdev)
> > >   	 */
> > >   	regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
> > >
> > > -	ret = watchdog_register_device(wdog);
> > > -	if (ret)
> > > -		goto disable_clk;
> > > -
> > > -	dev_info(dev, "timeout %d sec (nowayout=%d)\n",
> > > -		 wdog->timeout, nowayout);
> >
> > Does the core put this info in the kernel log? If not dropping it isn't obviously
> > right enough to be done en passant.
>
> This is just an info for user which I think NOT unnecessary, so I drop it in this patch
> as well.
>
> > > -	return 0;
> > > -
> > > -disable_clk:
> > > -	clk_disable_unprepare(wdev->clk);
> > > -	return ret;
> > > -}
> > > -
> > > -static int __exit imx2_wdt_remove(struct platform_device *pdev) -{
> > > -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> > > -	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
> > > -
> > > -	watchdog_unregister_device(wdog);
> > > -
> > > -	if (imx2_wdt_is_running(wdev)) {
> > > -		imx2_wdt_ping(wdog);
> > > -		dev_crit(&pdev->dev, "Device removed: Expect reboot!\n");
> > > -	}
> >
> > I also wonder about this one. This changes the timing behaviour and so
> > IMHO shouldn't be done as a side effect of a cleanup patch.
>
> Guenter has a comment of "use devm_watchdog_register_device(), and the watchdog subsystem
> should prevent removal if the watchdog is running ", so I thought no need to check the watchdog's
> status here, but after further check the core code of watchdog_cdev_unregister() function, I ONLY
> see it will check whether need to stop watchdog before unregister,

I would suggest for someone to try and trigger this message, and let me know
how you did it. If the watchdog is running, it should not be possible to unload
the driver; attempts to unload it should result in -EBUSY. If it is possible
to unload the driver, there is a bug in watchdog core which will need to get
fixed.

> ...
>
> 1083         if (watchdog_active(wdd) &&
> 1084             test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
> 1085                 watchdog_stop(wdd);
> 1086         }
>
> Hi, Guenter
> 	Do you think watchdog_stop_on_unregister() should be called in .probe function to
> make watchdog stop before unregister?
How would you expect the watchdog core to stop the watchdog
with no stop function in the driver ?

Thanks,
Guenter