Date:   Tue, 25 Feb 2020 00:31:06 +0000
From:   Anson Huang <anson.huang@....com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] watchdog: imx2_wdt: Drop .remove callback

Hi, Guenter

> Subject: Re: [PATCH] watchdog: imx2_wdt: Drop .remove callback
> 
> 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@....com>
> >>> ---
> >>>   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 ?

Now I understand your point, thanks for you detail explanation.

Thanks,
Anson

Powered by blists - more mailing lists