On 06/26/2013 12:16 AM, Sarah Sharp wrote:
I'm a little bit confused by this patch, so I'm CC-ing the list.
On Tue, Jun 25, 2013 at 08:52:49AM +0300, Xenia Ragiadakou wrote:
According to DMA-API-HOWTO, if coherent DMA address
mask has not been set explicitly, and the driver
calls dma_alloc_coherent() or dma_pool_create() to
allocate consistent DMA memory blocks, the consistent
DMA mapping interface will return by default DMA
addresses which are 32-bit addressable.
In the xhci driver, coherent_dma_mask is not set to
64-bits to take advantage of 64-bit DMA mapping
when it is supported.
Another thing is that dma_set_mask() tests internally
whether dma_mask pointer is NULL. For pci platforms,
dma_mask pointer initialization is done when pci devices
are enumerated (the dma_mask for pci devices is set to
32-bits). However, for non-pci platforms, dma_mask pointer
is not initialized and dma_set_mask() will fail.
This patch initializes the dma_mask pointer to point
to the coherent_dma_mask, since the same value will
be assigned to both, and adds a check on the value
returned by dma_set_mask().
dma_set_coherent_mask() is not called because it is
guaranteed that if dma_set_mask() succeeds, for a
given bitmask, dma_set_coherent_mask() will also
succeed for the same bitmask.
Did you mean to say "The return value of dma_set_coherent_mask() is not
checked because..." instead of "dma_set_coherent_mask() is not called
because..." ?
I believe the discussion was that dma_set_coherent_mask() is guaranteed
to succeed if dma_set_mask() succeeds. However, you still need to set
dev.coherent_dma_mask with a call to dma_set_coherent_mask(). I don't
see where in this patch you do that.
Since, now the dma_mask pointer points to the coherent_dma_mask,
the value of both will be set via dma_set_mask().
I do not know if that is more clear.
Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
---
drivers/usb/host/xhci.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d8f640b..b5db324 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4672,11 +4672,14 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
*/
xhci = hcd_to_xhci(hcd);
temp = xhci_readl(xhci, &xhci->cap_regs->hcc_params);
- if (HCC_64BIT_ADDR(temp)) {
+ dev->dma_mask = &dev->coherent_dma_mask;
This should be
if (!dev->dev.dma_mask)
dev->dma_mask = &dev->coherent_dma_mask;
If the dma_mask pointer is already set (say by the PCI core), we don't
want to overwrite it. We just want to set the mask pointer if this is a
platform device without a DMA pointer.
I did not add, in purpose, this check because since both dma_mask
and coherent_dma_mask will be assigned the same bitmask,
I thought better to make the assignment once.
The fact that maybe there is problem when changing the address to
which points dma_mask in pci platforms didnot cross my mind at
that moment.
+ if (HCC_64BIT_ADDR(temp) &&
+ !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));
+ dma_set_mask(dev, DMA_BIT_MASK(64));
I believe you wanted to call dma_set_coherent_mask() in that last line
above, rather than calling dma_set_mask() again (you called it in the if
statement).
Sorry, about that! Actually I wanted to call just:
xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
} else {
- dma_set_mask(hcd->self.controller, DMA_BIT_MASK(32));
+ if (dma_set_mask(dev, DMA_BIT_MASK(32)))
+ goto error;
I think you need to call dma_set_coherent_mask(dev, DMA_BIT_MASK(32))
when that dma_set_mask() call succeeds. If we're dealing with a
platform device, it may not have the coherent mask set. Alan, Andy,
Felipe, does this sound correct?
As I said above, dma_set_mask() will set the DMA bitmask to both,
if this bitmask is supported. If it is supported, it is guaranteed
that the same bitmask will also be supported for consistent DMA
mappings so there is no need to call dma_set_coherent_mask()
(since now the coherent mask is already set via dma_set_mask()
and it is sure that it is supported).
}
return 0;
}
@@ -4710,11 +4713,14 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
xhci_dbg(xhci, "Reset complete\n");
temp = xhci_readl(xhci, &xhci->cap_regs->hcc_params);
- if (HCC_64BIT_ADDR(temp)) {
+ dev->dma_mask = &dev->coherent_dma_mask;
+ if (HCC_64BIT_ADDR(temp) &&
+ !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));
+ dma_set_mask(dev, DMA_BIT_MASK(64));
} else {
- dma_set_mask(hcd->self.controller, DMA_BIT_MASK(32));
+ if (dma_set_mask(dev, DMA_BIT_MASK(32)))
+ goto error;
}
Given that this code is in xhci_gen_setup twice, can you please refactor
the existing code into a separate function, and then in another patch
change that function to set the dev->dma_mask pointer, and then call
dma_set_mask and dma_set_coherent_mask?
Yes.
xhci_dbg(xhci, "Calling HCD init\n");
--
1.8.3.1
Sarah Sharp
p.s.
+ if (HCC_64BIT_ADDR(temp) &&
+ !dma_set_mask(dev, DMA_BIT_MASK(64))) {
Although Greg prefers aligning trailing lines to parenthesis, I do not.
I would prefer this line look like:
if (HCC_64BIT_ADDR(temp) &&
!dma_set_mask(dev, DMA_BIT_MASK(64))) {
Please stick with the coding style in the xHCI driver and align the code
according the standard vim or emacs C file indentation. You can set up
your .vimrc file to recognize file types by adding this line:
filetype plugin indent on
Ok, I just overlooked at it. I will stick to the coding style that xhci uses
so far.
--
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
