RE: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[David Laight <David.Laight@ACULAB.COM>]

From: Jonas Dreßler
> Sent: 14 September 2021 12:48
> 
> On the 88W8897 card it's very important the TX ring write pointer is
> updated correctly to its new value before setting the TX ready
> interrupt, otherwise the firmware appears to crash (probably because
> it's trying to DMA-read from the wrong place). The issue is present in
> the latest firmware version 15.68.19.p21 of the pcie+usb card.
> 
> Since PCI uses "posted writes" when writing to a register, it's not
> guaranteed that a write will happen immediately. That means the pointer
> might be outdated when setting the TX ready interrupt, leading to
> firmware crashes especially when ASPM L1 and L1 substates are enabled
> (because of the higher link latency, the write will probably take
> longer).
> 
> So fix those firmware crashes by always using a non-posted write for
> this specific register write. We do that by simply reading back the
> register after writing it, just as a few other PCI drivers do.
> 
> This fixes a bug where during rx/tx traffic and with ASPM L1 substates
> enabled (the enabled substates are platform dependent), the firmware
> crashes and eventually a command timeout appears in the logs.

I think you need to change your terminology.
PCIe does have some non-posted write transactions - but I can't
remember when they are used.

What you need to say is that you are flushing the PCIe posted
writes in order to avoid a timing 'issue' setting the TX ring
write pointer.

Quite where the bug is, and why the read-back actually fixes
it is another matter.

A typical ethernet transmit needs three things written
in the correct order (as seen by the hardware):

1) The transmit frame data.
2) The descriptor ring entry referring to the frame.
3) The 'prod' of the MAC engine to process the frame.

You seems to also have:
2.5) Write the TX ring write pointer to the MAC engine.

The updates of (1) and (2) are normally handles by DMA coherent
memory or cache flushes done by using the DMA APIs.

If the writes for (2.5) and (3) are both writing to the
PCIe card (which seems likely) then the PCIe spec will
guarantee that they happen in the correct order.

This means that the PCIe readback of the (2.5) write doesn't
have any effect on the order of the bus cycles seen by the card.
So flushing the PCIe write isn't what fixes your problem.

The readback between (2.5) and (3) does have two effects:
a) it adds a short delay between the two writes.
b) it (probably) forces the first write to by flushed through
   any posted-write buffers on the card itself.

It may well be that the card has separate posted write buffers
for different parts of the hardware.
In that case the write (3) might get actioned before the write (2.5).
OTOH you'd expect that to only cause packet transmit to be delayed.

If the write (2.5) ends up being non-atomic (ie a 64bit write
converted to multiple 8 bit writes internally) then you'll hit
problems if the mac engine looks at the register while it is
being changed just after transmitting the previous packet.
(ie when the tx starts before write (3) because the tx logic
is active.)

The other horrid possibility is that you have a truly broken
PCIe slave that corrupts its posted-write buffer when a second
write arrives.
If that is actually true then you may need to also add locks
to ensure that multiple threads cannot do writes at the same time.
Or do all (and I mean all) accesses from a single thread/context.

The latter problem reminds me of a PCI card that got terribly
confused if it saw a read request from a 2nd cpu while generating
'cycle rerun' responses to an earlier read request.

Most code that flushes posted writes only needs to do so for
writes that drop level-sensitive interrupt requests.
Failure to flush those can lead to unexpected interrupts.
That problem goes back to VMEbus sunos (amongst others).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)