Re: [PATCH] usb: xhci: inherit dma_mask from bus if set correctly

From: Pankaj Dubey
Date: Wed Apr 24 2019 - 05:06:06 EST

On 4/10/19 4:32 AM, Robin Murphy wrote:
> On 2019-04-09 3:56 am, Sriram Dash wrote:
>> On Tue, Apr 2, 2019 at 9:53 PM Pankaj Dubey
>> <pankaj.dubey@xxxxxxxxxxx> wrote:
>>> On Tue, 2 Apr 2019 at 15:34, Robin Murphy <robin.murphy@xxxxxxx> wrote:
>>>> On 02/04/2019 10:40, Pankaj Dubey wrote:
>>>>> From: Sriram Dash <sriram.dash@xxxxxxxxxxx>
>>>>> The xhci forcefully converts the dma_mask to either 64 or 32 and the
>>>>> dma-mask set by the bus is somewhat ignored. If the platform sets
>>>>> the
>>>>> correct dma_mask, then respect that.
>>>> It's expected for dma_mask to be larger than bus_dma_mask if the
>>>> latter
>>>> is set - conceptually, the device mask represents what the device is
>>>> inherently capable of, while the bus mask represents external
>>>> interconnect restrictions which individual drivers should not have to
>>>> care about. The DMA API backend should take care of combining the
>>>> two to
>>>> find the intersection.
>>> Thanks for the review.
>>> We are dealing here with the xhci platform which inherits the dma mask
>>> of the parent, which is from a controller device.
>>> When the controller dma mask is set by the platform in DT, what we
>>> observe is, its not getting inherited properly and the xhci bus is
>>> forcing the dma address to be either 32 bit or 64 bit.
>>> In "drivers/usb/host/xhci-plat.c" we have dma_mask setting as below:
>>> Â /* Try to set 64-bit DMA first */
>>> if (WARN_ON(!sysdev->dma_mask))
>>> ÂÂÂÂÂ /* Platform did not initialize dma_mask */
>>> ÂÂÂÂÂ ret = dma_coerce_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
>>> else
>>> ÂÂÂÂÂ ret = dma_set_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
>>> So even if the controller device has set the dma_mask as per it's
>>> configuration in DT, xhci-plat.c will override it here in else part.
>>> Next it goes to "drivers/usb/host/xhci.c" file, here we have code as:
>>> /* Set dma_mask and coherent_dma_mask to 64-bits,
>>> Â * if xHC supports 64-bit addressing */
>>> if (HCC_64BIT_ADDR(xhci->hcc_params) &&
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ !dma_set_mask(dev, DMA_BIT_MASK(64))) {
>>> ÂÂÂÂÂÂÂÂ xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
>>> ÂÂÂÂÂÂÂÂ dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>>> } else {
>>> ÂÂÂÂÂÂÂÂÂ * This is to avoid error in cases where a 32-bit USB
>>> ÂÂÂÂÂÂÂÂÂ * controller is used on a 64-bit capable system.
>>> ÂÂÂÂÂÂÂÂ retval = dma_set_mask(dev, DMA_BIT_MASK(32));
>>> ÂÂÂÂÂÂÂÂ if (retval)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return retval;
>>> ÂÂÂÂÂÂÂÂ xhci_dbg(xhci, "Enabling 32-bit DMA addresses.\n");
>>> ÂÂÂÂÂÂÂÂ dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>> }
>>> So xhci will force the dma_mask to either DMA_BIT_MASK(32) or
>>> DMA_BIT_MASK(64), what if my device needs other than 32 bit or 64 bit
>>> dma_mask.
>>> The bus_dma_mask was introduced for a case when the bus from a
>>> device's dma interface may carry fewer address bits. But apparently,
>>> it is the only mask which retains the original dma addressing from the
>>> parent. So basically what we observe is currently there is no way we
>>> can pass dma_mask greater than 32-bit, from DT via dev->dma_mask or
>>> dev->coherent_dma_mask due to below logic in
>>> from "drivers/of/platform.c" we have
>>> static struct platform_device *of_platform_device_create_pdata(
>>> {
>>> ÂÂÂÂÂÂ struct platform_device *dev;
>>> ÂÂÂÂÂÂ ...
>>> ÂÂÂÂÂÂ dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>> ÂÂÂÂÂÂ if (!dev->dev.dma_mask)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
>>> ÂÂ ...
>>> }
>>> and then in of_dma_configure function in "drivers/of/device.c" we
>>> have..
>>> mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1); //This mask
>>> computation is going fine and gets mask greater than 32-bit if defined
>>> in DT
>>> dev->coherent_dma_mask &= mask;Â // Here the higher bit [63:32] will
>>> get truncated as coherent_dma_mask is initialized to DMA_BIT_MASK(32)
>>> in platform.c
>>> *dev->dma_mask &= mask; //Same here higher bits will get truncated
>>> /* ...but only set bus mask if we found valid dma-ranges earlier */
>>> if (!ret)
>>> dev->bus_dma_mask = mask; //Only bus_dma_mask can carry the original
>>> mask as specified in platform DT.
>>> To minimise the impact on existing code, we reused the bus_dma_mask
>>> for finding the dma addressing bits.
>>> Or other way we may need to initialise dma_mask/coherent_dma_mask as
>>> DMA_BIT_MASK(64) in "drivers/of/platform.c" and let all devices set
>>> dma_mask via DT using "dma-ranges" property or respective platform
>>> driver.
>>>> Are you seeing an actual problem here, and if so
>>>> on which platform? (If the bus mask is set at all then it wouldn't
>>>> seem
>>>> to be the DT PCI issue that I'm still trying to fix).
>>> We are facing this issue in one of the Samsung's upcoming SoC where we
>>> need dma_mask greater than 32-bit.
>>> Thanks,
>>> Pankaj
>>>> Robin.
>>>>> Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
>>>>> Signed-off-by: Sriram Dash <sriram.dash@xxxxxxxxxxx>
>>>>> ---
>>>>> ÂÂ drivers/usb/host/xhci.c | 10 ++++++++++
>>>>> ÂÂ 1 file changed, 10 insertions(+)
>>>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>>>> index 005e659..55cf89e 100644
>>>>> --- a/drivers/usb/host/xhci.c
>>>>> +++ b/drivers/usb/host/xhci.c
>>>>> @@ -5119,6 +5119,16 @@ int xhci_gen_setup(struct usb_hcd *hcd,
>>>>> xhci_get_quirks_t get_quirks)
>>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>>>> ÂÂÂÂÂÂ }
>>>>> +ÂÂÂÂ /*
>>>>> +ÂÂÂÂÂ * A platform may require coherent masks other than 64/32
>>>>> bit, and we
>>>>> +ÂÂÂÂÂ * should respect that. If the firmware has already
>>>>> requested for a
>>>>> +ÂÂÂÂÂ * dma-range, we inherit the dma_mask presuming the platform
>>>>> knows
>>>>> +ÂÂÂÂÂ * what it is doing.
>>>>> +ÂÂÂÂÂ */
>>>>> +
>>>>> +ÂÂÂÂ if (dev->bus_dma_mask)
>>>>> +ÂÂÂÂÂÂÂÂÂÂÂÂ dma_set_mask_and_coherent(dev, dev->bus_dma_mask);
>>>>> +
>>>>> ÂÂÂÂÂÂ xhci_dbg(xhci, "Calling HCD init\n");
>>>>> ÂÂÂÂÂÂ /* Initialize HCD and host controller data structures. */
>>>>> ÂÂÂÂÂÂ retval = xhci_init(hcd);
>> Hello Robin,
>> Hope you found the crux of the matter. Any comments on the same?
> Sorry, I never received either of these replies - I've just happened
> to notice this thread again by pure chance while looking at the
> linux-usb patchwork for something else entirely, and managed to dredge
> an mbox off to reply to. Mail is not my area of
> expertise, but looking at the headers of the initial patch in my inbox
> it seems that is doing SPF negotiation with,
> so sending via gmail (as those replies appear to be) may be failing
> that and getting silently discarded (they're not even in my spam
> quarantine).
> From the snippets of code quoted above I don't see anything obviously
> wrong, but I'll take a closer look tomorrow. AFAICS though, if
> dev->bus_dma_mask is set then dev is probably the appropriate device
> for DMA, so I wouldn't expect a problem - XHCI is inherently a 64-bit
> device, so its driver *should* be setting a 64-bit mask in this case.
> To reiterate, what's the nature of the DMA issue? Do the mapping
> operations fail, or do you actually see transfers going wrong due to
> address truncation? Also, which arch is involved here - is it arm64
> (as I seem to have assumed), or something else?
> Robin.

