| Message ID | 20220509163304.86-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Watchdog: sp5100_tco: Lower verbosity of disabled watchdog hardware | expand |
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; > } >
[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&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 <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; > > } > >
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
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; > > }
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; >>> } >
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
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; }
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(-)