Re: [PATCH v3] usb: dwc3: support 64 bit DMA in platform driver

[Arnd Bergmann <arnd@arndb.de>]

On Mon, Jun 7, 2021 at 11:06 AM Sven Peter <sven@svenpeter.dev> wrote:
> On Mon, Jun 7, 2021, at 10:22, Arnd Bergmann wrote:
>
> I've looked at Documentation/core-api/dma-api-howto.rst again which mentions that
>
>         By default, the kernel assumes that your device can address 32-bits of DMA
>         addressing.  For a 64-bit capable device, this needs to be increased, and for
>         a device with limitations, it needs to be decreased.
>         [...]
>         These calls usually return zero to indicated your device can perform DMA
>         properly on the machine given the address mask you provided, but they might
>         return an error if the mask is too small to be supportable on the given
>         system.  If it returns non-zero, your device cannot perform DMA properly on
>         this platform, and attempting to do so will result in undefined behavior.
>         You must not use DMA on this device unless the dma_set_mask family of
>         functions has returned success.
>
> which, unless I'm reading this incorrectly, should mean that asking for a 64bit
> mask is always fine. In the worst case the mask will just be downgraded to
> 32bit if the bus is correctly annotated (the places I looked at that use the mask
> take the min of that one and dev->bus_dma_limit).
> Only asking for a mask that is too small would be bad.
>
> I have also found [1],[2] which made changes to that documentation and that also
> seems to confirm that it's fine to just ask for a 64 bit mask either way.

Indeed, I forgot about that change, this does make it easier.

> So for these cases
>
> > > > This will now  fail on machines with dwc3 connected to a 32-bit bus (or a
> > > > bus that is accidentally not annotated as supporting 64-bit) when there is
> > > > some memory that is not addressable through that bus.
>
> the call should return success but the final mask used for allocations should
> remain at 32bit. Before the change no memory above the 32bit limit was used by
> the dwc3 core and after the change we still can't use any memory above the
> 32bit limit.

Right.

> Now if we had a dwc3 controller with
>  * a quirk that only allows 32bit DMA for the core dwc3 controller
>  * but support for >32bit DMA for xhci buffers (xhci already asks for a 64bit mask)
>  * on a bus that's otherwise annotated to support 64bit
> this change will break that.
>
> But that's unrelated to the dma_set_mask_and_coherent return value since
> just calling it with a 64bit mask will already cause trouble (and also be successful!).
>
> The problem I see is that we likely wouldn't know about devices with a quirk like this
> since so far everything has been working fine there. I'm not really sure how to guard
> against that either since we would only notice on the first DMA transfer above the 32bit
> limit. I'm also not sure how likely the existence of such a weird device is.
>
> This hypothetical dwc3 controller should probably either be confined to a bus with a
> proper 32bit limit or get a quirk that enforces allocations from ZONE_DMA32. Doesn't
> change the fact that they used to work but would now break after this patch.

Makes sense, so for your v3 patch:

Reviewed-by: Arnd Bergmann <arnd@arndb.de>