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.
> Signed-off-by: Xenia Ragiadakou <burzalod...@gmail.com>
> ---
> 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.
> + 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).
> } 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?
> }
> 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?
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
