* [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).