Skip to contentSkip to conversation

myri10ge - Always do a dummy RDMA after loading the firmware

PATCH In review Brice Goglin 2 participants 4 messages Jul 21, 2006 – Jul 22, 2006 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH 1/2 myri10ge - Always do a dummy RDMA after loading the firmware

Always do a dummy RDMA after loading the firmware to work around buggy PCIe chipsets which do not implement resending properly. This is so cheap as to be almost free, and should never have been conditional on the tx boundary != 4096.

---
 drivers/net/myri10ge/myri10ge.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/myri10ge/myri10ge.c +1 -1
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
@@ -620,7 +620,7 @@
 		return -ENXIO;
 	}
 	dev_info(&mgp->pdev->dev, "handoff confirmed\n");
-	myri10ge_dummy_rdma(mgp, mgp->tx.boundary != 4096);
+	myri10ge_dummy_rdma(mgp, 1);
 
 	return 0;
 }
#2 lore raw

PATCH 2/2 myri10ge - Write the firmware in 256-bytes chunks

In reply to a message not in the archive

When writing the firmware to the NIC, the FIFO is 256-bytes long, so we use 256-bytes chunks and a read to wait until the previous write is done.

---
 drivers/net/myri10ge/myri10ge.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/net/myri10ge/myri10ge.c +8 -12
Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================
@@ -448,6 +448,7 @@
 	struct mcp_gen_header *hdr;
 	size_t hdr_offset;
 	int status;
+	unsigned i;
 
 	if ((status = request_firmware(&fw, mgp->fw_name, dev)) < 0) {
 		dev_err(dev, "Unable to load %s firmware image via hotplug\n",
@@ -479,18 +480,13 @@
 		goto abort_with_fw;
 
 	crc = crc32(~0, fw->data, fw->size);
-	if (mgp->tx.boundary == 2048) {
-		/* Avoid PCI burst on chipset with unaligned completions. */
-		int i;
-		__iomem u32 *ptr = (__iomem u32 *) (mgp->sram +
-						    MYRI10GE_FW_OFFSET);
-		for (i = 0; i < fw->size / 4; i++) {
-			__raw_writel(((u32 *) fw->data)[i], ptr + i);
-			wmb();
-		}
-	} else {
-		myri10ge_pio_copy(mgp->sram + MYRI10GE_FW_OFFSET, fw->data,
-				  fw->size);
+	for (i = 0; i < fw->size; i += 256) {
+		myri10ge_pio_copy(mgp->sram + MYRI10GE_FW_OFFSET + i,
+				  fw->data + i,
+				  min(256U, (unsigned)(fw->size - i)));
+		mb();
+		readb(mgp->sram);
+		mb();
 	}
 	/* corruption checking is good for parity recovery and buggy chipset */
 	memcpy_fromio(fw->data, mgp->sram + MYRI10GE_FW_OFFSET, fw->size);
#3 lore raw

Re: [patch 2/2] myri10ge - Write the firmware in 256-bytes chunks

Quoted 34 lines · Brice Goglin

When writing the firmware to the NIC, the FIFO is 256-bytes long, so we use 256-bytes chunks and a read to wait until the previous write is done.

---
 drivers/net/myri10ge/myri10ge.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

Index: linux-mm/drivers/net/myri10ge/myri10ge.c
===================================================================

drivers/net/myri10ge/myri10ge.c +1 -9
@@ -448,6 +448,7 @@
 	struct mcp_gen_header *hdr;
 	size_t hdr_offset;
 	int status;
+	unsigned i;
 
 	if ((status = request_firmware(&fw, mgp->fw_name, dev)) < 0) {
 		dev_err(dev, "Unable to load %s firmware image via hotplug\n",
@@ -479,18 +480,13 @@
 		goto abort_with_fw;
 
 	crc = crc32(~0, fw->data, fw->size);
-	if (mgp->tx.boundary == 2048) {
-		/* Avoid PCI burst on chipset with unaligned completions. */
-		int i;
-		__iomem u32 *ptr = (__iomem u32 *) (mgp->sram +
-						    MYRI10GE_FW_OFFSET);
-		for (i = 0; i < fw->size / 4; i++) {
-			__raw_writel(((u32 *) fw->data)[i], ptr + i);
-			wmb();
-		}

drivers/net/myri10ge/myri10ge.c line 489

Quoted 10 lines
drivers/net/myri10ge/myri10ge.c +7 -3
-	} else {
-		myri10ge_pio_copy(mgp->sram + MYRI10GE_FW_OFFSET, fw->data,
-				  fw->size);
+	for (i = 0; i < fw->size; i += 256) {
+		myri10ge_pio_copy(mgp->sram + MYRI10GE_FW_OFFSET + i,
+				  fw->data + i,
+				  min(256U, (unsigned)(fw->size - i)));
+		mb();
+		readb(mgp->sram);
+		mb();

Why two mb() here?
I would say actually none is needed.
The readb fully synchronizes the previous writes on bus level (and so on CPU level, too).

Signature · 1 line
Greetings Michael.
#4 lore raw

Re: [patch 2/2] myri10ge - Write the firmware in 256-bytes chunks

Quoted 14 lines · Michael Buesch

drivers/net/myri10ge/myri10ge.c line 489

Quoted 7 lines · Brice Goglin
drivers/net/myri10ge/myri10ge.c +6 -0
+		myri10ge_pio_copy(mgp->sram + MYRI10GE_FW_OFFSET + i,
+				  fw->data + i,
+				  min(256U, (unsigned)(fw->size - i)));
+		mb();
+		readb(mgp->sram);
+		mb();

Why two mb() here?
I would say actually none is needed.
The readb fully synchronizes the previous writes on bus level (and so on CPU level, too)

At least on i386 and x86-64, readb does not pass an explicit memory barrier to the processor. We use a "weak-ordering" write-combining mapping, so the previous PIO-write accesses and the readb are not automatically serialized. So in absence of the first mb(), and because "WC" read can definitely pass "WC" write, the readb (whose purpose is to guarantee the previous write is finished) could actually be complete long before those have even started (especially because WC buffers can stay in the processor a long time before being flushed in absence of synchronization instructions).

The second mb() is indeed probably superfluous and could be removed. The WC semantics are not weak enough to allow WC writes to pass WC read (given it was only one instruction in init code, we did the lazy thing).

Brice