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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Monday, May 9, 2022 17:56
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Wim Van Sebroeck
> <wim@xxxxxxxxxxxxxxxxxx>; open list:WATCHDOG DEVICE DRIVERS <linux-
> watchdog@xxxxxxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>
> Cc: ionut_n2001@xxxxxxxxx
> 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@xxxxxxxxx
> > 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@xxxxxxx>
> > ---
> >   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;
> >   	}
> >