Re: [watchdog] watchdog: mei_wdt: request stop on unregister

[Guenter Roeck <linux@roeck-us.net>]

Tomas,

On Thu, Jan 07, 2021 at 04:12:15PM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Jan 07, 2021 at 09:57:30PM +0200, Tomas Winkler wrote:
> > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > 
> > Send the stop command to the firmware on watchdog unregister
> > to eleminate false event on suspend.
> > 
> 
> Normally the watchdog driver would not be expected to unregister
> during suspend, only when the driver is manually unloaded.
> To support suspend/resume, other watchdog drivers implement
> suspend/resume functions to stop the watchdog on suspend and
> to restart it on resume. Unloading a watchdog driver on suspend
> would also have odd implications for userspace watchdog daemons.
> 
> On top of that, it should not actually be possible to unregister
> a watchdog while it is in use (because it is open in that case
> and should be marked as busy). watchdog_stop_on_unregister()
> only serves as backup in case someone actually manages to unload
> the driver while the watchdog is running. The function was
> implemented to avoid calls to stop the watchdog in the remove
> function because I can not mathematically prove that there are
> no situations where the watchdog is unloaded while running.
> However, I have not actually been able to do that.
> 
> Are you sure this patch is doing what you expect it to do ?
> 

I have not heard anything back. I tried to understand how this
patch would resolve a problem during suspend/resume, but
I didn't find anything.

Can you maybe add a log message showing the false event that is
prevented with this patch, and some context explaining how the
patch fixes the problem ?

Thanks,
Guenter

> Thanks,
> Guenter
> 
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >  drivers/watchdog/mei_wdt.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> > index 5391bf3e6b11..c5967d8b4256 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -382,6 +382,7 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> >  
> >  	watchdog_set_drvdata(&wdt->wdd, wdt);
> >  	watchdog_stop_on_reboot(&wdt->wdd);
> > +	watchdog_stop_on_unregister(&wdt->wdd);
> >  
> >  	ret = watchdog_register_device(&wdt->wdd);
> >  	if (ret)
> > -- 
> > 2.26.2
> >