Skip to contentSkip to conversation

tg3: Refresh hw index in tg3_rx()

PATCH In review Michael Chan 2 participants 4 messages Apr 25, 2005 – May 5, 2005 5 messages not in the archive

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 1114463061.4917.34.camel@rh4

Michael Chan

author
#1 lore raw

PATCH 2/3 tg3: Refresh hw index in tg3_rx()

This patch refreshes the hw rx producer in tg3_rx() so that additional work posted by the hardware can be processed.

Signed-off-by Michael Chan <[email protected]>
drivers/net/tg3.c +6 -0
diff -Nru c/drivers/net/tg3.c d/drivers/net/tg3.c
@@ -2802,6 +2802,12 @@
 next_pkt_nopost:
 		sw_idx++;
 		sw_idx %= TG3_RX_RCB_RING_SIZE(tp);
+
+		/* Refresh hw_idx to see if there is new work */
+		if (sw_idx == hw_idx) {
+			hw_idx = tp->hw_status->idx[0].rx_producer;
+			rmb();
+		}
 	}
 
 	/* ACK the status ring. */

A message that is not in the archive (sent off-list or not yet received): 1114464194.4917.52.camel@rh4

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

author
#4 lore raw

Re: [PATCH 2.6.12-rc2 3/3] tg3: Fix tg3_restart_ints()

Quoted 7 lines · David S. Miller

All 3 patches applied, looks great.

The only thing I see is that we might want to put a rmb() at the beginning of tg3_has_work() since we are clearing the status block bit right before we call it. It might not be necessary though.

I think memory barriers are not needed since tg3_has_work() does not depend on what's been written or read before it, other than sblk->status which is a direct dependency.

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

A message that is not in the archive (sent off-list or not yet received): 1114470490.4917.85.camel@rh4

maintainer
#7 lore raw

Re: [PATCH 2.6.12-rc2 3/3] tg3: Fix tg3_restart_ints()

Quoted 12 lines · On Mon, 25 Apr 2005 16:08:10 -0700 "Michael Chan" <[email protected]> wrote:

I think removing the read back of intr. mailbox in tg3_interrupt() will have the following impact:

On machines where PIOs are posted by the chipset for a long time, the write to the intr. mailbox will not reach the 57xx chip for a long time. As a result, the interrupt signal INTA will remain asserted until the mailbox write is flushed to the 57xx chip. So this can cause tg3_interrupt() to be called multiple times until the INTA signal is de- asserted. I think at most it will be called 2 times since on the second time, the status block updated bit will not be set and we will read the PCI state register, causing the mailbox write to be flushed.

Quoted 7 lines

As you described, the netif_rx_schedule() will be called again the second time in tg3_interrupt() but it will do nothing.

So I think reading the intr. mailbox will help performance by eliminating multiple calls to tg3_interrupt() in the scenario I described, but is a rather high price to pay on other systems where PIOs are not posted for very long.

I think the second interrupt will not happen, here is why.

The platform specific code invokes the interrupt handler roughly like so:

	local_irq_enable();
	ret = action->handler(irq, dev_id, regs);
	interrupt_controller->ack_irq(irq);

That ACK of the IRQ in the interrupt controller register set will be STRONGLY ordered wrt. the tg3 mailbox register write.

Therefore, the mailbox write would reach the tg3 chip, then the IRQ would be ACK'd in the interrupt controller, in that exact order.

There may be a tiny window, in the case where the interrupt controller is processor-near and the PCI bus is far away, where the IRQ unmask could occur before the tg3 register write reaches the PCI bus. But that would be 1) rare and 2) handled properly if it did occur (as you described).

I therefore see no reason not to remove this register readback.

author
#8 lore raw

Re: [PATCH 2.6.12-rc2 3/3] tg3: Fix tg3_restart_ints()

Quoted 6 lines · David S. Miller

I think the second interrupt will not happen, here is why.

The platform specific code invokes the interrupt handler roughly like so:

Quoted 8 lines
	local_irq_enable();
	ret = action->handler(irq, dev_id, regs);
	interrupt_controller->ack_irq(irq);

That ACK of the IRQ in the interrupt controller register set will be STRONGLY ordered wrt. the tg3 mailbox register write.

But no PCI cycles will necessarily be generated during the ack_irq() call. The apic_write() call or the I/O cycles to the 8259 PIC or other cycles to ack the interrupt controller are not PCI cycles and therefore may not necessarily flush the tg3 mailbox write PCI cycle. There is no spec that I'm aware of that requires posted cycles to be flushed when INTx is asserted or the IRQ is acknowledged.

I am not arguing that the read-back should not be removed. I think most systems will benefit from its removal. But there may be some systems with long posted cycles that will be adversely (performance-wise) affected by its removal.

BTW, when we use MSI, all these problems will disappear. The interrupt will arrive in order and since MSI is equivalent to an edge interrupt, it will not remain asserted like INTx and there is no need to flush the interrupt mailbox.

Quoted 9 lines

Therefore, the mailbox write would reach the tg3 chip, then the IRQ would be ACK'd in the interrupt controller, in that exact order.

There may be a tiny window, in the case where the interrupt controller is processor-near and the PCI bus is far away, where the IRQ unmask could occur before the tg3 register write reaches the PCI bus. But that would be 1) rare and 2) handled properly if it did occur (as you described).

Agreed. It will be handled properly.

Quoted 3 lines

I therefore see no reason not to remove this register readback.