e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Frans Pop]

With the current kernel I suddenly get the following error displayed
during boot:
  e1000e: Intel(R) PRO/1000 Network Driver - 1.0.2-k2
  e1000e: Copyright (c) 1999-2008 Intel Corporation.
  e1000e 0000:00:19.0: PCI INT A -> GSI 22 (level, low) -> IRQ 22
! e1000e 0000:00:19.0: pci_enable_pcie_error_reporting failed 0xfffffffb
  e1000e 0000:00:19.0: setting latency timer to 64
  e1000e 0000:00:19.0: irq 26 for MSI/MSI-X

This looks to be the consequence of:
commit 68eac4602b9104cdaa6c18b3edd914cececa6a1e
Author: Xiaotian Feng <dfeng@redhat.com>
Date:   Fri Aug 14 14:35:52 2009 +0000
    e1000e: fix use of pci_enable_pcie_error_reporting

Looking at the change it seems logical that this error, that I previously
only saw during resumes from suppend to RAM, now also shows up during
system boot. So I don't think it can be qualified as a regression, but it
is certainly rather annoying.

Questions:
- is it possible that the error occurs due to a kernel bug, or is this
  certain to be due to broken hardware?
- as the error is non-fatal, should it maybe be changed from dev_err
  to dev_info, so that at least it does not show up during a boot
  with 'quiet'?
- if broken hardware is common, should the kernel complain at all,
  or at least maybe show a slightly "friendlier" message?

System: hp2510p (ICH8, Core2 Duo) running x86_64, Debian stable ("Lenny")

00:19.0 Ethernet controller [0200]: Intel Corporation 82566MM
        Gigabit Network Connection [8086:1049] (rev 03)
    Subsystem: Hewlett-Packard Company Device 30c9
    Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
    Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
    Latency: 0
    Interrupt: pin A routed to IRQ 26
    Region 0: Memory at e0620000 (32-bit, non-prefetchable) [size=128K]
    Region 1: Memory at e0640000 (32-bit, non-prefetchable) [size=4K]
    Region 2: I/O ports at 2060 [size=32]
    Capabilities: [c8] Power Management version 2
            Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
            Status: D0 PME-Enable- DSel=0 DScale=1 PME-
    Capabilities: [d0] Message Signalled Interrupts: Mask- 64bit+ Queue=0/0 Enable+
            Address: 00000000fee0300c  Data: 41b1
    Kernel driver in use: e1000e
    Kernel modules: e1000e

I can provide full dmesg and kernel config if desired.

Cheers,
FJP

Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Bjørn Mork]

Frans Pop <elendil@planet.nl> writes:

> - if broken hardware is common, should the kernel complain at all,
>   or at least maybe show a slightly "friendlier" message?

FWIW, I see the same message on resume on slighly different hardware, so
it's at least more common than just a single configuration:

[80885.498749] e1000e 0000:00:19.0: PCI INT A disabled
[80885.498755] e1000e 0000:00:19.0: PME# enabled
[80885.498763] e1000e 0000:00:19.0: wake-up capability enabled by ACPI
[80887.614545] e1000e 0000:00:19.0: PCI INT A -> GSI 20 (level, low) -> IRQ 20
[80887.614548] e1000e 0000:00:19.0: pci_enable_pcie_error_reporting failed 0xfffffffb
[80887.614554] e1000e 0000:00:19.0: setting latency timer to 64
[80887.614559] e1000e 0000:00:19.0: wake-up capability disabled by ACPI
[80887.614565] e1000e 0000:00:19.0: PME# disabled
[80887.614567] e1000e 0000:00:19.0: wake-up capability disabled by ACPI
[80887.614572] e1000e 0000:00:19.0: PME# disabled
[80887.614626] e1000e 0000:00:19.0: irq 26 for MSI/MSI-X

nemi:/etc/apt# lspci -vvnns 00:19.0
00:19.0 Ethernet controller [0200]: Intel Corporation 82567LM Gigabit Network Connection [8086:10f5] (rev 03)
        Subsystem: Lenovo Device [17aa:20ee]
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx+
        Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
        Latency: 0
        Interrupt: pin A routed to IRQ 26
        Region 0: Memory at f0600000 (32-bit, non-prefetchable) [size=128K]
        Region 1: Memory at f0625000 (32-bit, non-prefetchable) [size=4K]
        Region 2: I/O ports at 1840 [size=32]
        Capabilities: [c8] Power Management version 2
                Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
                Status: D0 PME-Enable- DSel=0 DScale=1 PME-
        Capabilities: [d0] Message Signalled Interrupts: Mask- 64bit+ Queue=0/0 Enable+
                Address: 00000000fee0300c  Data: 41b2
        Capabilities: [e0] PCIe advanced features <?>
        Kernel driver in use: e1000e
        Kernel modules: e1000e


