EDAC/dmc520: Don't print an error for each unconfigured interrupt line

[PATCH] EDAC/dmc520: Don't print an error for each unconfigured interrupt line

Posted by Tyler Hicks 4 years, 5 months ago

The dmc520 driver requires that at least one interrupt line, out of the ten
possible, is configured. The driver prints an error and returns -EINVAL
from its .probe function if there are no interrupt lines configured.

Don't print a KERN_ERR level message for each interrupt line that's
unconfigured as that can confuse users into thinking that there is an
error condition.

Before this change, the following KERN_ERR level messages would be
reported if only dram_ecc_errc and dram_ecc_errd were configured in the
device tree:

 dmc520 68000000.dmc: IRQ ram_ecc_errc not found
 dmc520 68000000.dmc: IRQ ram_ecc_errd not found
 dmc520 68000000.dmc: IRQ failed_access not found
 dmc520 68000000.dmc: IRQ failed_prog not found
 dmc520 68000000.dmc: IRQ link_err not
 dmc520 68000000.dmc: IRQ temperature_event not found
 dmc520 68000000.dmc: IRQ arch_fsm not found
 dmc520 68000000.dmc: IRQ phy_request not found

Fixes: 1088750d7839 ("EDAC: Add EDAC driver for DMC520")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: <stable@vger.kernel.org>
Reported-by: Sinan Kaya <okaya@kernel.org>
---
 drivers/edac/dmc520_edac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/dmc520_edac.c b/drivers/edac/dmc520_edac.c
