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@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 ?
Now I understand your point, thanks for you detail explanation.
Thanks,
Anson
