svn commit: r205090 - head/sys/dev/bge

Bruce Evans brde at optusnet.com.au
Mon Mar 15 16:47:33 UTC 2010


On Sun, 14 Mar 2010, Pyun YongHyeon wrote:

> On Sat, Mar 13, 2010 at 11:05:11PM +1100, Bruce Evans wrote:
>> On Fri, 12 Mar 2010, Pyun YongHyeon wrote:
>>
>>> Log:
>>> Reorder interrupt handler a bit such that producer/consumer
>>> index of status block is read first before acknowledging the
>>> interrupts. Otherwise bge(4) may get stale status block as
>>> acknowledging an interrupt may yield another status block update.
>>>
>>> Reviewed by:	marius
>>
>> Er, doesn't this give a race instead?  It undoes a critical part of
>> rev.1.169 but not the comment part which still says that the ack is
>
> You're probably right it may increase race window for interrupts.

Rev.1.169 was supposed to fix all races involving the interrupt ack.
An increase from 0 to epsilon is large :-).

> But it ensures coherent accesses to the status block which is more
> important than losing interrupts.

Interrupts weren't lost after 1.169 AFAIK.  Rather the reverse.  We
take a few extra interrupts in order not to miss any.  I don't see how
your change improves coherency but don't really understand the coherency.
But^2 I now remember discussing with scottl that there is no locking
for the status block and it is not clear whether there should be or
how things mostly work when there isn't (mutex-type locking seems to
be useless).

> I think old code fought not to
> lose interrupts by acknowledging interrupts first and tried to get
> next interrupts if status block was updated in interrupt handler.
> Old code also had similar race window because it blindly
> acknowledged the interrupt, status block could be updated after the
> acknowledgment of the interrupt.

Why is this fighting?  Acking the interrupt should have no affect on
the contents of the status block, and any status block update after
acking should cause a new interrupt.  Of course the hardware might
not be as nice as that, but something like this seems to be needed
for a status block to work efficiently.

Anyway, the old race window (if it is a race that matters) isn't similar,
but is the opposite race.

>
>> done first, and why (to ensure getting another interrupt if the status
>> block changes after we have looked at it).
>>
>> % 	/*
>> % 	 * Do the mandatory PCI flush as well as get the link status.
>> % 	 */
>> % 	statusword = CSR_READ_4(sc, BGE_MAC_STS) & BGE_MACSTAT_LINK_CHANGED;
>> %
>> % 	/* Make sure the descriptor ring indexes are coherent. */
>> % 	bus_dmamap_sync(sc->bge_cdata.bge_status_tag,
>> % 	    sc->bge_cdata.bge_status_map,
>> % 	    BUS_DMASYNC_POSTREAD | BUS_DMASYNC_POSTWRITE);
>> % 	rx_prod = sc->bge_ldata.bge_status_block->bge_idx[0].bge_rx_prod_idx;
>> % 	tx_cons = sc->bge_ldata.bge_status_block->bge_idx[0].bge_tx_cons_idx;
>> % 	sc->bge_ldata.bge_status_block->bge_status = 0;
>> % 	bus_dmamap_sync(sc->bge_cdata.bge_status_tag,
>> % 	    sc->bge_cdata.bge_status_map,
>> % 	    BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
>>
>> The above presumably gives sufficiently coherent accesses to the status
>> block, but what happens if a status update occurs now (before the ack).
>
> Theoretically this may happen if interrupt is shared with other
> devices. Since bge(4) does not check whether the interrupt is ours
> it may blindly process the interrupt.

Well, my version checks, but you said before that some hardware cannot
be trusted to get this right (it works with my 5701 UP no-MSI).

I see that you are saying that your change doesn't help if the interrupt
isn't ours.  Then the ack is still done first (long ago, on the last
bge_intr()) so the above may run partly before the interrupt is ours
and partly after, giving the same problem (the one that I don't
understand :-) caused by running the above after an ack.  Is the status
block supposed to be frozen once an interrupt really for us occurs
until we ack the interrupt?

>
>> Doesn't the ack prevent an interrupt for this status update?  I think
>
> If interrupt is shared with other devices it wouldn't.
>
>> tx_prod and tx cons (read above) don't become stale since they are only
>> advanced by software, and we may processes tx and rx descriptors beyond
>> the ones reported by status updates before or after the ack, but
>> statusword (read above) does become stale.
>>
>
> I think this was a long standing bug of bge(4) and it kept bge(4)
> from running on PAE environments. In order not to lose interrupts I
> believe bge(4) should use tagged status mode which will enable
> interrupt tracking via status block. Relying on some timing in
> interrupt handler can't solve the root cause. All controllers
> except BCM5700 supports tagged status mode and this commit is the
> first step to the tagged status mode which requires coherent
> accesses to the status block. Because driver should tell which
> interrupts were handled in the interrupt handler coherent access
> to status block is critical one.

> ...
>> % 	 *
>> % 	 * We do the ack first to ensure another interrupt if there is a
>> % 	 * status update after the ack.  We don't check for the status
>>
>> But we don't do the ack first any more.
>>
>
> I agree current comment does not match with code. I'll fix that
> after implementing tagged status mode.

OK, if you as fast as usual :-).

Bruce


More information about the svn-src-head mailing list