[PATCH] dma-mapping: skip USB devices when configuring DMA during probe

[PATCH] dma-mapping: skip USB devices when configuring DMA during probe

[Johan Hovold]

USB devices use the DMA mask and offset of the controller, which have
already been setup when a device is probed. Note that modifying the mask
of a USB device would change the mask for the controller (and all
devices on the bus) as the mask is literally shared.

Since commit 2bf698671205 ("USB: of: fix root-hub device-tree node
handling"), of_dma_configure() would be called also for root hubs, which
use the device node of the controller. A separate bug that makes
of_dma_configure() generate a 30-bit DMA mask from the RPI3's
"dma-ranges" would thus set a broken mask also for the controller. This
in turn leads to USB devices failing to enumerate when control transfers
fail:

	dwc2 3f980000.usb: Cannot do DMA to address 0x000000003a166a00

Fix this, and similar future problems, by simply skipping USB devices
when dma_configure() is called during probe.

Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices")
Cc: stable <stable@vger.kernel.org>	# 4.12
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Sricharan R <sricharan@codeaurora.org>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/base/dma-mapping.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index b555ff9dd8fc..c6cde7e79599 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -13,6 +13,7 @@
 #include <linux/gfp.h>
 #include <linux/of_device.h>
 #include <linux/slab.h>
+#include <linux/usb.h>
 #include <linux/vmalloc.h>
 
 /*
@@ -345,6 +346,10 @@ int dma_configure(struct device *dev)
 	enum dev_dma_attr attr;
 	int ret = 0;
 
+	/* USB devices share the controller's mask. */
+	if (dev->bus == &usb_bus_type)
+		return 0;
+
 	if (dev_is_pci(dev)) {
 		bridge = pci_get_host_bridge_device(to_pci_dev(dev));
 		dma_dev = bridge;
@@ -369,6 +374,9 @@ int dma_configure(struct device *dev)
 
 void dma_deconfigure(struct device *dev)
 {
+	if (dev->bus == &usb_bus_type)
+		return;
+
 	of_dma_deconfigure(dev);
 	acpi_dma_deconfigure(dev);
 }
-- 
2.13.3

Re: [PATCH] dma-mapping: skip USB devices when configuring DMA during probe

[Robin Murphy]

Hi Johan,

On 03/08/17 11:05, Johan Hovold wrote:
> USB devices use the DMA mask and offset of the controller, which have
> already been setup when a device is probed. Note that modifying the mask
> of a USB device would change the mask for the controller (and all
> devices on the bus) as the mask is literally shared.

If I understand the situation correctly, some USB devices have to
pretend to be DMA masters in order to interact properly with subsystems
that make DMA mapping calls directly, because those subsystems don't
have access to the 'real' DMA master sysdev (and shouldn't have to know
anyway). If that is the case, then there's really a much more general
problem, because correct DMA API operation can depend on other things
like IOMMU stuff and archdata which cannot be arbitrarily copied or
shared between devices.

What with things like the DWC3 glue layer device mess as well, the
feeling I'm getting is that this probably needs addressing at the driver
core/DMA API level - if we had some flag in struct device to say "I can
effectively do DMA, but proxied through my parent", the DMA API could
pick that up and internally chase down the correct DMA master device,
and we could then probably clean up a lot of the "sysdev"-type gunk in
various driver layers as well.

For expedience of keeping the existing hack working, though, I think
this patch is OK.

> Since commit 2bf698671205 ("USB: of: fix root-hub device-tree node
> handling"), of_dma_configure() would be called also for root hubs, which
> use the device node of the controller. A separate bug that makes
> of_dma_configure() generate a 30-bit DMA mask from the RPI3's
> "dma-ranges" would thus set a broken mask also for the controller. This
> in turn leads to USB devices failing to enumerate when control transfers
> fail:

If we're calling out that (long-standing) bug in particular, it might be
worth clarifying that it's benign for the HCD itself because the dwc2
driver still sets a 32-bit mask afterwards on probe - the breakage is
specifically from the subsequent of_dma_configure() call with the root
hub resetting the HCD's mask again *after* the HCD driver has configured
it (and started to perform DMA), which is just fundamentally wrong
regardless of what it ends up set to.

> 	dwc2 3f980000.usb: Cannot do DMA to address 0x000000003a166a00
> 
> Fix this, and similar future problems, by simply skipping USB devices
> when dma_configure() is called during probe.
> 
> Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices")
> Cc: stable <stable@vger.kernel.org>	# 4.12
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Sricharan R <sricharan@codeaurora.org>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/base/dma-mapping.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
> index b555ff9dd8fc..c6cde7e79599 100644
> --- a/drivers/base/dma-mapping.c
> +++ b/drivers/base/dma-mapping.c
> @@ -13,6 +13,7 @@
>  #include <linux/gfp.h>
>  #include <linux/of_device.h>
>  #include <linux/slab.h>
> +#include <linux/usb.h>
>  #include <linux/vmalloc.h>
>  
>  /*
> @@ -345,6 +346,10 @@ int dma_configure(struct device *dev)
>  	enum dev_dma_attr attr;
>  	int ret = 0;
>  
> +	/* USB devices share the controller's mask. */
> +	if (dev->bus == &usb_bus_type)

Maybe a dev_is_usb() helper to mimic dev_is_pci()?

Robin.

> +		return 0;
> +
>  	if (dev_is_pci(dev)) {
>  		bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>  		dma_dev = bridge;
> @@ -369,6 +374,9 @@ int dma_configure(struct device *dev)
>  
>  void dma_deconfigure(struct device *dev)
>  {
> +	if (dev->bus == &usb_bus_type)
> +		return;
> +
>  	of_dma_deconfigure(dev);
>  	acpi_dma_deconfigure(dev);
>  }
> 

Re: [PATCH] dma-mapping: skip USB devices when configuring DMA during probe

[Johan Hovold]

On Thu, Aug 03, 2017 at 12:50:20PM +0100, Robin Murphy wrote:
> Hi Johan,
> 
> On 03/08/17 11:05, Johan Hovold wrote:
> > USB devices use the DMA mask and offset of the controller, which have
> > already been setup when a device is probed. Note that modifying the mask
> > of a USB device would change the mask for the controller (and all
> > devices on the bus) as the mask is literally shared.
> 
> If I understand the situation correctly, some USB devices have to
> pretend to be DMA masters in order to interact properly with subsystems
> that make DMA mapping calls directly, because those subsystems don't
> have access to the 'real' DMA master sysdev (and shouldn't have to know
> anyway). If that is the case, then there's really a much more general
> problem, because correct DMA API operation can depend on other things
> like IOMMU stuff and archdata which cannot be arbitrarily copied or
> shared between devices.

Right, take a look at usb_alloc_dev() and commit b44bbc46a8bb ("usb:
core: setup dma_pfn_offset for USB devices and, interfaces") for some
background. Note that we've been setting dma_mask this way since before
git.

> What with things like the DWC3 glue layer device mess as well, the
> feeling I'm getting is that this probably needs addressing at the driver
> core/DMA API level - if we had some flag in struct device to say "I can
> effectively do DMA, but proxied through my parent", the DMA API could
> pick that up and internally chase down the correct DMA master device,
> and we could then probably clean up a lot of the "sysdev"-type gunk in
> various driver layers as well.
> 
> For expedience of keeping the existing hack working, though, I think
> this patch is OK.

Yes, we should fix the immediate regression first.

> > Since commit 2bf698671205 ("USB: of: fix root-hub device-tree node
> > handling"), of_dma_configure() would be called also for root hubs, which
> > use the device node of the controller. A separate bug that makes
> > of_dma_configure() generate a 30-bit DMA mask from the RPI3's
> > "dma-ranges" would thus set a broken mask also for the controller. This
> > in turn leads to USB devices failing to enumerate when control transfers
> > fail:
> 
> If we're calling out that (long-standing) bug in particular, it might be
> worth clarifying that it's benign for the HCD itself because the dwc2
> driver still sets a 32-bit mask afterwards on probe - the breakage is
> specifically from the subsequent of_dma_configure() call with the root
> hub resetting the HCD's mask again *after* the HCD driver has configured
> it (and started to perform DMA), which is just fundamentally wrong
> regardless of what it ends up set to.

Good point, I'll amend this paragraph.

> > 	dwc2 3f980000.usb: Cannot do DMA to address 0x000000003a166a00
> > 
> > Fix this, and similar future problems, by simply skipping USB devices
> > when dma_configure() is called during probe.
> > 
> > Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices")
> > Cc: stable <stable@vger.kernel.org>	# 4.12
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Sricharan R <sricharan@codeaurora.org>
> > Cc: Stefan Wahren <stefan.wahren@i2se.com>
> > Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/base/dma-mapping.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
> > index b555ff9dd8fc..c6cde7e79599 100644
> > --- a/drivers/base/dma-mapping.c
> > +++ b/drivers/base/dma-mapping.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/gfp.h>
> >  #include <linux/of_device.h>
> >  #include <linux/slab.h>
> > +#include <linux/usb.h>
> >  #include <linux/vmalloc.h>
> >  
> >  /*
> > @@ -345,6 +346,10 @@ int dma_configure(struct device *dev)
> >  	enum dev_dma_attr attr;
> >  	int ret = 0;
> >  
> > +	/* USB devices share the controller's mask. */
> > +	if (dev->bus == &usb_bus_type)
> 
> Maybe a dev_is_usb() helper to mimic dev_is_pci()?

Yeah, I considered that but figured it wasn't essential for a minimal
patch marked for stable, but then again, such a macro would be
straight-forward enough. I'll respin.

> > +		return 0;
> > +
> >  	if (dev_is_pci(dev)) {
> >  		bridge = pci_get_host_bridge_device(to_pci_dev(dev));
> >  		dma_dev = bridge;
> > @@ -369,6 +374,9 @@ int dma_configure(struct device *dev)
> >  
> >  void dma_deconfigure(struct device *dev)
> >  {
> > +	if (dev->bus == &usb_bus_type)
> > +		return;
> > +
> >  	of_dma_deconfigure(dev);
> >  	acpi_dma_deconfigure(dev);
> >  }

Thanks,
Johan

Re: [PATCH] dma-mapping: skip USB devices when configuring DMA during probe

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]

Hi Johan,

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on v4.13-rc3 next-20170803]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Johan-Hovold/dma-mapping-skip-USB-devices-when-configuring-DMA-during-probe/20170804-014620
config: microblaze-mmu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=microblaze 

All errors (new ones prefixed by >>):

   drivers/base/dma-mapping.o: In function `dma_configure':
>> drivers/base/dma-mapping.c:350: undefined reference to `usb_bus_type'
   drivers/base/dma-mapping.o: In function `dma_deconfigure':
   drivers/base/dma-mapping.c:377: undefined reference to `usb_bus_type'
   net/sunrpc/stats.o: In function `rpc_print_iostats':
   net/sunrpc/stats.c:206: undefined reference to `_GLOBAL_OFFSET_TABLE_'
   scripts/link-vmlinux.sh: line 93: 76193 Segmentation fault      ${LD} ${LDFLAGS} ${LDFLAGS_vmlinux} -o ${2} -T ${lds} ${objects}

vim +350 drivers/base/dma-mapping.c

   342	
   343	int dma_configure(struct device *dev)
   344	{
   345		struct device *bridge = NULL, *dma_dev = dev;
   346		enum dev_dma_attr attr;
   347		int ret = 0;
   348	
   349		/* USB devices share the controller's mask. */
 > 350		if (dev->bus == &usb_bus_type)
   351			return 0;
   352	
   353		if (dev_is_pci(dev)) {
   354			bridge = pci_get_host_bridge_device(to_pci_dev(dev));
   355			dma_dev = bridge;
   356			if (IS_ENABLED(CONFIG_OF) && dma_dev->parent &&
   357			    dma_dev->parent->of_node)
   358				dma_dev = dma_dev->parent;
   359		}
   360	
   361		if (dma_dev->of_node) {
   362			ret = of_dma_configure(dev, dma_dev->of_node);
   363		} else if (has_acpi_companion(dma_dev)) {
   364			attr = acpi_get_dma_attr(to_acpi_device_node(dma_dev->fwnode));
   365			if (attr != DEV_DMA_NOT_SUPPORTED)
   366				ret = acpi_dma_configure(dev, attr);
   367		}
   368	
   369		if (bridge)
   370			pci_put_host_bridge_device(bridge);
   371	
   372		return ret;
   373	}
   374	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12949 bytes --]

Re: [PATCH] dma-mapping: skip USB devices when configuring DMA during probe

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

Hi Johan,

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on v4.13-rc3 next-20170803]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Johan-Hovold/dma-mapping-skip-USB-devices-when-configuring-DMA-during-probe/20170804-014620
config: sparc-defconfig (attached as .config)
compiler: sparc-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc 

All errors (new ones prefixed by >>):

   drivers/base/dma-mapping.o: In function `dma_configure':
>> dma-mapping.c:(.text+0x720): undefined reference to `usb_bus_type'
   dma-mapping.c:(.text+0x728): undefined reference to `usb_bus_type'
   drivers/base/dma-mapping.o: In function `dma_deconfigure':
   dma-mapping.c:(.text+0x7d0): undefined reference to `usb_bus_type'
   dma-mapping.c:(.text+0x7d4): undefined reference to `usb_bus_type'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12096 bytes --]

Re: [PATCH] dma-mapping: skip USB devices when configuring DMA during probe

[Johan Hovold]

On Fri, Aug 04, 2017 at 03:40:42AM +0800, kbuild test robot wrote:
> Hi Johan,
> 
> [auto build test ERROR on driver-core/driver-core-testing]
> [also build test ERROR on v4.13-rc3 next-20170803]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Johan-Hovold/dma-mapping-skip-USB-devices-when-configuring-DMA-during-probe/20170804-014620
> config: microblaze-mmu_defconfig (attached as .config)
> compiler: microblaze-linux-gcc (GCC) 6.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=microblaze 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/base/dma-mapping.o: In function `dma_configure':
> >> drivers/base/dma-mapping.c:350: undefined reference to `usb_bus_type'
>    drivers/base/dma-mapping.o: In function `dma_deconfigure':
>    drivers/base/dma-mapping.c:377: undefined reference to `usb_bus_type'
>    net/sunrpc/stats.o: In function `rpc_print_iostats':
>    net/sunrpc/stats.c:206: undefined reference to `_GLOBAL_OFFSET_TABLE_'
>    scripts/link-vmlinux.sh: line 93: 76193 Segmentation fault      ${LD} ${LDFLAGS} ${LDFLAGS_vmlinux} -o ${2} -T ${lds} ${objects}

Thanks for the report. Alan Stern pointed this out during review, so
this patch has already been superseded. 

Johan