The system is a Lenovo Thinkpad X301. The chipset is ICH9M.

I guess the "non-fatal, continue" comment is a pretty strong indication
that dev_err() is overkill here?


	/* AER (Advanced Error Reporting) hooks */
	err = pci_enable_pcie_error_reporting(pdev);
	if (err) {
		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
		        "0x%x\n", err);
		/* non-fatal, continue */
	}




Bjørn

Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Danny Feng]

On 08/20/2009 11:14 PM, Frans Pop wrote:
> With the current kernel I suddenly get the following error displayed
> during boot:
>    e1000e: Intel(R) PRO/1000 Network Driver - 1.0.2-k2
>    e1000e: Copyright (c) 1999-2008 Intel Corporation.
>    e1000e 0000:00:19.0: PCI INT A ->  GSI 22 (level, low) ->  IRQ 22
> ! e1000e 0000:00:19.0: pci_enable_pcie_error_reporting failed 0xfffffffb
>    e1000e 0000:00:19.0: setting latency timer to 64
>    e1000e 0000:00:19.0: irq 26 for MSI/MSI-X
>
> This looks to be the consequence of:
> commit 68eac4602b9104cdaa6c18b3edd914cececa6a1e
> Author: Xiaotian Feng<dfeng@redhat.com>
> Date:   Fri Aug 14 14:35:52 2009 +0000
>      e1000e: fix use of pci_enable_pcie_error_reporting
>
> Looking at the change it seems logical that this error, that I previously
> only saw during resumes from suppend to RAM, now also shows up during
> system boot. So I don't think it can be qualified as a regression, but it
> is certainly rather annoying.
>
> Questions:
> - is it possible that the error occurs due to a kernel bug, or is this
>    certain to be due to broken hardware?
I think this messages comes from hardware does not support pcie aer.
In fact the messages is not fatal and can be ignored. Some drivers
is silence with pci_enable_pcie_error_reporting ....

My patch is just to fix the logic of pcie aer usage ...
> - as the error is non-fatal, should it maybe be changed from dev_err
>    to dev_info, so that at least it does not show up during a boot
>    with 'quiet'?
I agree with this.... Maybe some of other drivers needs this too 
(igb,ixgbe)...
> - if broken hardware is common, should the kernel complain at all,
>    or at least maybe show a slightly "friendlier" message?
>
> System: hp2510p (ICH8, Core2 Duo) running x86_64, Debian stable ("Lenny")
>
> 00:19.0 Ethernet controller [0200]: Intel Corporation 82566MM
>          Gigabit Network Connection [8086:1049] (rev 03)
>      Subsystem: Hewlett-Packard Company Device 30c9
>      Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
>      Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast>TAbort-<TAbort-<MAbort->SERR-<PERR- INTx-
>      Latency: 0
>      Interrupt: pin A routed to IRQ 26
>      Region 0: Memory at e0620000 (32-bit, non-prefetchable) [size=128K]
>      Region 1: Memory at e0640000 (32-bit, non-prefetchable) [size=4K]
>      Region 2: I/O ports at 2060 [size=32]
>      Capabilities: [c8] Power Management version 2
>              Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
>              Status: D0 PME-Enable- DSel=0 DScale=1 PME-
>      Capabilities: [d0] Message Signalled Interrupts: Mask- 64bit+ Queue=0/0 Enable+
>              Address: 00000000fee0300c  Data: 41b1
>      Kernel driver in use: e1000e
>      Kernel modules: e1000e
>
> I can provide full dmesg and kernel config if desired.
>
> Cheers,
> FJP
>

[PATCH] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Frans Pop]

On Friday 21 August 2009, Danny Feng wrote:
> On 08/20/2009 11:14 PM, Frans Pop wrote:
> > e1000e 0000:00:19.0: pci_enable_pcie_error_reporting failed
>
> I think this messages comes from hardware does not support pcie aer.
> In fact the messages is not fatal and can be ignored. Some drivers
> is silence with pci_enable_pcie_error_reporting ....

