Re: [PATCH v1 1/1] ARM: Select DMA_DIRECT_REMAP to fix restricted DMA

[Jim Quinlan]

On Thu, Sep 28, 2023 at 11:17 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Thu, Sep 28, 2023, at 10:00, Jim Quinlan wrote:
> > On Thu, Sep 28, 2023 at 9:32 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >>
> >> On Thu, Sep 28, 2023, at 08:07, Jim Quinlan wrote:
> >> > On Wed, Sep 27, 2023 at 7:10 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> >> >>
> >> >> Clearly if you want to do this, surely the ARM-specific
> >> >> arch/arm/mm/dma-mapping.c and arch/arm/mm/dma-mapping-nommu.c
> >> >> needs to be removed at the same time?
> >> >
> >> >
> >> > Yes, this is the reason I used "RFC" as the fix looked too easy to be viable :-)
> >> > I debugged it enough to see that the host driver's
> >> > writes to the dma_alloc_coherent() region  were not appearing in
> >> > memory, and that
> >> > led me to DMA_DIRECT_REMAP.
> >>
> >> Usually when you see a mismatch between the data observed by the
> >> device and the CPU, the problem is an incorrect "dma-coherent"
> >> property in the DT: either the device is coherent and accesses
> >> the cache but the CPU tries to bypass it because the property
> >> is missing, or there is an extraneous property and the CPU
> >> goes the through the cache but the devices bypasses it.
> >
> > I just searched, there are no "dt-coherent" properties in our device tree.
> > Also, even if we did have them, wouldn't things also fail when not using
> > restricted DMA?
>
> Correct, it should be independent of restricted DMA, but it might
> work by chance that way even if it's still wrong. If your DT
> is marked as non-coherent (note: the property to look for
> is "dma-coherent", not "dt-coherent"), can you check the
> datasheet of the SoC to if that is actually correct?

Our PCIe RC device is not dma-coherent.

>
> If the chip is designed to support high-speed devices on
> PCIe, it's likely that the PCIe root complex is either coherent
> with the caches, or can (and should) be configured that way
> for performance reasons.

Our RC is definitely not coherent with the ARM/ARM64 caches.

>
> >> It could also be a driver bug if the device  mixes up the
> >> address spaces, e.g. passing virt_to_phys(pointer) rather
> >> than the DMA address returned by dma_alloc_coherent().
> >
> > This is an Intel 7260 part using the iwlwifi driver, I doubt it has
> > errors of that kind.
>
> It's unlikely but not impossible, as the driver has some
> unusual constructs, using a lot of coherent mappings that
> might otherwise be streaming mappings, and relying on
> dma_sync_single_for_device(..., DMA_BIDIRECTIONAL) for other
> data, but without the corresponding dma_sync_single_for_cpu().
> If all the testing happens on x86, this might easily lead
> to a bug that only shows up on non-coherent systems but
> is never seen during testing.
>
> If the problem is not the "dma-coherent" property, can you
> double-check if using a different PCIe device works, or narrow
> down which specific buffer you saw get corrupted?

I've done some testing, below are the results.  The new two devices, a
USB controller
and an M2 NVMe stick, behave the same as iwlwifi.

Note that I'm not advocating that "select DMA_DIRECT_REMAP" is the
anser, I'm just
showing that it fixes my examples.

Regards,
Jim Quinlan
Broadcom STB/CM


VER      PCI-DEV                       <--------- RESTRICTED DMA --------->
                      ARM64    ARM     ARM64    ARM    ARM+DMA_DIRECT_REMAP
5.15     iwlwifi        P       P        P       F             P
5.15     nvme           P       P        P       F             P
5.15     usb            P       P        P       F             P

6.1      iwlwifi        P       P        P       F             P
6.1      nvme           P       P        P       F             P
6.1      usb            P        P       P       F             P

Upstrm   iwlwifi        P       P        F       F             F
Upstrm   nvme           P       P        F       F             F
Upstrm   usb            P       P        F       F             F
                      ARM64    ARM     ARM64    ARM    ARM+DMA_DIRECT_REMAP
VER      PCI-DEV                       <--------- RESTRICTED DMA --------->

LEGEND:
  P       := pass, driver probe and some functional test performed
  F       := fail, usually when probe is called; impossible to do
functional test
  Upstrm  := 633b47cb009d "Merge tag 'scsi-fixes' of
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi"

  iwlwifi := 7260 Wifi 8086:08b1
  nvme    := 1e95:1007
  usb     := Supahub, 1912:0014



>
>      Arnd

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature