Felipe, Andy, and Seb, I have a couple questions below.
On Fri, Jul 05, 2013 at 08:24:56PM +0300, Xenia Ragiadakou 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 initialization of the device dma_mask pointer is
> performed when pci devices are enumerated and is set to point to the
> pci_dev->dma_mask which is 0xffffffff. However, for non-pci platforms,
> the dma_mask pointer remains uninitialized and dma_set_mask() will fail.
>
> Also, 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, otherwise the DMA mapping interface will return
> by default DMA addresses which are 32-bit addressable.
>
> This patch removes the dma_mask setup code from xhci_gen_setup() and places
> it in xhci_pci_setup() and xhci_plat_setup(). The dma_mask setup must be
> done before the call to xhci_gen_setup() which allocates and maps to dma
> addresses the xhci-hcd buffers.
>
> For pci platforms, the dma_mask and coherent_dma_mask are set by default
> to 32bit DMA addresses. If both the xHC controller and the host support
> 64bit DMA addresses, the device dma_mask and coherent_dma_mask will be
> set to 64bits.
Xenia, I'm not sure what you mean by "the xHC controller and the host support
64 bit DMA addresses". The xHC controller is the xHCI host. Did you maybe
mean "If both the xHCI host and the system support 64-bit DMA"?
> For non-pci platforms, the dma_mask pointer is initialized to point to
> the coherent_dma_mask since the same value will be assigned to both.
Felipe, Andy, is there any chance that a platform_device dma_mask
pointer would already be initialized by the time the probe function is
called? We wouldn't want to overwrite it. Can you please check the
xhci_plat_probe code?
I'm also a bit confused as to why the platform device code could work at
all in the current state. Xenia's patch sets usb_hcd->self.uses_dma.
The xHCI platform code currently doesn't set this flag. The xHCI driver
also doesn't set the HCD_LOCAL_MEM flag. So what the heck happens with
a platform device without either of those flags set in this code:
int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
gfp_t mem_flags)
{
enum dma_data_direction dir;
int ret = 0;
/* Map the URB's buffers for DMA access.
* Lower level HCD code should use *_dma exclusively,
* unless it uses pio or talks to another transport,
* or uses the provided scatter gather list for bulk.
*/
if (usb_endpoint_xfer_control(&urb->ep->desc)) {
if (hcd->self.uses_pio_for_control)
return ret;
if (hcd->self.uses_dma) {
urb->setup_dma = dma_map_single(
hcd->self.controller,
urb->setup_packet,
sizeof(struct usb_ctrlrequest),
DMA_TO_DEVICE);
if (dma_mapping_error(hcd->self.controller,
urb->setup_dma))
return -EAGAIN;
urb->transfer_flags |= URB_SETUP_MAP_SINGLE;
} else if (hcd->driver->flags & HCD_LOCAL_MEM) {
ret = hcd_alloc_coherent(
urb->dev->bus, mem_flags,
&urb->setup_dma,
(void **)&urb->setup_packet,
sizeof(struct usb_ctrlrequest),
DMA_TO_DEVICE);
if (ret)
return ret;
urb->transfer_flags |= URB_SETUP_MAP_LOCAL;
}
}
As far as I can tell, that means the setup packet for control transfers
doesn't actually get mapped for DMA currently. With Xenia's patch it
will.
Does that mean xHCI platform devices have just been broken all this
time? Has this code been tested at all?
> If dma_set_mask() succeeds, for a given bitmask, it is guaranteed that
> the given bitmask is also supported for consistent DMA mappings.
> That is the reason why this patch does not add checks when setting the
> coherent_dma_mask.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> ---
>
> Differences from version 1 and 2:
>
> The dma_mask setup code was removed from the xhci_gen_setup()
> function defined in xhci.c and was placed in xhci_pci_setup(),
> defined in xhci-pci.c, and in xhci_plat_setup(), defined in
> xhci-plat.c. The dma mask setup code has to be called after
> the mapping of host controller registers and before the
> allocation of xhci memory buffers, so it was placed in the
> driver's 'reset' callback function before the call to
> xhci_gen_setup().
>
> Initialization of the dma_mask pointer was added for non-pci
> platforms, following a remark made by Andy Shevchenco.
When someone suggests a change to your patch, what some people do is
list them as a Cc, below your Signed-off-by line. Then, if they're
happy with your changes, they may respond with an Acked-by line.
Acked-by simply means that they have reviewed the patch, and it looks
good to them. The maintainer (or you, if you have to send a pull
request) will change the Cc line to an Acked-by line in the final patch.
So, this patch could have had the following lines:
Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
Cc: Andy Shevchenco <andy.shevchenko@xxxxxxxxx>
(Note that some people fail to respond to Ccs on patches, and their Cc
lines are just left in the final patch, instead of being converted to
Acked-by lines.)
> drivers/usb/host/xhci-pci.c | 19 ++++++++++++++++++-
> drivers/usb/host/xhci-plat.c | 26 ++++++++++++++++++++++++++
> drivers/usb/host/xhci.c | 17 -----------------
> 3 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index cc24e39..dff5a22 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -122,9 +122,26 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> static int xhci_pci_setup(struct usb_hcd *hcd)
> {
> struct xhci_hcd *xhci;
> - struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> + struct pci_dev *pdev;
> + struct device *dev;
> int retval;
>
> + dev = hcd->self.controller;
> + pdev = to_pci_dev(dev);
> +
> + if (usb_hcd_is_primary_hcd(hcd)) {
> + struct xhci_cap_regs __iomem *cap_regs;
> + u32 hcc_params;
checkpatch.pl complains about this line, and another line. Can you
please make sure to turn on checkpatch.pl in your .git/hooks/pre-commit
hook, and make sure it's executable and doesn't end in .sample?
sarah@xanatos:~/git/kernels/xhci$ git am ~/Maildir.fetchmail/.to-apply
Applying: xhci: fix dma mask setup in xhci.c
WARNING: please, no space before tabs
#18: FILE: drivers/usb/host/xhci-pci.c:133:
+^I^Istruct xhci_cap_regs __iomem ^I*cap_regs;$
WARNING: please, no space before tabs
#48: FILE: drivers/usb/host/xhci-plat.c:40:
+^I^Istruct xhci_cap_regs __iomem ^I*cap_regs;$
total: 0 errors, 2 warnings, 94 lines checked
NOTE: Ignored message types: CAMELCASE PREFER_PACKED
Your patch has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
As a style preference, I don't like to tab-align variable names, because
when variable type gets longer, you have to re-indent all of them, and
the diff looks messy. So please just format variable declarations like:
struct xhci_cap_regs __iomem *cap_regs;
u32 hcc_params;
Otherwise the patch looks fine.
> +
> + cap_regs = hcd->regs;
> + hcc_params = readl(&cap_regs->hcc_params);
> + if (HCC_64BIT_ADDR(hcc_params) &&
> + !dma_set_mask(dev, DMA_BIT_MASK(64))) {
> + dev_dbg(dev, "Enabling 64-bit DMA addresses.\n");
> + dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> + }
> + }
> +
> retval = xhci_gen_setup(hcd, xhci_pci_quirks);
> if (retval)
> return retval;
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 51e22bf..d718134 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -30,6 +30,32 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> /* called during probe() after chip reset completes */
> static int xhci_plat_setup(struct usb_hcd *hcd)
> {
> + struct device *dev;
> + int retval;
> +
> + dev = hcd->self.controller;
> + dev->dma_mask = &dev->dma_coherent_mask;
> +
> + if (usb_hcd_is_primary_hcd(hcd)) {
> + struct xhci_cap_regs __iomem *cap_regs;
> + u32 hcc_params;
> +
> + cap_regs = hcd->regs;
> + hcc_params = readl(&cap_regs->hcc_params);
> + if (HCC_64BIT_ADDR(hcc_params) &&
> + !dma_set_mask(dev, DMA_BIT_MASK(64))) {
> + dev_dbg(dev, "Enabling 64-bit DMA addresses.\n");
> + } else {
> + retval = dma_set_mask(dev, DMA_BIT_MASK(32));
> + if (retval) {
> + dev->dma_mask = NULL;
> + hcd->self.uses_dma = 0;
> + return retval;
> + }
> + }
> + hcd->self.uses_dma = 1;
> + }
> +
> return xhci_gen_setup(hcd, xhci_plat_quirks);
> }
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 189ca85..32e5493 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,14 +4819,6 @@ 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)) {
> - 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));
> - }
> -
> xhci_dbg(xhci, "Calling HCD init\n");
> /* Initialize HCD and host controller data structures. */
> retval = xhci_init(hcd);
> --
> 1.8.3.1
>
--
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
