pci: Fix e100 interrupt quirk

PATCH In review Valentine Barshak 3 participants 6 messages Sep 18, 2007 – Sep 20, 2007

Subsystems (3)

Networking Network drivers PCI

lore

Share & tools

lore

Valentine Barshak

author

2007-09-18 11:55 UTC #0 lore raw

PATCH 1/1 pci: Fix e100 interrupt quirk

PCI memory space may have a 64-bit offset on some architectures (for example, PowerPC 440) and the actual PCI memory address has to fixed up (an offset to PCI mem space shuld be added) before remapping. So, pci_iomap should be used instead of reading and remapping PCI BAR directly. This has been tested on Sequoia PowerPC 440EPx board.

Signed-off-by Valentine Barshak <[email protected]>

---

drivers/pci/quirks.c +9 -6

@@ -1444,9 +1444,9 @@
 static void __devinit quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command;
-	u32 bar;
 	u8 __iomem *csr;
 	u8 cmd_hi;
+	int rc;
 
 	switch (dev->device) {
 	/* PCI IDs taken from drivers/net/e100.c */

@@ -1476,16 +1476,17 @@
 	 * re-enable them when it's ready.
 	 */
 	pci_read_config_word(dev, PCI_COMMAND, &command);
-	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0, &bar);
 
-	if (!(command & PCI_COMMAND_MEMORY) || !bar)
+	rc = pci_request_region(dev, 0, "e100_quirk");
+
+	if (!(command & PCI_COMMAND_MEMORY) || (rc < 0))
 		return;
 
-	csr = ioremap(bar, 8);
+	csr = pci_iomap(dev, 0, 8);
 	if (!csr) {
 		printk(KERN_WARNING "PCI: Can't map %s e100 registers\n",
 			pci_name(dev));
-		return;
+		goto e100_quirk_exit;
 	}
 
 	cmd_hi = readb(csr + 3);

@@ -1495,7 +1496,9 @@
 		writeb(1, csr + 3);
 	}
 
-	iounmap(csr);
+	pci_iounmap(dev, csr);
+e100_quirk_exit:
+	pci_release_region(dev, 0);
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, quirk_e100_interrupt);

Jarek Poplawski

2007-09-19 10:41 UTC #1 lore raw

Quoted 24 lines · Valentine Barshak

PCI memory space may have a 64-bit offset on some architectures (for example, PowerPC 440) and the actual PCI memory address has to fixed up (an offset to PCI mem space shuld be added) before remapping. So, pci_iomap should be used instead of reading and remapping PCI BAR directly. This has been tested on Sequoia PowerPC 440EPx board.
Signed-off-by Valentine Barshak <[email protected]>
---
drivers/pci/quirks.c +1 -1
@@ -1444,9 +1444,9 @@
 static void __devinit quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command;
-	u32 bar;
 	u8 __iomem *csr;
 	u8 cmd_hi;
+	int rc;
 
 	switch (dev->device) {
 	/* PCI IDs taken from drivers/net/e100.c */

drivers/pci/quirks.c line 1483

Quoted 10 lines

drivers/pci/quirks.c +3 -2

@@ -1476,16 +1476,17 @@
 	 * re-enable them when it's ready.
 	 */
 	pci_read_config_word(dev, PCI_COMMAND, &command);
-	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0, &bar);
 
-	if (!(command & PCI_COMMAND_MEMORY) || !bar)
+	rc = pci_request_region(dev, 0, "e100_quirk");
+
+	if (!(command & PCI_COMMAND_MEMORY) || (rc < 0))
 		return;

I didn't look at this too much, but isn't something like:

		if (rc >=  0)
			goto e100_quirk_exit;

needed before this return?

Regards,
Jarek P.

Valentine Barshak

author

2007-09-19 11:00 UTC #2 lore raw

Quoted 37 lines · Jarek Poplawski