Regarding issue in above code snippet, current code sets
"dev->dev.coherent_dma_mask" as DMA_BIT_MASK(32) in platform.c
(irrespective of architecture) and when we parse "dma-ranges" property
and try to set coherent_dma_mask or dma_mask in of_dma_configure
function in "drivers/of/device.c", even if "dma-ranges" specified in DT
is more than 32-bit, 32-63 bits will be cleared to zero due to masking
done in platform.c.

So effectively we are not able to set dma_mask or coherent_dma_mask
larger than 32-bit mask.

For the SoC concerned here is based on arm64, the USB IP (64 bit
capable) is connected to a 36-bit Data bus and a 32-bit Control bus. The
36-bit Data bus is connected to an IOMMU which translates the address
before they are passed on to other blocks. Here we have IOMMU is capable
of 40-bit addressing. But as data bus is only capable of 36-bit, we need
to ensure that IOMMU translates to address which does not exceed 36-bit.

Without this fix we are observing context fault from IOMMU.

Now, to get a workaround to this problem, we are inheriting the
bus_dma_mask which is apparently the only one which contains the 36-bit
bus mask.

Or as alternate solution we need to change coherent_dma_mask default
mask as DMA_BIT_MASK(64) rather DMA_BIT_MASK(32) so that in
of_dma_configure, the dma_mask/coherent_dma_mask get populated from
"dma_ranges" DT property during device registration.