Right. The only error returned by the function is -EIO, which looks to be
"not supported". That's really not an issue to shout about in dmesg...

> My patch is just to fix the logic of pcie aer usage ...
>
> > - as the error is non-fatal, should it maybe be changed from dev_err
> >    to dev_info, so that at least it does not show up during a boot
> >    with 'quiet'?
>
> I agree with this.... Maybe some of other drivers needs this too
> (igb,ixgbe)...

Here's a patch that simply drops the message.
Note: the two scsi drivers that call the function (arcmsr and qla2xxx)
also don't check the return code.

Alternatives would be:
- change the message to something like "device does not support advanced
  error reporting (AER)" and make it dev_debug
- suppress the error in case of -EIO, but I'm not sure that makes sense
  as -EIO is the only error returned and you might not want to ignore other
  errors

If you'd prefer the alternatives, let me know and I'll modify the patch.

Cheers,
FJP


From: Frans Pop <elendil@planet.nl>
Subject: net: Don't report an error if devices don't support AER

The only error returned by pci_enable_pcie_error_reporting() is -EIO
which simply means that Advanced Error Reporting is not supported.
There is no need to report that, so remove the error check from e1001e,
igb and ixgbe.

Signed-off-by: Frans Pop <elendil@planet.nl>
    ---

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index fa92a68..42ac8f1 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4983,12 +4983,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 		goto err_pci_reg;
 
 	/* AER (Advanced Error Reporting) hooks */
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		        "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	/* PCI config space info */
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index adb09d3..8e80859 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1232,12 +1232,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_pci_reg;
 
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		        "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	pci_save_state(pdev);
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 77b0381..4e0b523 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5430,12 +5430,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 		goto err_pci_reg;
 	}
 
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		                    "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	pci_save_state(pdev);

Re: [PATCH] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Danny Feng]

On 08/21/2009 02:29 PM, Frans Pop wrote:
> On Friday 21 August 2009, Danny Feng wrote:
>> On 08/20/2009 11:14 PM, Frans Pop wrote:
>>> e1000e 0000:00:19.0: pci_enable_pcie_error_reporting failed
>>
>> I think this messages comes from hardware does not support pcie aer.
>> In fact the messages is not fatal and can be ignored. Some drivers
>> is silence with pci_enable_pcie_error_reporting ....
>
> Right. The only error returned by the function is -EIO, which looks to be
> "not supported". That's really not an issue to shout about in dmesg...
>
>> My patch is just to fix the logic of pcie aer usage ...
>>
>>> - as the error is non-fatal, should it maybe be changed from dev_err
>>>     to dev_info, so that at least it does not show up during a boot
>>>     with 'quiet'?
>>
>> I agree with this.... Maybe some of other drivers needs this too
>> (igb,ixgbe)...
>
> Here's a patch that simply drops the message.
> Note: the two scsi drivers that call the function (arcmsr and qla2xxx)
> also don't check the return code.
>
> Alternatives would be:
> - change the message to something like "device does not support advanced
>    error reporting (AER)" and make it dev_debug
> - suppress the error in case of -EIO, but I'm not sure that makes sense
>    as -EIO is the only error returned and you might not want to ignore other
>    errors
>
> If you'd prefer the alternatives, let me know and I'll modify the patch.
You may also need to silence pci_disable_pcie_error_reporting, otherwise 
rmmod/shutdown, you will get

e1000e 0000:00:19.0: pci_disable_pcie_error_reporting failed

