If watchdog hardware has been disabled, currently the kernel driver
will show at err level during probe:
"Watchdog hardware is disabled"
This is unnecessarily verbose as there is already a -ENODEV returned.
Lower the level to debug.
Reported-by: [email protected]
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215762
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/watchdog/sp5100_tco.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 86ffb58fbc85..e51ecbd5c8b7 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -286,7 +286,7 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
val = readl(SP5100_WDT_CONTROL(tco->tcobase));
if (val & SP5100_WDT_DISABLED) {
- dev_err(dev, "Watchdog hardware is disabled\n");
+ dev_dbg(dev, "Watchdog hardware is disabled\n");
return -ENODEV;
}
--
2.34.1
On 5/9/22 09:33, Mario Limonciello wrote:
> If watchdog hardware has been disabled, currently the kernel driver
> will show at err level during probe:
>
> "Watchdog hardware is disabled"
>
> This is unnecessarily verbose as there is already a -ENODEV returned.
> Lower the level to debug.
Is it ? Without this message, a user may try to load the driver,
get an error message, and have no idea why the driver was not
enabled even though the hardware exists. If anything , -ENODEV
is less than perfect. Unfortunately there does not seem to be
a better error code, or at least I don't see one.
Guenter
>
> Reported-by: [email protected]
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215762
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
> drivers/watchdog/sp5100_tco.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 86ffb58fbc85..e51ecbd5c8b7 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -286,7 +286,7 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
>
> val = readl(SP5100_WDT_CONTROL(tco->tcobase));
> if (val & SP5100_WDT_DISABLED) {
> - dev_err(dev, "Watchdog hardware is disabled\n");
> + dev_dbg(dev, "Watchdog hardware is disabled\n");
> return -ENODEV;
> }
>
On 5/9/22 16:10, Limonciello, Mario wrote:
> [Public]
>
>
>
>> -----Original Message-----
>> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
>> Sent: Monday, May 9, 2022 17:56
>> To: Limonciello, Mario <[email protected]>; Wim Van Sebroeck
>> <[email protected]>; open list:WATCHDOG DEVICE DRIVERS <linux-
>> [email protected]>; open list <[email protected]>
>> Cc: [email protected]
>> Subject: Re: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled
>> watchdog hardware
>>
>> On 5/9/22 09:33, Mario Limonciello wrote:
>>> If watchdog hardware has been disabled, currently the kernel driver
>>> will show at err level during probe:
>>>
>>> "Watchdog hardware is disabled"
>>>
>>> This is unnecessarily verbose as there is already a -ENODEV returned.
>>> Lower the level to debug.
>>
>> Is it ? Without this message, a user may try to load the driver,
>> get an error message, and have no idea why the driver was not
>> enabled even though the hardware exists. If anything , -ENODEV
>> is less than perfect. Unfortunately there does not seem to be
>> a better error code, or at least I don't see one.
>
> If it didn't have modaliases and users only manually loaded it; I would agree
> with you. However it has MODULE_DEVICE_TABLE, so if that PCI device is around
> then the driver will load either way. That would translate into an "error message"
> on every boot if you have this module compiled and didn't manually try to load it.
>
Why don't you just blacklist the driver ?
Guenter
[Public]
> -----Original Message-----
> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
> Sent: Monday, May 9, 2022 17:56
> To: Limonciello, Mario <[email protected]>; Wim Van Sebroeck
> <[email protected]>; open list:WATCHDOG DEVICE DRIVERS <linux-
> [email protected]>; open list <[email protected]>
> Cc: [email protected]
> Subject: Re: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled
> watchdog hardware
>
> On 5/9/22 09:33, Mario Limonciello wrote:
> > If watchdog hardware has been disabled, currently the kernel driver
> > will show at err level during probe:
> >
> > "Watchdog hardware is disabled"
> >
> > This is unnecessarily verbose as there is already a -ENODEV returned.
> > Lower the level to debug.
>
> Is it ? Without this message, a user may try to load the driver,
> get an error message, and have no idea why the driver was not
> enabled even though the hardware exists. If anything , -ENODEV
> is less than perfect. Unfortunately there does not seem to be
> a better error code, or at least I don't see one.
If it didn't have modaliases and users only manually loaded it; I would agree
with you. However it has MODULE_DEVICE_TABLE, so if that PCI device is around
then the driver will load either way. That would translate into an "error message"
on every boot if you have this module compiled and didn't manually try to load it.
If debug is too quiet; how about info level instead?
>
> Guenter
>
> >
> > Reported-by: [email protected]
> > Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D215762&data=05%7C01%7Cm
> ario.limonciello%40amd.com%7Ccb7bc29e837747aeca4a08da320f1569%7C3d
> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637877337609997805%7CU
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=i%2FrEQEXw
> n19%2Fi3oJ0aqomniBaQe9WKGGiDQ97YeCfss%3D&reserved=0
> > Signed-off-by: Mario Limonciello <[email protected]>
> > ---
> > drivers/watchdog/sp5100_tco.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/sp5100_tco.c
> b/drivers/watchdog/sp5100_tco.c
> > index 86ffb58fbc85..e51ecbd5c8b7 100644
> > --- a/drivers/watchdog/sp5100_tco.c
> > +++ b/drivers/watchdog/sp5100_tco.c
> > @@ -286,7 +286,7 @@ static int sp5100_tco_timer_init(struct sp5100_tco
> *tco)
> >
> > val = readl(SP5100_WDT_CONTROL(tco->tcobase));
> > if (val & SP5100_WDT_DISABLED) {
> > - dev_err(dev, "Watchdog hardware is disabled\n");
> > + dev_dbg(dev, "Watchdog hardware is disabled\n");
> > return -ENODEV;
> > }
> >
On 5/9/22 19:33, Jerry Hoemann wrote:
> On Mon, May 09, 2022 at 03:55:54PM -0700, Guenter Roeck wrote:
>> On 5/9/22 09:33, Mario Limonciello wrote:
>>> If watchdog hardware has been disabled, currently the kernel driver
>>> will show at err level during probe:
>>>
>>> "Watchdog hardware is disabled"
>>>
>>> This is unnecessarily verbose as there is already a -ENODEV returned.
>>> Lower the level to debug.
>>
>> Is it ? Without this message, a user may try to load the driver,
>> get an error message, and have no idea why the driver was not
>> enabled even though the hardware exists. If anything , -ENODEV
>> is less than perfect. Unfortunately there does not seem to be
>> a better error code, or at least I don't see one.
>>
>> Guenter
>
> Coincidentally, I was looking at this code on Friday.
>
> Some HPE Proliant servers are disabling the AMD WDT in BIOS. However,
> sp5100_tco was still getting configured. It was the lack of
> "Watchdog hardware is disabled" message that helped clue us into
> what was going on (Linux is enabling the WDT anyway.)
>
> So, I liked that this message exists.
>
> I'll send an RFC patch for this other issue as it orthogonal.
> But just wanted to point out the message is useful.
I personally don't have a problem blacklisting on a system I encounter
this. I take anything at "err" level as there is a firmware problem or a
hardware problem that should be looked at.
As the message is genuinely useful as Jerry points out how about meeting
in the middle at info or notice?
>
>
>>
>>>
>>> Reported-by: [email protected]
>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D215762&data=05%7C01%7Cmario.limonciello%40amd.com%7C5eb5f65caaf241d1515308da321cbd10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637877396427792688%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tqQYyfwbGK%2BTvTxlkef0I2iKyLe8WsOjxskA0SQPlcw%3D&reserved=0
>>> Signed-off-by: Mario Limonciello <[email protected]>
>>> ---
>>> drivers/watchdog/sp5100_tco.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>>> index 86ffb58fbc85..e51ecbd5c8b7 100644
>>> --- a/drivers/watchdog/sp5100_tco.c
>>> +++ b/drivers/watchdog/sp5100_tco.c
>>> @@ -286,7 +286,7 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
>>> val = readl(SP5100_WDT_CONTROL(tco->tcobase));
>>> if (val & SP5100_WDT_DISABLED) {
>>> - dev_err(dev, "Watchdog hardware is disabled\n");
>>> + dev_dbg(dev, "Watchdog hardware is disabled\n");
>>> return -ENODEV;
>>> }
>
On 5/9/22 17:37, Mario Limonciello wrote:
> On 5/9/22 19:33, Jerry Hoemann wrote:
>> On Mon, May 09, 2022 at 03:55:54PM -0700, Guenter Roeck wrote:
>>> On 5/9/22 09:33, Mario Limonciello wrote:
>>>> If watchdog hardware has been disabled, currently the kernel driver
>>>> will show at err level during probe:
>>>>
>>>> "Watchdog hardware is disabled"
>>>>
>>>> This is unnecessarily verbose as there is already a -ENODEV returned.
>>>> Lower the level to debug.
>>>
>>> Is it ? Without this message, a user may try to load the driver,
>>> get an error message, and have no idea why the driver was not
>>> enabled even though the hardware exists. If anything , -ENODEV
>>> is less than perfect. Unfortunately there does not seem to be
>>> a better error code, or at least I don't see one.
>>>
>>> Guenter
>>
>> Coincidentally, I was looking at this code on Friday.
>>
>> Some HPE Proliant servers are disabling the AMD WDT in BIOS. However,
>> sp5100_tco was still getting configured. It was the lack of
>> "Watchdog hardware is disabled" message that helped clue us into
>> what was going on (Linux is enabling the WDT anyway.)
>>
>> So, I liked that this message exists.
>>
>> I'll send an RFC patch for this other issue as it orthogonal.
>> But just wanted to point out the message is useful.
>
> I personally don't have a problem blacklisting on a system I encounter this. I take anything at "err" level as there is a firmware problem or a hardware problem that should be looked at.
>
> As the message is genuinely useful as Jerry points out how about meeting in the middle at info or notice?
>
I really don't want to change it. It does point out a serious issue,
intentional or not. Anyone concerned or disturbed about the message
can easily block the module from loading, and others may rely on it.
From driver perspective it _is_ an error, and it should be treated
as such.
Guenter
On Mon, May 09, 2022 at 03:55:54PM -0700, Guenter Roeck wrote:
> On 5/9/22 09:33, Mario Limonciello wrote:
> > If watchdog hardware has been disabled, currently the kernel driver
> > will show at err level during probe:
> >
> > "Watchdog hardware is disabled"
> >
> > This is unnecessarily verbose as there is already a -ENODEV returned.
> > Lower the level to debug.
>
> Is it ? Without this message, a user may try to load the driver,
> get an error message, and have no idea why the driver was not
> enabled even though the hardware exists. If anything , -ENODEV
> is less than perfect. Unfortunately there does not seem to be
> a better error code, or at least I don't see one.
>
> Guenter
Coincidentally, I was looking at this code on Friday.
Some HPE Proliant servers are disabling the AMD WDT in BIOS. However,
sp5100_tco was still getting configured. It was the lack of
"Watchdog hardware is disabled" message that helped clue us into
what was going on (Linux is enabling the WDT anyway.)
So, I liked that this message exists.
I'll send an RFC patch for this other issue as it orthogonal.
But just wanted to point out the message is useful.
>
> >
> > Reported-by: [email protected]
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215762
> > Signed-off-by: Mario Limonciello <[email protected]>
> > ---
> > drivers/watchdog/sp5100_tco.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> > index 86ffb58fbc85..e51ecbd5c8b7 100644
> > --- a/drivers/watchdog/sp5100_tco.c
> > +++ b/drivers/watchdog/sp5100_tco.c
> > @@ -286,7 +286,7 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
> > val = readl(SP5100_WDT_CONTROL(tco->tcobase));
> > if (val & SP5100_WDT_DISABLED) {
> > - dev_err(dev, "Watchdog hardware is disabled\n");
> > + dev_dbg(dev, "Watchdog hardware is disabled\n");
> > return -ENODEV;
> > }
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------