* [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 related [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, other threads:[~2021-04-05 18:02 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).