Message-ID: <69fa8e4d-df35-c89e-ba60-545b8aa40e31@arm.com>
Date:   Fri, 26 Apr 2019 15:03:24 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Pankaj Dubey <pankaj.dubey@...sung.com>,
        Sriram Dash <sriram.dash@...sung.com>, mathias.nyman@...el.com
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        gregkh@...uxfoundation.org, Jingoo Han <jingoohan1@...il.com>,
        Krzysztof Kozlowski <krzk@...nel.org>, mgautam@...eaurora.org,
        felipe.balbi@...ux.intel.com
Subject: Re: [PATCH] usb: xhci: inherit dma_mask from bus if set correctly

On 26/04/2019 05:46, Pankaj Dubey wrote:
> 
> On 4/24/19 4:28 PM, Robin Murphy wrote:
>> On 24/04/2019 10:05, Pankaj Dubey wrote:
>>> 
>>> 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@...sung.com> wrote:
>>>>>> 
>>>>>> On Tue, 2 Apr 2019 at 15:34, Robin Murphy
>>>>>> <robin.murphy@....com> wrote:
>>>>>>> 
>>>>>>> On 02/04/2019 10:40, Pankaj Dubey wrote:
>>>>>>>> From: Sriram Dash <sriram.dash@...sung.com>
>>>>>>>> 
>>>>>>>> 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
>>>>>> device_node *np, const char *bus_id, void *platform_data, 
>>>>>> struct device *parent) { 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@...sung.com> 
>>>>>>>> Signed-off-by: Sriram Dash <sriram.dash@...sung.com> 
>>>>>>>> --- 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 lore.kernel.org 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 outlook.com is
>>>> doing SPF negotiation with samsung.com, 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 better or worse, that is the expected and intended behaviour
>> for the default device masks. Drivers these days are expected to 
>> explicitly set their supported mask to replace the default, but
>> there are still some remaining legacy assumptions that the default
>> masks are 32-bit, so making them bigger risks subtle breakage, and
>> that's why of_dma_configure() does the weird things it does.
>> 
> In that case, for systems supporting masks greater than 32-bit, IMO, 
> they should be able to handle the mask properly via DT. Without 
> disturbing legacy code, this is one solution that can be considered. 
> Requesting you to give your opinion on this, if it is acceptable we
> will submit formal patch for this.
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c index
> 3717f2a..9cc7a28 100644 --- a/drivers/of/device.c +++
> b/drivers/of/device.c @@ -151,10 +151,19 @@ int
> of_dma_configure(struct device *dev, struct device_node *np, bool
> force_dma) mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1); 
> dev->coherent_dma_mask &= mask; *dev->dma_mask &= mask; -       /*
> ...but only set bus mask if we found valid dma-ranges earlier */ -
> if (!ret) -               dev->bus_dma_mask = mask;
> 
> +       /* +        * ...but only set bus mask if we found valid
> dma-ranges earlier +        * and also, set the coherent_dma_mask and
> dma_mask properly +        * for busses with size more than 32-bit +
> */ +       if (!ret) { +               dev->bus_dma_mask = mask; +
> if (mask  >= (1ULL << 32)) { +
> dev->coherent_dma_mask = mask; +                       *dev->dma_mask
> = mask; +               } +       } coherent =
> of_dma_is_coherent(np); dev_dbg(dev, "device is%sdma coherent\n", 
> coherent ? " " : " not ");
> 
> 
>>> 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.
>> 
>> Thanks for the clarification.
>> 
>>> 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.
>> 
>> If the bus mask is correctly set to 36 bits, but the DMA API 
>> implementation is failing to take that into account and giving you 
>> 40-bit DMA addresses, that is a bug in the DMA API implementation,
>> and it needs to be fixed in that DMA API code, not worked around
>> in individual drivers.
> 
> There are 2 issues here. First being, the 32-bit limitation for the 
> device dma_mask during device registration. You have already
> suggested one approach for this to set from driver itself. IMO, this
> would require another DT node property addition for any individual
> IP. As same driver is being used in another SoC where, it may not
> need more than 32-bit/64-bit dma_mask.
> 
> For the SoC concerned here, there are multiple IPs which are sharing
> the 36-bit DATA bus. If of_dma_configure" takes care of assigning
> the dma_mask properly from "dma-ranges" DT property, it would solve
> this issue and driver's do not need to set this explicitly either
> via hard-coding or through another DT property. For this we suggest
> code snippet as given above.
> 
> The second problem is the XHCI overrides dma_mask to 32-bit or
> 64-bit. During the device registration, the DWC3 device get the
> default 32-bit dma_mask. This DWC3 serves as the parent device for
> XHCI device, which also gets 32-bit dma_mask from inception. There
> are 2 places for the xhci-device, where the dma_mask of the
> xhci-device is explicitly modified to either 32-bit or 64-bit.
> 
> 1) In "drivers/usb/host/xhci-plat.c" we have dma_mask setting as
> below:
> 
> ret = dma_coerce_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
> 
> 2) In "drivers/usb/host/xhci.c" file, here we have code as:
> 
> dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> 
> So, even if the platform sets the dma_mask properly to 36-bit, the
> xhci driver overrides it to 32-bit or 64-bit. This can be fixed in
> the xhci driver by checking if the dma_mask is already getting set in
> the parent driver. For this we have not yet submitted the change, as
> in this case of_dma_configure needs to be fixed first.

None of that is a real problem, and none of it needs to be fixed. As I 
tried to explain before, the bus mask and device masks are *expected* to 
be different sizes, because they represent different things, and are 
owned by different levels of the driver model abstraction.

> However, the proposed solution (current patch) is leveraging the
> dma_mask from bus_dma_mask, which is set from the dma-ranges 
> properly, and this case we don't need any changes in
> of_dma_configure.
> 
>> Is this a 32-bit Arm system, by any chance?
>> 
> For the SoC concerned here, it is a 64-bit ARM system, which has
> many IPs connected via the 36-bit DATA bus.

OK, if it's an arm64 system with an IOMMU, then all your DMA addresses 
come from here:


static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
		size_t size, dma_addr_t dma_limit, struct device *dev)
{
	...
	if (dev->bus_dma_mask)
		dma_limit &= dev->bus_dma_mask;
	...
}


Most likely something somewhere is passing the wrong device into DMA API 
calls - *that* is what needs to be fixed.

Of course I can't 100% rule out the possibility that something else is 
going screwy as well or instead, but either way, start debugging from 
iommu_dma_alloc_iova() and work backwards until you can see why 
dma_limit is not getting clamped to the appropriate bus mask.

Robin.

Powered by blists - more mailing lists