On 07/01/2013 01:29 AM, Sarah Sharp wrote:
On Wed, Jun 26, 2013 at 01:20:53AM +0300, Xenia Ragiadakou wrote:
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:
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.
Ah, ok, I understand why you wrote the original code that way.
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.
I looked at the PCI init code. Here's what happens in
drivers/pci/probe.c:
During PCI initialization, pci_dev->dma_mask gets set to 32-bits in
pci_setup_device(). Later, when the PCI device is added, the base
device structure's DMA mask is changed to point to pci_dev->dma_mask.
That means pci_device_add() sets pci_dev->dev.dma mask to point to
pci_dev->dma_mask. The pci_dev->dev.coherent_dma_mask is set to a
32-bit value.
If you change the dev->dma_mask pointer to point to
&dev->coherent_dma_mask, that means the pci_dev->dma_mask won't get
updated when you call dma_set_mask(). That wouldn't be good, since the
underlying base device and pci_dev structures could end up with
different DMA masks.
The TLDR; version is that if dev->dma_mask is set, you shouldn't change
it. That means you'll need to add the additional if statement, and
call dma_set_coherent_mask() as well.
+ 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");
Ok, please fix that too.
} 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).
Right, there was no need to call dma_set_coherent_mask() in your
original code, but in the next revision of the patch, you will need to
call it here.
Sarah Sharp
Yes, you are right. What about removing entirely the dma_mask
setup code from xhci_gen_setup() and let the dma_mask setup
to be handled in xhci_pci_setup() and xhci_plat_setup() accordingly?
I suggest that because, for pci platforms since both dma_mask
and dma_coherent_mask is already initialized to 32 bits, we don't
need to set them again if dma_set_mask(dev, DMA_BIT_MASK(64))
fails or to check if dma_mask is uninitialized.
And for non-pci platforms, if dma_mask=&coherent_dma_mask,
we will only need to set the dma_mask and not both.
But anyway it does not make a big difference if there is a generic
dma_mask setup in xhci_gen_setup().
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
