[PATCH] watchdog: imx2_wdt: Drop .remove callback

[PATCH] watchdog: imx2_wdt: Drop .remove callback

[Anson Huang]

.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@nxp.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);
+}
+
 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);
-
-	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");
-	}
-	return 0;
+	return devm_watchdog_register_device(dev, wdog);
 }
 
 static void imx2_wdt_shutdown(struct platform_device *pdev)
@@ -417,7 +401,6 @@ static const struct of_device_id imx2_wdt_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, imx2_wdt_dt_ids);
 
 static struct platform_driver imx2_wdt_driver = {
-	.remove		= __exit_p(imx2_wdt_remove),
 	.shutdown	= imx2_wdt_shutdown,
 	.driver		= {
 		.name	= DRIVER_NAME,
-- 
2.7.4

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

[Guenter Roeck]

On 2/23/20 6:51 PM, 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@nxp.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   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);
> +}
> +
>   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);
> -
> -	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");
> -	}
> -	return 0;
> +	return devm_watchdog_register_device(dev, wdog);
>   }
>   
>   static void imx2_wdt_shutdown(struct platform_device *pdev)
> @@ -417,7 +401,6 @@ static const struct of_device_id imx2_wdt_dt_ids[] = {
>   MODULE_DEVICE_TABLE(of, imx2_wdt_dt_ids);
>   
>   static struct platform_driver imx2_wdt_driver = {
> -	.remove		= __exit_p(imx2_wdt_remove),
>   	.shutdown	= imx2_wdt_shutdown,
>   	.driver		= {
>   		.name	= DRIVER_NAME,
> 

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

[Uwe Kleine-König]

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@nxp.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)?

Is there some watchdog core policy that tells if the watchdog should be
stopped on unload?

> +}
> +
>  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.

> -	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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Anson Huang]

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@nxp.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.

> 
> > +}
> > +
> >  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,

...

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?

Thanks,
Anson.

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@nxp.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 ?

Thanks,
Guenter

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

[Uwe Kleine-König]

On Mon, Feb 24, 2020 at 06:15:17AM -0800, Guenter Roeck wrote:
> How would you expect the watchdog core to stop the watchdog
> with no stop function in the driver ?

I'm not 100% sure, but I think the situation is that you cannot stop the
watchdog in the watchdog register range, but if you stop the clock it
will never expire.

That's why I asked if the devm action callback stops the watchdog.

Didn't look into the details though ...

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Guenter Roeck]

On Mon, Feb 24, 2020 at 07:25:22PM +0100, Uwe Kleine-König wrote:
> On Mon, Feb 24, 2020 at 06:15:17AM -0800, Guenter Roeck wrote:
> > How would you expect the watchdog core to stop the watchdog
> > with no stop function in the driver ?
> 
> I'm not 100% sure, but I think the situation is that you cannot stop the
> watchdog in the watchdog register range, but if you stop the clock it
> will never expire.
> 
> That's why I asked if the devm action callback stops the watchdog.
> 

Again, it should not be possible to unload the driver if the watchdog
is running.

Guenter

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

[Anson Huang]

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@nxp.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