tg3: tg3_disable_ints using uninitialized mailbox value to disable interrupts

PATCH In review Prashant Sreedharan 4 participants 4 messages Dec 20, 2014 – Dec 22, 2014

Subsystems (4)

Networking Network drivers PCI Broadcom TG3 gigabit Ethernet driver

lore

Share & tools

lore

Prashant Sreedharan

author

2014-12-20 20:16 UTC #0 lore raw

PATCH 1/1 tg3: tg3_disable_ints using uninitialized mailbox value to disable interrupts

During driver load in tg3_init_one, if the driver detects DMA activity before intializing the chip tg3_halt is called. As part of tg3_halt interrupts are disabled using routine tg3_disable_ints. This routine was using mailbox value which was not initialized (default value is 0). As a result driver was writing 0x00000001 to pci config space register 0, which is the vendor id / device id.

This driver bug was exposed because of the commit a7877b17a667 (PCI: Check only the Vendor ID to identify Configuration Request Retry). Also this issue is only seen in older generation chipsets like 5722 because config space write to offset 0 from driver is possible. The newer generation chips ignore writes to offset 0. Also without commit a7877b17a667, for these older chips when a GRC reset is issued the Bootcode would reprogram the vendor id/device id, which is the reason this bug was masked earlier.

Fixed by initializing the interrupt mailbox registers before calling tg3_halt.

Please queue for -stable.

Reported-by Nils Holland <[email protected]>

Reported-by Marcelo Ricardo Leitner <[email protected]>

Signed-off-by Prashant Sreedharan <[email protected]>

Signed-off-by Michael Chan <[email protected]>

---
 drivers/net/ethernet/broadcom/tg3.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

drivers/net/ethernet/broadcom/tg3.c +17 ~~-17~~

index bb48a61..553dcd8 100644
@@ -17800,23 +17800,6 @@ static int tg3_init_one(struct pci_dev *pdev,
 		goto err_out_apeunmap;
 	}
 
-	/*
-	 * Reset chip in case UNDI or EFI driver did not shutdown
-	 * DMA self test will enable WDMAC and we'll see (spurious)
-	 * pending DMA on the PCI bus at that point.
-	 */
-	if ((tr32(HOSTCC_MODE) & HOSTCC_MODE_ENABLE) ||
-	    (tr32(WDMAC_MODE) & WDMAC_MODE_ENABLE)) {
-		tw32(MEMARB_MODE, MEMARB_MODE_ENABLE);
-		tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
-	}
-
-	err = tg3_test_dma(tp);
-	if (err) {
-		dev_err(&pdev->dev, "DMA engine test failed, aborting\n");
-		goto err_out_apeunmap;
-	}
-
 	intmbx = MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW;
 	rcvmbx = MAILBOX_RCVRET_CON_IDX_0 + TG3_64BIT_REG_LOW;
 	sndmbx = MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW;

@@ -17861,6 +17844,23 @@ static int tg3_init_one(struct pci_dev *pdev,
 			sndmbx += 0xc;
 	}
 
+	/*
+	 * Reset chip in case UNDI or EFI driver did not shutdown
+	 * DMA self test will enable WDMAC and we'll see (spurious)
+	 * pending DMA on the PCI bus at that point.
+	 */
+	if ((tr32(HOSTCC_MODE) & HOSTCC_MODE_ENABLE) ||
+	    (tr32(WDMAC_MODE) & WDMAC_MODE_ENABLE)) {
+		tw32(MEMARB_MODE, MEMARB_MODE_ENABLE);
+		tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
+	}
+
+	err = tg3_test_dma(tp);
+	if (err) {
+		dev_err(&pdev->dev, "DMA engine test failed, aborting\n");
+		goto err_out_apeunmap;
+	}
+
 	tg3_init_coal(tp);
 
 	pci_set_drvdata(pdev, dev);

Signature · 1 line

1.7.1

Nils Holland

2014-12-20 22:16 UTC #1 lore raw

Quoted 9 lines · Prashant Sreedharan

This driver bug was exposed because of the commit a7877b17a667 (PCI: Check only the Vendor ID to identify Configuration Request Retry). Also this issue is only seen in older generation chipsets like 5722 because config space write to offset 0 from driver is possible.
Fixed by initializing the interrupt mailbox registers before calling tg3_halt.
Please queue for -stable.

I gave this patch a try and can confirm what was to be expected: It fixes the issue and the network interface is once again working properly on my system. Thus, I guess the issue is adequately solved.

Thanks to everyone involved, especially to Marcelo for additional debugging and the guys at Broadcom for the quick fix!

Greetings,
Nils

Marcelo Ricardo Leitner

2014-12-22 14:06 UTC #2 lore raw

Quoted 15 lines · Nils Holland

Quoted 9 lines · Prashant Sreedharan
This driver bug was exposed because of the commit a7877b17a667 (PCI: Check only the Vendor ID to identify Configuration Request Retry). Also this issue is only seen in older generation chipsets like 5722 because config space write to offset 0 from driver is possible.
Fixed by initializing the interrupt mailbox registers before calling tg3_halt.
Please queue for -stable.
I gave this patch a try and can confirm what was to be expected: It fixes the issue and the network interface is once again working properly on my system. Thus, I guess the issue is adequately solved.

Quoted 5 lines

Thanks to everyone involved, especially to Marcelo for additional debugging and the guys at Broadcom for the quick fix!
Greetings,
Nils

Same here, it works again.

Thanks!
Marcelo

David Miller

maintainer

2014-12-22 21:13 UTC #3 lore raw

In reply to Prashant Sreedharan

From: Prashant Sreedharan <[email protected]>
Date: Sat, 20 Dec 2014 12:16:17 -0800

Quoted 14 lines

During driver load in tg3_init_one, if the driver detects DMA activity before intializing the chip tg3_halt is called. As part of tg3_halt interrupts are disabled using routine tg3_disable_ints. This routine was using mailbox value which was not initialized (default value is 0). As a result driver was writing 0x00000001 to pci config space register 0, which is the vendor id / device id.
This driver bug was exposed because of the commit a7877b17a667 (PCI: Check only the Vendor ID to identify Configuration Request Retry). Also this issue is only seen in older generation chipsets like 5722 because config space write to offset 0 from driver is possible. The newer generation chips ignore writes to offset 0. Also without commit a7877b17a667, for these older chips when a GRC reset is issued the Bootcode would reprogram the vendor id/device id, which is the reason this bug was masked earlier.

Quoted 8 lines

Fixed by initializing the interrupt mailbox registers before calling tg3_halt.
Please queue for -stable.
Reported-by Nils Holland <[email protected]>
Reported-by Marcelo Ricardo Leitner <[email protected]>
Signed-off-by Prashant Sreedharan <[email protected]>
Signed-off-by Michael Chan <[email protected]>

Applied and queued up for -stable, thanks.