Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] watchdog: imx_sc_wdt: fix pretimeout
@ 2021-04-05 12:59 eichest
  2021-04-05 13:23 ` Ahmad Fatoum
  2021-04-05 14:42 ` Guenter Roeck
  0 siblings, 2 replies; 5+ messages in thread
From: eichest @ 2021-04-05 12:59 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Wim Van Sebroeck, Guenter Roeck, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Stefan Eichenberger

From: Stefan Eichenberger <eichest@gmail.com>

If the WDIOF_PRETIMEOUTE flag is not set when registering the device the
driver will not show the sysfs entries or register the default governor.
By moving the registering after the decision whether pretimeout is
supported this gets fixed.

Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
 drivers/watchdog/imx_sc_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
index e9ee22a7cb45..eddb1ae630e0 100644
--- a/drivers/watchdog/imx_sc_wdt.c
+++ b/drivers/watchdog/imx_sc_wdt.c
@@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(wdog);
 	watchdog_stop_on_unregister(wdog);
 
-	ret = devm_watchdog_register_device(dev, wdog);
-	if (ret)
-		return ret;
-
 	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
 				       SC_IRQ_WDOG,
 				       true);
@@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	else
 		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
 
+	ret = devm_watchdog_register_device(dev, wdog);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] watchdog: imx_sc_wdt: fix pretimeout
  2021-04-05 12:59 [PATCH] watchdog: imx_sc_wdt: fix pretimeout eichest
@ 2021-04-05 13:23 ` Ahmad Fatoum
  2021-04-05 14:42 ` Guenter Roeck
  1 sibling, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2021-04-05 13:23 UTC (permalink / raw)
  To: eichest, linux-watchdog
  Cc: Shawn Guo, Sascha Hauer, NXP Linux Team, Pengutronix Kernel Team,
	Wim Van Sebroeck, Fabio Estevam, Guenter Roeck


On 05.04.21 14:59, eichest@gmail.com wrote:
> From: Stefan Eichenberger <eichest@gmail.com>
> 
> If the WDIOF_PRETIMEOUTE flag is not set when registering the device the
> driver will not show the sysfs entries or register the default governor.
> By moving the registering after the decision whether pretimeout is
> supported this gets fixed.
> 
> Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
> ---
>  drivers/watchdog/imx_sc_wdt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
> index e9ee22a7cb45..eddb1ae630e0 100644
> --- a/drivers/watchdog/imx_sc_wdt.c
> +++ b/drivers/watchdog/imx_sc_wdt.c
> @@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
>  	watchdog_stop_on_reboot(wdog);
>  	watchdog_stop_on_unregister(wdog);
>  
> -	ret = devm_watchdog_register_device(dev, wdog);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
>  				       SC_IRQ_WDOG,
>  				       true);
> @@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
>  	else
>  		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
>  
> +	ret = devm_watchdog_register_device(dev, wdog);
> +	if (ret)
> +		return ret;

This could be rewritten as return devm_watchdog_register_device(dev, wdog);

> +
>  	return 0;
>  }
>  
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] watchdog: imx_sc_wdt: fix pretimeout
  2021-04-05 12:59 [PATCH] watchdog: imx_sc_wdt: fix pretimeout eichest
  2021-04-05 13:23 ` Ahmad Fatoum
@ 2021-04-05 14:42 ` Guenter Roeck
  2021-04-05 17:43   ` Stefan Eichenberger
  1 sibling, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2021-04-05 14:42 UTC (permalink / raw)
  To: eichest, linux-watchdog
  Cc: Wim Van Sebroeck, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team

On 4/5/21 5:59 AM, eichest@gmail.com wrote:
> From: Stefan Eichenberger <eichest@gmail.com>
> 
> If the WDIOF_PRETIMEOUTE flag is not set when registering the device the

WDIOF_PRETIMEOUT

> driver will not show the sysfs entries or register the default governor.
> By moving the registering after the decision whether pretimeout is
> supported this gets fixed.
> 

This is problematic. If the notifier is called and the watchdog is not yet
registered, it will call watchdog_notify_pretimeout on an unregistered
watchdog device which would crash the system. The notifier function needs
to handle that situation. This is for both registration and removal: On
removal, the watchdog device would be unregistered first (because of the
use of devm_ functions) and, again, the notifier could be called on an
unregistered watchdog device.

> Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
> ---
>  drivers/watchdog/imx_sc_wdt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
> index e9ee22a7cb45..eddb1ae630e0 100644
> --- a/drivers/watchdog/imx_sc_wdt.c
> +++ b/drivers/watchdog/imx_sc_wdt.c
> @@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
>  	watchdog_stop_on_reboot(wdog);
>  	watchdog_stop_on_unregister(wdog);
>  
> -	ret = devm_watchdog_register_device(dev, wdog);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
>  				       SC_IRQ_WDOG,
>  				       true);
> @@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
>  	else
>  		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
>  
> +	ret = devm_watchdog_register_device(dev, wdog);
> +	if (ret)
> +		return ret;
> +
>  	return 0;

This is equivalent to just "return ret;".

>  }
>  
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] watchdog: imx_sc_wdt: fix pretimeout
  2021-04-05 14:42 ` Guenter Roeck
@ 2021-04-05 17:43   ` Stefan Eichenberger
  2021-04-05 18:02     ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Stefan Eichenberger @ 2021-04-05 17:43 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-watchdog, Wim Van Sebroeck, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team

Hi Guenter,

On Mon, Apr 05, 2021 at 07:42:20AM -0700, Guenter Roeck wrote:
> On 4/5/21 5:59 AM, eichest@gmail.com wrote:
> > From: Stefan Eichenberger <eichest@gmail.com>
> > 
> > If the WDIOF_PRETIMEOUTE flag is not set when registering the device the
> 
> WDIOF_PRETIMEOUT
> 

Thanks for that finding.

> > driver will not show the sysfs entries or register the default governor.
> > By moving the registering after the decision whether pretimeout is
> > supported this gets fixed.
> > 
> 
> This is problematic. If the notifier is called and the watchdog is not yet
> registered, it will call watchdog_notify_pretimeout on an unregistered
> watchdog device which would crash the system. The notifier function needs
> to handle that situation. This is for both registration and removal: On
> removal, the watchdog device would be unregistered first (because of the
> use of devm_ functions) and, again, the notifier could be called on an
> unregistered watchdog device.
> 

Isn't that handled in watchdog_notify_pretimeout with if (!wdd->gov)?
As I understand the code, a governor is registered earliest in
watchdog_register_pretimeout, which is called by watchdog_dev_register.
The same when doing an unregister, which should be called when doing a
watchdog_dev_unregister.

> > Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
> > ---
> >  drivers/watchdog/imx_sc_wdt.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
> > index e9ee22a7cb45..eddb1ae630e0 100644
> > --- a/drivers/watchdog/imx_sc_wdt.c
> > +++ b/drivers/watchdog/imx_sc_wdt.c
> > @@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
> >  	watchdog_stop_on_reboot(wdog);
> >  	watchdog_stop_on_unregister(wdog);
> >  
> > -	ret = devm_watchdog_register_device(dev, wdog);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> >  				       SC_IRQ_WDOG,
> >  				       true);
> > @@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
> >  	else
> >  		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
> >  
> > +	ret = devm_watchdog_register_device(dev, wdog);
> > +	if (ret)
> > +		return ret;
> > +
> >  	return 0;
> 
> This is equivalent to just "return ret;".
> 

Thanks, will fix that too. 

Regards,
Stefan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] watchdog: imx_sc_wdt: fix pretimeout
  2021-04-05 17:43   ` Stefan Eichenberger
@ 2021-04-05 18:02     ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2021-04-05 18:02 UTC (permalink / raw)
  To: Stefan Eichenberger
  Cc: linux-watchdog, Wim Van Sebroeck, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team

On Mon, Apr 05, 2021 at 07:43:52PM +0200, Stefan Eichenberger wrote:
> Hi Guenter,
> 
> On Mon, Apr 05, 2021 at 07:42:20AM -0700, Guenter Roeck wrote:
> > On 4/5/21 5:59 AM, eichest@gmail.com wrote:
> > > From: Stefan Eichenberger <eichest@gmail.com>
> > > 
> > > If the WDIOF_PRETIMEOUTE flag is not set when registering the device the
> > 
> > WDIOF_PRETIMEOUT
> > 
> 
> Thanks for that finding.
> 
> > > driver will not show the sysfs entries or register the default governor.
> > > By moving the registering after the decision whether pretimeout is
> > > supported this gets fixed.
> > > 
> > 
> > This is problematic. If the notifier is called and the watchdog is not yet
> > registered, it will call watchdog_notify_pretimeout on an unregistered
> > watchdog device which would crash the system. The notifier function needs
> > to handle that situation. This is for both registration and removal: On
> > removal, the watchdog device would be unregistered first (because of the
> > use of devm_ functions) and, again, the notifier could be called on an
> > unregistered watchdog device.
> > 
> 
> Isn't that handled in watchdog_notify_pretimeout with if (!wdd->gov)?
> As I understand the code, a governor is registered earliest in
> watchdog_register_pretimeout, which is called by watchdog_dev_register.
> The same when doing an unregister, which should be called when doing a
> watchdog_dev_unregister.
> 

Ah yes, you are correct. Thanks for checking, and sorry for the noise.

Guenter

> > > Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
> > > ---
> > >  drivers/watchdog/imx_sc_wdt.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
> > > index e9ee22a7cb45..eddb1ae630e0 100644
> > > --- a/drivers/watchdog/imx_sc_wdt.c
> > > +++ b/drivers/watchdog/imx_sc_wdt.c
> > > @@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
> > >  	watchdog_stop_on_reboot(wdog);
> > >  	watchdog_stop_on_unregister(wdog);
> > >  
> > > -	ret = devm_watchdog_register_device(dev, wdog);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> > >  				       SC_IRQ_WDOG,
> > >  				       true);
> > > @@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
> > >  	else
> > >  		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
> > >  
> > > +	ret = devm_watchdog_register_device(dev, wdog);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	return 0;
> > 
> > This is equivalent to just "return ret;".
> > 
> 
> Thanks, will fix that too. 
> 
> Regards,
> Stefan

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-05 12:59 [PATCH] watchdog: imx_sc_wdt: fix pretimeout eichest
2021-04-05 13:23 ` Ahmad Fatoum
2021-04-05 14:42 ` Guenter Roeck
2021-04-05 17:43   ` Stefan Eichenberger
2021-04-05 18:02     ` Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git