>
> Cheers,
> FJP
>
>
> From: Frans Pop<elendil@planet.nl>
> Subject: net: Don't report an error if devices don't support AER
>
> The only error returned by pci_enable_pcie_error_reporting() is -EIO
> which simply means that Advanced Error Reporting is not supported.
> There is no need to report that, so remove the error check from e1001e,
> igb and ixgbe.
>
> Signed-off-by: Frans Pop<elendil@planet.nl>
>      ---
>
> diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
> index fa92a68..42ac8f1 100644
> --- a/drivers/net/e1000e/netdev.c
> +++ b/drivers/net/e1000e/netdev.c
> @@ -4983,12 +4983,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
>   		goto err_pci_reg;
>
>   	/* AER (Advanced Error Reporting) hooks */
> -	err = pci_enable_pcie_error_reporting(pdev);
> -	if (err) {
> -		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
> -		        "0x%x\n", err);
> -		/* non-fatal, continue */
> -	}
> +	pci_enable_pcie_error_reporting(pdev);
>
>   	pci_set_master(pdev);
>   	/* PCI config space info */
> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> index adb09d3..8e80859 100644
> --- a/drivers/net/igb/igb_main.c
> +++ b/drivers/net/igb/igb_main.c
> @@ -1232,12 +1232,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
>   	if (err)
>   		goto err_pci_reg;
>
> -	err = pci_enable_pcie_error_reporting(pdev);
> -	if (err) {
> -		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
> -		        "0x%x\n", err);
> -		/* non-fatal, continue */
> -	}
> +	pci_enable_pcie_error_reporting(pdev);
>
>   	pci_set_master(pdev);
>   	pci_save_state(pdev);
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index 77b0381..4e0b523 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -5430,12 +5430,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>   		goto err_pci_reg;
>   	}
>
> -	err = pci_enable_pcie_error_reporting(pdev);
> -	if (err) {
> -		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
> -		                    "0x%x\n", err);
> -		/* non-fatal, continue */
> -	}
> +	pci_enable_pcie_error_reporting(pdev);
>
>   	pci_set_master(pdev);
>   	pci_save_state(pdev);
>

[PATCH,v2] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Frans Pop]

On Friday 21 August 2009, Danny Feng wrote:
> You may also need to silence pci_disable_pcie_error_reporting,
> otherwise rmmod/shutdown, you will get
>
> e1000e 0000:00:19.0: pci_disable_pcie_error_reporting failed

Yes, thanks. Exactly the same thing there. Updated patch below.


From: Frans Pop <elendil@planet.nl>
Subject: net: Don't report an error if devices don't support AER

The only error returned by pci_{en,dis}able_pcie_error_reporting() is
-EIO which simply means that Advanced Error Reporting is not supported.
There is no need to report that, so remove the error check from e1001e,
igb and ixgbe.

Signed-off-by: Frans Pop <elendil@planet.nl>

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index fa92a68..d67798f 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4983,12 +4983,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 		goto err_pci_reg;
 
 	/* AER (Advanced Error Reporting) hooks */
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		        "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	/* PCI config space info */
@@ -5301,9 +5296,6 @@ static void __devexit e1000_remove(struct pci_dev *pdev)
 
 	/* AER disable */
 	err = pci_disable_pcie_error_reporting(pdev);
-	if (err)
-		dev_err(&pdev->dev,
-		        "pci_disable_pcie_error_reporting failed 0x%x\n", err);
 
 	pci_disable_device(pdev);
 }
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index adb09d3..1533d6f 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1232,12 +1232,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_pci_reg;
 
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		        "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	pci_save_state(pdev);
@@ -1668,9 +1663,6 @@ static void __devexit igb_remove(struct pci_dev *pdev)
 	free_netdev(netdev);
 
 	err = pci_disable_pcie_error_reporting(pdev);
-	if (err)
-		dev_err(&pdev->dev,
-		        "pci_disable_pcie_error_reporting failed 0x%x\n", err);
 
 	pci_disable_device(pdev);
 }
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 77b0381..777556d 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5430,12 +5430,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 		goto err_pci_reg;
 	}
 
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		                    "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	pci_save_state(pdev);
@@ -5795,9 +5790,6 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
 	free_netdev(netdev);
 
 	err = pci_disable_pcie_error_reporting(pdev);
-	if (err)
-		dev_err(&pdev->dev,
-		        "pci_disable_pcie_error_reporting failed 0x%x\n", err);
 
 	pci_disable_device(pdev);
 }

Re: [PATCH,v2] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[David Miller]

From: Frans Pop <elendil@planet.nl>
Date: Fri, 21 Aug 2009 08:48:38 +0200

> From: Frans Pop <elendil@planet.nl>
> Subject: net: Don't report an error if devices don't support AER
> 
> The only error returned by pci_{en,dis}able_pcie_error_reporting() is
> -EIO which simply means that Advanced Error Reporting is not supported.
> There is no need to report that, so remove the error check from e1001e,
> igb and ixgbe.
> 
> Signed-off-by: Frans Pop <elendil@planet.nl>

Intel folks, just let me know what you want me to do with this,
if anything.

Thanks.

Re: [PATCH,v2] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Jeff Kirsher]

On Thu, Aug 20, 2009 at 23:52, David Miller<davem@davemloft.net> wrote:
> From: Frans Pop <elendil@planet.nl>
> Date: Fri, 21 Aug 2009 08:48:38 +0200
>
>> From: Frans Pop <elendil@planet.nl>
>> Subject: net: Don't report an error if devices don't support AER
>>
>> The only error returned by pci_{en,dis}able_pcie_error_reporting() is
>> -EIO which simply means that Advanced Error Reporting is not supported.
>> There is no need to report that, so remove the error check from e1001e,
>> igb and ixgbe.
>>
>> Signed-off-by: Frans Pop <elendil@planet.nl>
>
> Intel folks, just let me know what you want me to do with this,
> if anything.
>
> Thanks.
> --

I will add it to my queue of patches.  I would like to have Emil touch
test this patch at least before pushing the patch to Dave.

-- 
Cheers,
Jeff

Re: [PATCH,v2] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Jens Rosenboom]

On Fri, 2009-08-21 at 08:48 +0200, Frans Pop wrote:
[...]
> @@ -5301,9 +5296,6 @@ static void __devexit e1000_remove(struct pci_dev *pdev)
>  
>  	/* AER disable */
>  	err = pci_disable_pcie_error_reporting(pdev);
> -	if (err)
> -		dev_err(&pdev->dev,
> -		        "pci_disable_pcie_error_reporting failed 0x%x\n", err);
>  
>  	pci_disable_device(pdev);
>  }

While you're at it, why don't you also remove the "err =" here, which
allows you to drop the variable from that function completely? This
applies to all three *_remove() functions.

Re: [PATCH,v3] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Frans Pop]

On Friday 21 August 2009, Jens Rosenboom wrote:
> While you're at it, why don't you also remove the "err =" here, which
> allows you to drop the variable from that function completely? This
> applies to all three *_remove() functions.

Argh! Sloppy. Shouldn't do this kind of thing before breakfast :-/
Thanks. Hope this version will pass muster.


From: Frans Pop <elendil@planet.nl>
Subject: net: Don't report an error if devices don't support AER

The only error returned by pci_{en,dis}able_pcie_error_reporting() is
-EIO which simply means that Advanced Error Reporting is not supported.
There is no need to report that, so remove the error check from e1001e,
igb and ixgbe.

Signed-off-by: Frans Pop <elendil@planet.nl>

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index fa92a68..2d421da 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4983,12 +4983,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 		goto err_pci_reg;
 
 	/* AER (Advanced Error Reporting) hooks */
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		        "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	/* PCI config space info */
@@ -5264,7 +5259,6 @@ static void __devexit e1000_remove(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct e1000_adapter *adapter = netdev_priv(netdev);
-	int err;
 
 	/*
 	 * flush_scheduled work may reschedule our watchdog task, so
@@ -5300,10 +5294,7 @@ static void __devexit e1000_remove(struct pci_dev *pdev)
 	free_netdev(netdev);
 
 	/* AER disable */
-	err = pci_disable_pcie_error_reporting(pdev);
-	if (err)
-		dev_err(&pdev->dev,
-		        "pci_disable_pcie_error_reporting failed 0x%x\n", err);
+	pci_disable_pcie_error_reporting(pdev);
 
 	pci_disable_device(pdev);
 }
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index adb09d3..f4393df 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1232,12 +1232,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_pci_reg;
 
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		        "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	pci_save_state(pdev);
@@ -1613,7 +1608,6 @@ static void __devexit igb_remove(struct pci_dev *pdev)
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
-	int err;
 
 	/* flush_scheduled work may reschedule our watchdog task, so
 	 * explicitly disable watchdog tasks from being rescheduled  */
@@ -1667,10 +1661,7 @@ static void __devexit igb_remove(struct pci_dev *pdev)
 
 	free_netdev(netdev);
 
-	err = pci_disable_pcie_error_reporting(pdev);
-	if (err)
-		dev_err(&pdev->dev,
-		        "pci_disable_pcie_error_reporting failed 0x%x\n", err);
+	pci_disable_pcie_error_reporting(pdev);
 
 	pci_disable_device(pdev);
 }
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 77b0381..6520496 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5430,12 +5430,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 		goto err_pci_reg;
 	}
 
-	err = pci_enable_pcie_error_reporting(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
-		                    "0x%x\n", err);
-		/* non-fatal, continue */
-	}
+	pci_enable_pcie_error_reporting(pdev);
 
 	pci_set_master(pdev);
 	pci_save_state(pdev);