Quoted 24 lines · Valentine Barshak
PCI memory space may have a 64-bit offset on some architectures (for example, PowerPC 440) and the actual PCI memory address has to fixed up (an offset to PCI mem space shuld be added) before remapping. So, pci_iomap should be used instead of reading and remapping PCI BAR directly. This has been tested on Sequoia PowerPC 440EPx board.
Signed-off-by Valentine Barshak <[email protected]>
---
drivers/pci/quirks.c +1 -1
@@ -1444,9 +1444,9 @@
 static void __devinit quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command;
-	u32 bar;
 	u8 __iomem *csr;
 	u8 cmd_hi;
+	int rc;
 
 	switch (dev->device) {
 	/* PCI IDs taken from drivers/net/e100.c */
drivers/pci/quirks.c line 1483
Quoted 10 lines
drivers/pci/quirks.c +3 -2
@@ -1476,16 +1476,17 @@
 	 * re-enable them when it's ready.
 	 */
 	pci_read_config_word(dev, PCI_COMMAND, &command);
-	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0, &bar);
 
-	if (!(command & PCI_COMMAND_MEMORY) || !bar)
+	rc = pci_request_region(dev, 0, "e100_quirk");
+
+	if (!(command & PCI_COMMAND_MEMORY) || (rc < 0))
 		return;
I didn't look at this too much, but isn't something like:

Quoted 3 lines

		if (rc >=  0)
			goto e100_quirk_exit;
needed before this return?

I'll split these 2 checks and submit new patch in a minute. Thanks.

Quoted 3 lines

Regards,
Jarek P.

Valentine Barshak

author

2007-09-19 12:38 UTC #3 lore raw

PATCH 1/1 pci: Fix e100 interrupt quirk

In reply to Jarek Poplawski

Signed-off-by Valentine Barshak <[email protected]>

---

drivers/pci/quirks.c +9 -6

diff -ruN linux-2.6.orig/drivers/pci/quirks.c linux-2.6/drivers/pci/quirks.c
@@ -1444,7 +1444,6 @@
 static void __devinit quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command;
-	u32 bar;
 	u8 __iomem *csr;
 	u8 cmd_hi;

@@ -1476,16 +1475,18 @@
 	 * re-enable them when it's ready.
 	 */
 	pci_read_config_word(dev, PCI_COMMAND, &command);
-	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0, &bar);
 
-	if (!(command & PCI_COMMAND_MEMORY) || !bar)
+	if (!(command & PCI_COMMAND_MEMORY))
 		return;
 
-	csr = ioremap(bar, 8);
+	if (pci_request_region(dev, 0, "e100_quirk"))
+		return;
+
+	csr = pci_iomap(dev, 0, 8);
 	if (!csr) {
 		printk(KERN_WARNING "PCI: Can't map %s e100 registers\n",
 			pci_name(dev));
-		return;
+		goto e100_quirk_exit;
 	}
 
 	cmd_hi = readb(csr + 3);

@@ -1495,7 +1496,9 @@
 		writeb(1, csr + 3);
 	}
 
-	iounmap(csr);
+	pci_iounmap(dev, csr);
+e100_quirk_exit:
+	pci_release_region(dev, 0);
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, quirk_e100_interrupt);

Andrew Morton

2007-09-20 06:37 UTC #4 lore raw

In reply to Valentine Barshak

Quoted 24 lines · On Tue, 18 Sep 2007 15:17:37 +0400 Valentine Barshak <[email protected]>…

PCI memory space may have a 64-bit offset on some architectures (for example, PowerPC 440) and the actual PCI memory address has to fixed up (an offset to PCI mem space shuld be added) before remapping. So, pci_iomap should be used instead of reading and remapping PCI BAR directly. This has been tested on Sequoia PowerPC 440EPx board.
Signed-off-by Valentine Barshak <[email protected]>
---
drivers/pci/quirks.c +1 -1
@@ -1444,9 +1444,9 @@
 static void __devinit quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command;
-	u32 bar;
 	u8 __iomem *csr;
 	u8 cmd_hi;
+	int rc;
 
 	switch (dev->device) {
 	/* PCI IDs taken from drivers/net/e100.c */

drivers/pci/quirks.c line 1483

Quoted 10 lines

drivers/pci/quirks.c +3 -2

@@ -1476,16 +1476,17 @@
 	 * re-enable them when it's ready.
 	 */
 	pci_read_config_word(dev, PCI_COMMAND, &command);
-	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0, &bar);
 
-	if (!(command & PCI_COMMAND_MEMORY) || !bar)
+	rc = pci_request_region(dev, 0, "e100_quirk");
+
+	if (!(command & PCI_COMMAND_MEMORY) || (rc < 0))
 		return;

Really? So if pci_request_region() failed and !(command & PCI_COMMAND_MEMORY), we leak the region? So the next call to this function will fail?

Quoted 26 lines

drivers/pci/quirks.c +5 -4

-	csr = ioremap(bar, 8);
+	csr = pci_iomap(dev, 0, 8);
 	if (!csr) {
 		printk(KERN_WARNING "PCI: Can't map %s e100 registers\n",
 			pci_name(dev));
-		return;
+		goto e100_quirk_exit;
 	}
 
 	cmd_hi = readb(csr + 3);

@@ -1495,7 +1496,9 @@
 		writeb(1, csr + 3);
 	}
 
-	iounmap(csr);
+	pci_iounmap(dev, csr);
+e100_quirk_exit:
+	pci_release_region(dev, 0);
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, quirk_e100_interrupt);
 
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Valentine Barshak

author

2007-09-20 11:34 UTC #5 lore raw

Quoted 31 lines · Andrew Morton

Quoted 29 lines · On Tue, 18 Sep 2007 15:17:37 +0400 Valentine Barshak <[email protected]>…
PCI memory space may have a 64-bit offset on some architectures (for example, PowerPC 440) and the actual PCI memory address has to fixed up (an offset to PCI mem space shuld be added) before remapping. So, pci_iomap should be used instead of reading and remapping PCI BAR directly. This has been tested on Sequoia PowerPC 440EPx board.
Signed-off-by Valentine Barshak <[email protected]>
---
drivers/pci/quirks.c +1 -2
@@ -1444,9 +1444,9 @@
 static void __devinit quirk_e100_interrupt(struct pci_dev *dev)
 {
 	u16 command;
-	u32 bar;
 	u8 __iomem *csr;
 	u8 cmd_hi;
+	int rc;
 
 	switch (dev->device) {
 	/* PCI IDs taken from drivers/net/e100.c */
@@ -1476,16 +1476,17 @@
 	 * re-enable them when it's ready.
 	 */
 	pci_read_config_word(dev, PCI_COMMAND, &command);
-	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0, &bar);
 

Quoted 9 lines

drivers/pci/quirks.c line 1483
Quoted 5 lines
drivers/pci/quirks.c +3 -1
-	if (!(command & PCI_COMMAND_MEMORY) || !bar)
+	rc = pci_request_region(dev, 0, "e100_quirk");
+
+	if (!(command & PCI_COMMAND_MEMORY) || (rc < 0))
 		return;
Really? So if pci_request_region() failed and !(command & PCI_COMMAND_MEMORY), we leak the region? So the next call to this function will fail?

I've split command and request region checks and submitted new patch:
http://lkml.org/lkml/2007/9/19/106
Please, take a look,
Thanks,
Valentine.

Quoted 27 lines

Quoted 27 lines
drivers/pci/quirks.c +5 -4
-	csr = ioremap(bar, 8);
+	csr = pci_iomap(dev, 0, 8);
 	if (!csr) {
 		printk(KERN_WARNING "PCI: Can't map %s e100 registers\n",
 			pci_name(dev));
-		return;
+		goto e100_quirk_exit;
 	}
 
 	cmd_hi = readb(csr + 3);
@@ -1495,7 +1496,9 @@
 		writeb(1, csr + 3);
 	}
 
-	iounmap(csr);
+	pci_iounmap(dev, csr);
+e100_quirk_exit:
+	pci_release_region(dev, 0);
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, quirk_e100_interrupt);
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/