watchdog: imx_sc_wdt: fix pretimeout

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(-)

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

>  }
>  
>

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