RE: RE: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped

RE: RE: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped

[liuchao (CR)]

On 1/26/25 21:10, Guenter Roeck wrote:
> On 1/26/25 00:38, Liu Chao wrote:
> > Every time the user echoes 0 > /dev/watchdog0, meaningless critical
> > log is printed.
> >
> 
> It is not meaningless, and it will still be displayed after this change, making the
> change pointless.

The change is not pointless. For example, the softdog driver does not invoke 
watchdog_stop or print logs in the watchdog_release.

Liu Chao

> 
> Guenter
> 
> > Signed-off-by: Liu Chao <liuchao173@huawei.com>
> > ---
> >   drivers/watchdog/watchdog_dev.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/watchdog/watchdog_dev.c
> > b/drivers/watchdog/watchdog_dev.c index e2bd266b1b5b..0a9d5e6f3a88
> > 100644
> > --- a/drivers/watchdog/watchdog_dev.c
> > +++ b/drivers/watchdog/watchdog_dev.c
> > @@ -960,14 +960,15 @@ static int watchdog_release(struct inode *inode,
> struct file *file)
> >   	if (!watchdog_active(wdd))
> >   		err = 0;
> >   	else if (test_and_clear_bit(_WDOG_ALLOW_RELEASE,
> &wd_data->status) ||
> > -		 !(wdd->info->options & WDIOF_MAGICCLOSE))
> > +		 !(wdd->info->options & WDIOF_MAGICCLOSE)) {
> >   		err = watchdog_stop(wdd);
> > +		if (err < 0)
> > +			pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
> > +	}
> >
> >   	/* If the watchdog was not stopped, send a keepalive ping */
> > -	if (err < 0) {
> > -		pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
> > +	if (err < 0)
> >   		watchdog_ping(wdd);
> > -	}
> >
> >   	watchdog_update_worker(wdd);
> >

Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped

[Guenter Roeck]

On 1/27/25 01:35, liuchao (CR) wrote:
> On 1/26/25 21:10, Guenter Roeck wrote:
>> On 1/26/25 00:38, Liu Chao wrote:
>>> Every time the user echoes 0 > /dev/watchdog0, meaningless critical
>>> log is printed.
>>>
>>
>> It is not meaningless, and it will still be displayed after this change, making the
>> change pointless.
> 
> The change is not pointless. For example, the softdog driver does not invoke
> watchdog_stop or print logs in the watchdog_release.
> 

It seems to me that is a problem in the softdog driver.

The change is actually worse than I initially thought.
The message is _supposed_ to be displayed if watchdog_stop()
is not called while the watchdog is running (i.e., if err == -EBUSY).
Otherwise it would not be displayed for real hardware watchdogs
which are not stopped because they were running and watchdog_stop()
is not called because WDIOF_MAGICCLOSE is set in the driver and the
magic release byte was not written.

Specifically, the softdog driver has WDIOF_MAGICCLOSE set. It is not
supposed to be unloadable (or unloaded) while the watchdog is running.

Guenter