@@ -5743,7 +5738,6 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	int err;
 
 	set_bit(__IXGBE_DOWN, &adapter->state);
 	/* clear the module not found bit to make sure the worker won't
@@ -5794,10 +5788,7 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
 
 	free_netdev(netdev);
 
-	err = pci_disable_pcie_error_reporting(pdev);
-	if (err)
-		dev_err(&pdev->dev,
-		        "pci_disable_pcie_error_reporting failed 0x%x\n", err);
+	pci_disable_pcie_error_reporting(pdev);
 
 	pci_disable_device(pdev);
 }

Re: [PATCH,v2] Re: e1000e: why does pci_enable_pcie_error_reporting() fail on my hp2510p?

[Jeff Kirsher]

On Thu, Aug 20, 2009 at 23:48, Frans Pop<elendil@planet.nl> wrote:
> On Friday 21 August 2009, Danny Feng wrote:
>> You may also need to silence pci_disable_pcie_error_reporting,
>> otherwise rmmod/shutdown, you will get
>>
>> e1000e 0000:00:19.0: pci_disable_pcie_error_reporting failed
>
> Yes, thanks. Exactly the same thing there. Updated patch below.
>
>
> From: Frans Pop <elendil@planet.nl>
> Subject: net: Don't report an error if devices don't support AER
>
> The only error returned by pci_{en,dis}able_pcie_error_reporting() is
> -EIO which simply means that Advanced Error Reporting is not supported.
> There is no need to report that, so remove the error check from e1001e,
> igb and ixgbe.
>
> Signed-off-by: Frans Pop <elendil@planet.nl>
>
> diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
> index fa92a68..d67798f 100644
> --- a/drivers/net/e1000e/netdev.c
> +++ b/drivers/net/e1000e/netdev.c
> @@ -4983,12 +4983,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
>                goto err_pci_reg;
>
>        /* AER (Advanced Error Reporting) hooks */
> -       err = pci_enable_pcie_error_reporting(pdev);
> -       if (err) {
> -               dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
> -                       "0x%x\n", err);
> -               /* non-fatal, continue */
> -       }
> +       pci_enable_pcie_error_reporting(pdev);
>
>        pci_set_master(pdev);
>        /* PCI config space info */
> @@ -5301,9 +5296,6 @@ static void __devexit e1000_remove(struct pci_dev *pdev)
>
>        /* AER disable */
>        err = pci_disable_pcie_error_reporting(pdev);
> -       if (err)
> -               dev_err(&pdev->dev,
> -                       "pci_disable_pcie_error_reporting failed 0x%x\n", err);
>
>        pci_disable_device(pdev);
>  }
> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> index adb09d3..1533d6f 100644
> --- a/drivers/net/igb/igb_main.c
> +++ b/drivers/net/igb/igb_main.c
> @@ -1232,12 +1232,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
>        if (err)
>                goto err_pci_reg;
>
> -       err = pci_enable_pcie_error_reporting(pdev);
> -       if (err) {
> -               dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
> -                       "0x%x\n", err);
> -               /* non-fatal, continue */
> -       }
> +       pci_enable_pcie_error_reporting(pdev);
>
>        pci_set_master(pdev);
>        pci_save_state(pdev);
> @@ -1668,9 +1663,6 @@ static void __devexit igb_remove(struct pci_dev *pdev)
>        free_netdev(netdev);
>
>        err = pci_disable_pcie_error_reporting(pdev);
> -       if (err)
> -               dev_err(&pdev->dev,
> -                       "pci_disable_pcie_error_reporting failed 0x%x\n", err);
>
>        pci_disable_device(pdev);
>  }
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index 77b0381..777556d 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -5430,12 +5430,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>                goto err_pci_reg;
>        }
>
> -       err = pci_enable_pcie_error_reporting(pdev);
> -       if (err) {
> -               dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
> -                                   "0x%x\n", err);
> -               /* non-fatal, continue */
> -       }
> +       pci_enable_pcie_error_reporting(pdev);
>
>        pci_set_master(pdev);
>        pci_save_state(pdev);
> @@ -5795,9 +5790,6 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
>        free_netdev(netdev);
>
>        err = pci_disable_pcie_error_reporting(pdev);
> -       if (err)
> -               dev_err(&pdev->dev,
> -                       "pci_disable_pcie_error_reporting failed 0x%x\n", err);
>
>        pci_disable_device(pdev);
>  }

Patch looks fine

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>