On Wed, Sep 01, 2021 at 07:07:58PM +0200, Johannes Berg wrote:
> On Wed, 2021-09-01 at 18:51 +0200, Heiner Kallweit wrote:
> > On 01.09.2021 17:51, Pali Rohár wrote:
> > > On Wednesday 01 September 2021 16:01:54 Jonas Dreßler wrote:
> > > > On 8/30/21 2:49 PM, Andy Shevchenko wrote:
> > > > > On Mon, Aug 30, 2021 at 3:38 PM Jonas Dreßler <verdre@xxxxxxx> wrote:
> > > > > >
> > > > > > 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. 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
[4] https://git.kernel.org/linus/77ffc1465cec
[5] https://lore.kernel.org/r/20210901204045.GA236987@bjorn-Precision-5520