index b8a7d9594afd..1fa5ca57e9ec 100644
--- a/drivers/edac/dmc520_edac.c
+++ b/drivers/edac/dmc520_edac.c
@@ -489,7 +489,7 @@ static int dmc520_edac_probe(struct platform_device *pdev)
 	dev = &pdev->dev;
 
 	for (idx = 0; idx < NUMBER_OF_IRQS; idx++) {
-		irq = platform_get_irq_byname(pdev, dmc520_irq_configs[idx].name);
+		irq = platform_get_irq_byname_optional(pdev, dmc520_irq_configs[idx].name);
 		irqs[idx] = irq;
 		masks[idx] = dmc520_irq_configs[idx].mask;
 		if (irq >= 0) {
-- 
2.25.1

RE: [PATCH] EDAC/dmc520: Don't print an error for each unconfigured interrupt line

Posted by Lei Wang (DPLAT) 4 years, 5 months ago

> -----Original Message-----
> From: Tyler Hicks <tyhicks@linux.microsoft.com>
> Sent: Tuesday, January 11, 2022 8:38 AM
> To: Lei Wang (DPLAT) <Wang.Lei@microsoft.com>; Borislav Petkov
> <bp@alien8.de>; Tony Luck <tony.luck@intel.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>
> Cc: Sinan Kaya <okaya@kernel.org>; Shiping Ji <shiping.linux@gmail.com>;
> James Morse <james.morse@arm.com>; Robert Richter <rric@kernel.org>;
> linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] EDAC/dmc520: Don't print an error for each unconfigured
> interrupt line
> 
> The dmc520 driver requires that at least one interrupt line, out of the ten
> possible, is configured. The driver prints an error and returns -EINVAL from
> its .probe function if there are no interrupt lines configured.
> 
> Don't print a KERN_ERR level message for each interrupt line that's
> unconfigured as that can confuse users into thinking that there is an error
> condition.
> 
> Before this change, the following KERN_ERR level messages would be reported
> if only dram_ecc_errc and dram_ecc_errd were configured in the device tree:
> 
>  dmc520 68000000.dmc: IRQ ram_ecc_errc not found
>  dmc520 68000000.dmc: IRQ ram_ecc_errd not found
>  dmc520 68000000.dmc: IRQ failed_access not found
>  dmc520 68000000.dmc: IRQ failed_prog not found
>  dmc520 68000000.dmc: IRQ link_err not
>  dmc520 68000000.dmc: IRQ temperature_event not found
>  dmc520 68000000.dmc: IRQ arch_fsm not found
>  dmc520 68000000.dmc: IRQ phy_request not found
> 
> Fixes: 1088750d7839 ("EDAC: Add EDAC driver for DMC520")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>

Looks good. EDAC-CORE maintainers, please take the patch through your tree. 
Thanks!

Acked-by: Lei Wang <lewan@microsoft.com>

> Cc: <stable@vger.kernel.org>
> Reported-by: Sinan Kaya <okaya@kernel.org>
> ---
>  drivers/edac/dmc520_edac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/dmc520_edac.c b/drivers/edac/dmc520_edac.c index
> b8a7d9594afd..1fa5ca57e9ec 100644
> --- a/drivers/edac/dmc520_edac.c
> +++ b/drivers/edac/dmc520_edac.c
> @@ -489,7 +489,7 @@ static int dmc520_edac_probe(struct platform_device
> *pdev)
>  	dev = &pdev->dev;
> 
>  	for (idx = 0; idx < NUMBER_OF_IRQS; idx++) {
> -		irq = platform_get_irq_byname(pdev,
> dmc520_irq_configs[idx].name);
> +		irq = platform_get_irq_byname_optional(pdev,
> +dmc520_irq_configs[idx].name);
>  		irqs[idx] = irq;
>  		masks[idx] = dmc520_irq_configs[idx].mask;
>  		if (irq >= 0) {
> --
> 2.25.1

Re: [PATCH] EDAC/dmc520: Don't print an error for each unconfigured interrupt line

Posted by Borislav Petkov 4 years, 5 months ago

On Tue, Jan 11, 2022 at 10:38:00AM -0600, Tyler Hicks wrote:
> The dmc520 driver requires that at least one interrupt line, out of the ten
> possible, is configured. The driver prints an error and returns -EINVAL
> from its .probe function if there are no interrupt lines configured.
> 
> Don't print a KERN_ERR level message for each interrupt line that's
> unconfigured as that can confuse users into thinking that there is an
> error condition.
> 
> Before this change, the following KERN_ERR level messages would be
> reported if only dram_ecc_errc and dram_ecc_errd were configured in the
> device tree:
> 
>  dmc520 68000000.dmc: IRQ ram_ecc_errc not found
>  dmc520 68000000.dmc: IRQ ram_ecc_errd not found
>  dmc520 68000000.dmc: IRQ failed_access not found
>  dmc520 68000000.dmc: IRQ failed_prog not found
>  dmc520 68000000.dmc: IRQ link_err not
>  dmc520 68000000.dmc: IRQ temperature_event not found
>  dmc520 68000000.dmc: IRQ arch_fsm not found
>  dmc520 68000000.dmc: IRQ phy_request not found
> 
> Fixes: 1088750d7839 ("EDAC: Add EDAC driver for DMC520")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: <stable@vger.kernel.org>

Why stable? AFAICT, this is fixing only the spew of some error messages
but the driver is still functional.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] EDAC/dmc520: Don't print an error for each unconfigured interrupt line

Posted by Tyler Hicks 4 years, 5 months ago

On 2022-01-16 19:29:46, Borislav Petkov wrote:
> On Tue, Jan 11, 2022 at 10:38:00AM -0600, Tyler Hicks wrote:
> > The dmc520 driver requires that at least one interrupt line, out of the ten
> > possible, is configured. The driver prints an error and returns -EINVAL
> > from its .probe function if there are no interrupt lines configured.
> > 
> > Don't print a KERN_ERR level message for each interrupt line that's
> > unconfigured as that can confuse users into thinking that there is an
> > error condition.
> > 
> > Before this change, the following KERN_ERR level messages would be
> > reported if only dram_ecc_errc and dram_ecc_errd were configured in the
> > device tree:
> > 
> >  dmc520 68000000.dmc: IRQ ram_ecc_errc not found
> >  dmc520 68000000.dmc: IRQ ram_ecc_errd not found
> >  dmc520 68000000.dmc: IRQ failed_access not found
> >  dmc520 68000000.dmc: IRQ failed_prog not found
> >  dmc520 68000000.dmc: IRQ link_err not
> >  dmc520 68000000.dmc: IRQ temperature_event not found
> >  dmc520 68000000.dmc: IRQ arch_fsm not found
> >  dmc520 68000000.dmc: IRQ phy_request not found
> > 
> > Fixes: 1088750d7839 ("EDAC: Add EDAC driver for DMC520")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: <stable@vger.kernel.org>
> 
> Why stable? AFAICT, this is fixing only the spew of some error messages
> but the driver is still functional.

KERN_ERR messages trip log scanners and cause concern that the
kernel/hardware is not configured or working correctly. They also add a
little big of ongoing stress into kernel maintainer's lives, as we
prepare and test kernel updates, since they show up as red text in
journalctl output that we have to think about regularly. Multiple
KERN_ERR messages, 8 in this case, can also be considered a little worse
than a single error message.

I feel like this trivial fix is worth taking into stable rather than
suppressing these errors (mentally and in log scanners) for years.

Tyler

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
>

Re: [PATCH] EDAC/dmc520: Don't print an error for each unconfigured interrupt line

Posted by Borislav Petkov 4 years, 5 months ago

On Tue, Jan 18, 2022 at 09:28:16AM -0600, Tyler Hicks wrote:
> KERN_ERR messages trip log scanners and cause concern that the
> kernel/hardware is not configured or working correctly. They also add a
> little big of ongoing stress into kernel maintainer's lives, as we
> prepare and test kernel updates, since they show up as red text in
> journalctl output that we have to think about regularly. Multiple
> KERN_ERR messages, 8 in this case, can also be considered a little worse
> than a single error message.

It sounds to me like you wanna read

Documentation/process/stable-kernel-rules.rst

first.

> I feel like this trivial fix is worth taking into stable rather than
> suppressing these errors (mentally and in log scanners) for years.

Years? 

In any case, sorry, no, I don't consider this stable material.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] EDAC/dmc520: Don't print an error for each unconfigured interrupt line

Posted by Tyler Hicks 4 years, 5 months ago

On 2022-01-18 18:28:16, Borislav Petkov wrote:
> On Tue, Jan 18, 2022 at 09:28:16AM -0600, Tyler Hicks wrote:
> > KERN_ERR messages trip log scanners and cause concern that the
> > kernel/hardware is not configured or working correctly. They also add a
> > little big of ongoing stress into kernel maintainer's lives, as we
> > prepare and test kernel updates, since they show up as red text in
> > journalctl output that we have to think about regularly. Multiple
> > KERN_ERR messages, 8 in this case, can also be considered a little worse
> > than a single error message.
> 
> It sounds to me like you wanna read
> 
> Documentation/process/stable-kernel-rules.rst
> 
> first.

I'm familiar with it and the sort of commits that flow into stable.

> > I feel like this trivial fix is worth taking into stable rather than
> > suppressing these errors (mentally and in log scanners) for years.
> 
> Years? 

Yes, years. v5.10 is supported through 2026.

> In any case, sorry, no, I don't consider this stable material.

The bar varies by subsystem maintainer but this wouldn't be the first
logging fix that made it into a stable branch. From the linux-5.10.y
branch of linux-stable:

 ddb13ddacc60 scsi: pm80xx: Fix misleading log statement in pm8001_mpi_get_nvmd_resp()
 526261c1b706 amd/display: downgrade validation failure log level
 9a3f52f73c04 bnxt_en: Improve logging of error recovery settings information.
 5f7bda9ba8d7 leds: lm3697: Don't spam logs when probe is deferred
 8b195380cd07 staging: fbtft: Don't spam logs when probe is deferred
 ...

But you do the hard work of maintaining the subsystem tree so you get to
call the shots about where fixes are routed. :) Thanks for applying the
change!

Tyler

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
>