Re: [PATCH 1/2] mwifiex: Use non-posted PCI register writes

[+cc Keith]

On Wed, Sep 01, 2021 at 05:41:56PM -0500, Bjorn Helgaas wrote:

On Wed, Sep 01, 2021 at 07:07:58PM +0200, Johannes Berg wrote:

quoted

On Wed, 2021-09-01 at 18:51 +0200, Heiner Kallweit wrote:

quoted

On 01.09.2021 17:51, Pali Rohár wrote:

quoted

On Wednesday 01 September 2021 16:01:54 Jonas Dreßler wrote:

quoted

On 8/30/21 2:49 PM, Andy Shevchenko wrote:

quoted

On Mon, Aug 30, 2021 at 3:38 PM Jonas Dreßler [off-list ref] wrote:

quoted

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).

This sounds somehow like the typical case where you write DMA descriptors
and then ring the doorbell. This normally requires a dma_wmb().
Maybe something like that is missing here?

But it looks like this "TX ring write pointer" is actually the register?

However, I would agree that doing it in mwifiex_write_reg() is possibly
too big a hammer - could be done only for reg->tx_wrptr, not all the
registers?

Actually, can two writes actually cross on PCI?

Per PCIe r5.0, sec 2.4.1,

  A2a  A Posted Request must not pass another Posted Request unless A2b
       applies.

  A2b  A Posted Request with RO Set is permitted to pass another
       Posted Request.  A Posted Request with IDO Set is permitted to
       pass another Posted Request if the two Requester IDs are
       different or if both Requests contain a PASID TLP Prefix and
       the two PASID values are different.

A few drivers enable RO (Relaxed Ordering) for their devices, which
means the *device* is permitted to set the RO bit in transactions it
initiates.

BUt IIUC we're talking about MMIO writes initiated by a CPU, and they
won't have the RO bit set unless the Root Port has Relaxed Ordering
enabled, and Linux generally does not enable that.  So A2a should
apply, and writes should be ordered on PCI.

There are a few wrinkles that I worry about:

  d1e714db8129 ("mtip32xx: Fix ERO and NoSnoop values in PCIe upstream
  on AMD systems") [1] turns off RO for some AMD Root Ports, which
  makes me think BIOS might be enabling RO in these Root Ports.

  c56d4450eb68 ("PCI: Turn off Request Attributes to avoid Chelsio T5
  Completion erratum") [2] turns off RO for all Root Ports leading to
  Chelsio T5 devices, which again makes me think there's firmware that
  enables RO in Root Ports.  Follow-up [3].

  77ffc1465cec ("tegra: add PCI Express support") [4] (see
  tegra_pcie_relax_enable()) enables RO for Tegra Root Ports due to
  some hardware issue.  I don't whether these Root Ports every
  actually *set* RO in the PCIe transactions they generate.  Follow-up
  [5].

These concern me because I don't think we have a way for drivers to
specify whether their writes should use strong ordering or relaxed
ordering, and I think they depend on strong ordering.

Sorry, I was completely wrong about this part.  Keith Busch reminded
me that writel_relaxed() is an obvious way for drivers to specify that
they don't need strong ordering.

I apologize for the distraction.

If Root Ports have RO enabled, I think we are at risk, so I suspect
Linux should actively *disable* RO for Root Ports.

[1] https://git.kernel.org/linus/d1e714db8129
[2] https://git.kernel.org/linus/c56d4450eb68
[3] https://lore.kernel.org/r/20210901222353.GA251391@bjorn-Precision-5520 (local)
[4] https://git.kernel.org/linus/77ffc1465cec
[5] https://lore.kernel.org/r/20210901204045.GA236987@bjorn-Precision-5520 (local)