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
--
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
