[PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[Mario Limonciello]

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: ionut_n2001@yahoo.com
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215762
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 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

Re: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[Guenter Roeck]

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: ionut_n2001@yahoo.com
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215762
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   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;
>   	}
>   

RE: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, May 9, 2022 17:56
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Wim Van Sebroeck
> <wim@linux-watchdog.org>; open list:WATCHDOG DEVICE DRIVERS <linux-
> watchdog@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Cc: ionut_n2001@yahoo.com
> 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: ionut_n2001@yahoo.com
> > Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D215762&amp;data=05%7C01%7Cm
> ario.limonciello%40amd.com%7Ccb7bc29e837747aeca4a08da320f1569%7C3d
> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637877337609997805%7CU
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=i%2FrEQEXw
> n19%2Fi3oJ0aqomniBaQe9WKGGiDQ97YeCfss%3D&amp;reserved=0
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >   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;
> >   	}
> >

Re: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[Guenter Roeck]

On 5/9/22 16:10, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Monday, May 9, 2022 17:56
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Wim Van Sebroeck
>> <wim@linux-watchdog.org>; open list:WATCHDOG DEVICE DRIVERS <linux-
>> watchdog@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
>> Cc: ionut_n2001@yahoo.com
>> 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

Re: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[Jerry Hoemann]

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: ionut_n2001@yahoo.com
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215762
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >   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
-----------------------------------------------------------------------------

Re: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[Mario Limonciello]

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: ionut_n2001@yahoo.com
>>> 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 <mario.limonciello@amd.com>
>>> ---
>>>    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;
>>>    	}
> 

Re: [PATCH] Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware

[Guenter Roeck]

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