* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
@ 2025-02-05 2:02 liuchao (CR)
2025-02-05 2:35 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: liuchao (CR) @ 2025-02-05 2:02 UTC (permalink / raw)
To: linux@roeck-us.net
Cc: caihe, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org, lixiaokeng,
wim@linux-watchdog.org
On 1/27/2025 22:30, Guenter Roeck wrote:
> 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.
When echo to /dev/watchdog0, The watchdog_open, watchdog_write, and
watchdog_release functions are invoked in sequence. Do you mean the softdog
driver should not call watchdog_release?
After the user opens /dev/watchdog0, the user feeds the watchdog through ioctl
WDIOC_KEEPALIVE and never closes. Is this the correct usage?
>
> Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
2025-02-05 2:02 [PATCH] watchdog: only print critical log when the watchdog fails to be stopped liuchao (CR)
@ 2025-02-05 2:35 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-02-05 2:35 UTC (permalink / raw)
To: liuchao (CR)
Cc: caihe, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org, lixiaokeng,
wim@linux-watchdog.org
On 2/4/25 18:02, liuchao (CR) wrote:
> On 1/27/2025 22:30, Guenter Roeck wrote:
>> 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.
>
> When echo to /dev/watchdog0, The watchdog_open, watchdog_write, and
> watchdog_release functions are invoked in sequence. Do you mean the softdog
> driver should not call watchdog_release?
>
> After the user opens /dev/watchdog0, the user feeds the watchdog through ioctl
> WDIOC_KEEPALIVE and never closes. Is this the correct usage?
>
I tried softdog. It works as advertised. Yes, "echo 0 > watchdog0" triggers the message.
"sudo modprobe -r softdog" then fails with
modprobe: FATAL: Module softdog is in use.
and, as expected, one minute later (or whatever the timeout is set to) the system reboots.
There is nothing wrong with the message. The softdog _does_ refuse to be unloaded
while running, and it _does_ reboot the system after the timeout expired. This is all
perfectly as expected. The log is not meaningless. Instead, it tells the user that
the system will reboot after the watchdog expired. Which it does.
Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
@ 2025-02-05 3:14 liuchao (CR)
2025-02-05 4:28 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: liuchao (CR) @ 2025-02-05 3:14 UTC (permalink / raw)
To: linux@roeck-us.net
Cc: caihe, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org, lixiaokeng,
wim@linux-watchdog.org
On 2/3/2025 18:35, Guenter Roeck wrote:
> On 2/4/25 18:02, liuchao (CR) wrote:
> > On 1/27/2025 22:30, Guenter Roeck wrote:
> >> 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.
> >
> > When echo to /dev/watchdog0, The watchdog_open, watchdog_write, and
> > watchdog_release functions are invoked in sequence. Do you mean the
> > softdog driver should not call watchdog_release?
> >
> > After the user opens /dev/watchdog0, the user feeds the watchdog
> > through ioctl WDIOC_KEEPALIVE and never closes. Is this the correct usage?
> >
>
> I tried softdog. It works as advertised. Yes, "echo 0 > watchdog0" triggers the
> message.
> "sudo modprobe -r softdog" then fails with
>
> modprobe: FATAL: Module softdog is in use.
>
> and, as expected, one minute later (or whatever the timeout is set to) the
> system reboots.
>
> There is nothing wrong with the message. The softdog _does_ refuse to be
> unloaded while running, and it _does_ reboot the system after the timeout
> expired. This is all perfectly as expected. The log is not meaningless. Instead, it
> tells the user that the system will reboot after the watchdog expired. Which it
> does.
User creates a process and echoes every time to ensure that the system is
normal, the process can be executed properly. Otherwise, the system reboots.
So the critical message keeps printing.
Is it reasonable to change it to info level?
>
> Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
2025-02-05 3:14 liuchao (CR)
@ 2025-02-05 4:28 ` Guenter Roeck
2025-02-05 5:20 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2025-02-05 4:28 UTC (permalink / raw)
To: liuchao (CR)
Cc: caihe, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org, lixiaokeng,
wim@linux-watchdog.org
On 2/4/25 19:14, liuchao (CR) wrote:
> On 2/3/2025 18:35, Guenter Roeck wrote:
>> On 2/4/25 18:02, liuchao (CR) wrote:
>>> On 1/27/2025 22:30, Guenter Roeck wrote:
>>>> 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.
>>>
>>> When echo to /dev/watchdog0, The watchdog_open, watchdog_write, and
>>> watchdog_release functions are invoked in sequence. Do you mean the
>>> softdog driver should not call watchdog_release?
>>>
>>> After the user opens /dev/watchdog0, the user feeds the watchdog
>>> through ioctl WDIOC_KEEPALIVE and never closes. Is this the correct usage?
>>>
>>
>> I tried softdog. It works as advertised. Yes, "echo 0 > watchdog0" triggers the
>> message.
>> "sudo modprobe -r softdog" then fails with
>>
>> modprobe: FATAL: Module softdog is in use.
>>
>> and, as expected, one minute later (or whatever the timeout is set to) the
>> system reboots.
>>
>> There is nothing wrong with the message. The softdog _does_ refuse to be
>> unloaded while running, and it _does_ reboot the system after the timeout
>> expired. This is all perfectly as expected. The log is not meaningless. Instead, it
>> tells the user that the system will reboot after the watchdog expired. Which it
>> does.
>
> User creates a process and echoes every time to ensure that the system is
> normal, the process can be executed properly. Otherwise, the system reboots.
> So the critical message keeps printing.
>
> Is it reasonable to change it to info level?
>
No. The user needs to echo the magic character instead of "0".
Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
2025-02-05 4:28 ` Guenter Roeck
@ 2025-02-05 5:20 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-02-05 5:20 UTC (permalink / raw)
To: liuchao (CR)
Cc: caihe, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org, lixiaokeng,
wim@linux-watchdog.org
On 2/4/25 20:28, Guenter Roeck wrote:
>>
>> User creates a process and echoes every time to ensure that the system is
>> normal, the process can be executed properly. Otherwise, the system reboots.
>> So the critical message keeps printing.
>>
>> Is it reasonable to change it to info level?
>>
>
> No. The user needs to echo the magic character instead of "0".
>
Actually, the underlying problem is that the user keeps closing
the watchdog device, which is not the intended use since the watchdog
is supposed to only run while the watchdog device is open. That is
why applications like watchdogd are normally used to manage and
control a watchdog.
The problem here is that the user insists in doing something wrong,
and you want the kernel to adjust to that wrong usage. In a sense,
this is like persistently driving above the speed limit, and instead
of paying the resulting fines (or reducing the speed), the user wants
the penalties to go away or to have them replaced with an informational
notice. That is not how it works, sorry.
If the user insists on their current approach, they will have to live
with the consequences.
Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
@ 2025-01-27 9:44 liuchao (CR)
0 siblings, 0 replies; 9+ messages in thread
From: liuchao (CR) @ 2025-01-27 9:44 UTC (permalink / raw)
To: linux@roeck-us.net
Cc: caihe, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org, liuchao (CR), lixiaokeng,
wim@linux-watchdog.org
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);
> >
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: RE: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
@ 2025-01-27 9:35 liuchao (CR)
2025-01-27 14:29 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: liuchao (CR) @ 2025-01-27 9:35 UTC (permalink / raw)
To: Guenter Roeck, wim@linux-watchdog.org,
linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: caihe, lixiaokeng
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);
> >
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
2025-01-27 9:35 liuchao (CR)
@ 2025-01-27 14:29 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-01-27 14:29 UTC (permalink / raw)
To: liuchao (CR), wim@linux-watchdog.org,
linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: caihe, lixiaokeng
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
@ 2025-01-26 8:38 Liu Chao
2025-01-26 13:10 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: Liu Chao @ 2025-01-26 8:38 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: caihe, lixiaokeng
Every time the user echoes 0 > /dev/watchdog0, meaningless critical log
is printed.
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);
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] watchdog: only print critical log when the watchdog fails to be stopped
2025-01-26 8:38 Liu Chao
@ 2025-01-26 13:10 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-01-26 13:10 UTC (permalink / raw)
To: Liu Chao, wim, linux-watchdog, linux-kernel; +Cc: caihe, lixiaokeng
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.
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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-02-05 5:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-05 2:02 [PATCH] watchdog: only print critical log when the watchdog fails to be stopped liuchao (CR)
2025-02-05 2:35 ` Guenter Roeck
-- strict thread matches above, loose matches on Subject: below --
2025-02-05 3:14 liuchao (CR)
2025-02-05 4:28 ` Guenter Roeck
2025-02-05 5:20 ` Guenter Roeck
2025-01-27 9:44 liuchao (CR)
2025-01-27 9:35 liuchao (CR)
2025-01-27 14:29 ` Guenter Roeck
2025-01-26 8:38 Liu Chao
2025-01-26 13:10 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox