| Message ID | 20210405125946.369230-1-eichest@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: imx_sc_wdt: fix pretimeout | expand |
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; > } > >
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;". > } > >
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
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
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; }