Re: [RFC v4] xhci: fix dma mask setup in xhci.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Thanks for your remarks. I forward your mail to the usb list.
Sorry about the double mails that you received, i forgot that i had CC you and
Al Cooper inside the patch.

On 07/24/2013 04:32 PM, Andy Shevchenko wrote:
On Wed, Jul 24, 2013 at 4:56 AM, Xenia Ragiadakou <burzalodowa@xxxxxxxxx> wrote:
The function dma_set_mask() tests internally whether the dma_mask pointer
for the device is initialized and fails if the dma_mask pointer is NULL.
On pci platforms, the device dma_mask pointer is initialized, when pci
devices are enumerated, to point to the pci_dev->dma_mask which is 0xffffffff.
However, for non-pci platforms, the dma_mask pointer may not be initialized
and in that case dma_set_mask() will fail.

This patch initializes the dma_mask and the coherent_dma_mask to 32bits
in xhci_plat_probe(), before the call to usb_create_hcd() that sets the
"uses_dma" flag for the usb bus and the call to usb_add_hcd() that creates
coherent dma pools for the usb hcd.

Moreover, a call to dma_set_mask() does not set the device coherent_dma_mask.
Since the xhci-hcd driver calls dma_alloc_coherent() and dma_pool_alloc()
to allocate consistent DMA memory blocks, the coherent DMA address mask
has to be set explicitly.

This patch sets the coherent_dma_mask to 64bits in xhci_gen_setup() when
the xHC is capable for 64-bit DMA addressing.

If dma_set_mask() succeeds, for a given bitmask, it is guaranteed that
the given bitmask is also supported for consistent DMA mappings.
So there is no need to call dma_set_coherent_mask(), that will call
again internally dma_supported(), and coherent_dma_mask can be assigned
directly.

Other changes introduced in this patch are:

- The return value of dma_set_mask() is checked to ensure that the required
   dma bitmask conforms with the host system's addressing capabilities.

- The dma_mask setup code for the non-primary hcd was removed since both
   primary and non-primary hcd refer to the same generic device whose
   dma_mask and coherent_dma_mask are already set during the setup of
   the primary hcd.

- The code for reading the HCCPARAMS register to find out the addressing
   capabilities of xHC was removed since its value is already cached in
   xhci->hccparams.

- hcd->self.controller was replaced with the dev variable since it is
   already available.
Thanks for an update, few comments below.

--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -104,6 +104,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
         if (!res)
                 return -ENODEV;

+       if (!pdev->dev.dma_mask)
+               pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+       if (!ret)
+               pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+       else
+               return ret;
+
Something wrong with the logic here.

You have two variables: dma_mask (u64 *) and dma_coherent_mask (u64).
In case of dma_mask == NULL you initialize it by &dma_coherent_mask.
That part is okay.
Next, you set dma_mask value followed by setting coherent mask which
doesn't make much sense in this order.

What you may do is to check dma_mask and: a)initialize it,  b) apply
coherent mask.
The other way (when dma_mask !=NULL) you may apply dma_mask followed
by configuring dma_coherent_mask.

Optimization of this is a home work.

Yes, you are right. There is an unnecessary assignment in the above piece of code
if dma_mask is initialized to point to the coherent_dma_mask.
The only thing that i can think to avoid that is by setting first the coherent_dma_mask.
For instance:

ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)).
if (ret) { return ret; }
if (!pdev->dev.dma_mask) { pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; }
else { dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); }

I don't know, is that what you have in mind?


         hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
         if (!hcd)
                 return -ENOMEM;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 189ca85..2938b99 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4761,7 +4761,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
         struct xhci_hcd         *xhci;
         struct device           *dev = hcd->self.controller;
         int                     retval;
-       u32                     temp;

         /* Accept arbitrarily long scatter-gather lists */
         hcd->self.sg_tablesize = ~0;
@@ -4789,14 +4788,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
                 /* xHCI private pointer was set in xhci_pci_probe for the second
                  * registered roothub.
                  */
-               xhci = hcd_to_xhci(hcd);
-               temp = xhci_readl(xhci, &xhci->cap_regs->hcc_params);
-               if (HCC_64BIT_ADDR(temp)) {
-                       xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
-                       dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64));
-               } else {
-                       dma_set_mask(hcd->self.controller, DMA_BIT_MASK(32));
-               }
                 return 0;
         }

@@ -4828,12 +4819,11 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
                 goto error;
         xhci_dbg(xhci, "Reset complete\n");

-       temp = xhci_readl(xhci, &xhci->cap_regs->hcc_params);
-       if (HCC_64BIT_ADDR(temp)) {
+       /* set 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_mask(hcd->self.controller, DMA_BIT_MASK(64));
-       } else {
-               dma_set_mask(hcd->self.controller, DMA_BIT_MASK(32));
+               dev->coherent_dma_mask = DMA_BIT_MASK(64);
What about be consistent and use dma_set_coherent_mask() wrapper?

--
With Best Regards,
Andy Shevchenko

I will use the dma_set_coherent_mask() in the 5th version of the RFC.
I was trying to avoid calling twice dma_supported() for the same bitmask
but i guess you are right in this point too.

regards,
ksenia

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux