Message-ID: <20170803162334.GY30136@localhost>
Date:   Thu, 3 Aug 2017 18:23:34 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Johan Hovold <johan@...nel.org>, Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Andreas Färber <afaerber@...e.de>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alan Stern <stern@...land.harvard.edu>,
        stable <stable@...r.kernel.org>,
        Robin Murphy <robin.murphy@....com>,
        Sricharan R <sricharan@...eaurora.org>,
        Stefan Wahren <stefan.wahren@...e.com>
Subject: Re: [PATCH v3] dma-mapping: skip USB devices when configuring DMA
 during probe

On Thu, Aug 03, 2017 at 09:07:28AM -0700, Greg Kroah-Hartman wrote:
> On Thu, Aug 03, 2017 at 05:52:08PM +0200, 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
> > DMA 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, long-standing 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 prevents USB devices from enumerating when control transfers
> > fail:
> > 
> > 	dwc2 3f980000.usb: Cannot do DMA to address 0x000000003a166a00
> > 
> > Note that the aforementioned DMA-mask bug was benign for the HCD itself
> > as the dwc2 driver overwrites the mask previously set by
> > of_dma_configure() for the platform device in its probe callback. The
> > mask would only later get corrupted when the root-hub child device was
> > probed.
> > 
> > Fix this, and similar future problems, by adding a flag to struct device
> > which prevents driver core from calling dma_configure() during probe and
> > making sure it is set for USB devices.
> > 
> > Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices")
> > Cc: stable <stable@...r.kernel.org>	# 4.12
> > Cc: Robin Murphy <robin.murphy@....com>
> > Cc: Sricharan R <sricharan@...eaurora.org>
> > Cc: Stefan Wahren <stefan.wahren@...e.com>
> > Reported-by: Hans Verkuil <hverkuil@...all.nl>
> > Signed-off-by: Johan Hovold <johan@...nel.org>
> > ---
> > 
> > v3
> >  - add flag to struct device to prevent DMA configuration during probe instead
> >    of checking for the USB bus type, which is not available when USB is built
> >    as a module as noted by Alan
> >  - drop moderated rpi list from CC
> > 
> > v2
> >  - amend commit message and point out that the long-standing 30-bit DMA-mask
> >    bug was benign to the dwc2 HCD itself (Robin)
> >  - add and use a new dev_is_usb() helper (Robin)
> > 
> > 
> >  drivers/base/dma-mapping.c | 6 ++++++
> >  drivers/usb/core/usb.c     | 1 +
> >  include/linux/device.h     | 3 +++
> >  3 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
> > index b555ff9dd8fc..f9f703be0ad1 100644
> > --- a/drivers/base/dma-mapping.c
> > +++ b/drivers/base/dma-mapping.c
> > @@ -345,6 +345,9 @@ int dma_configure(struct device *dev)
> >  	enum dev_dma_attr attr;
> >  	int ret = 0;
> >  
> > +	if (dev->skip_dma_configure)
> > +		return 0;
> > +
> >  	if (dev_is_pci(dev)) {
> >  		bridge = pci_get_host_bridge_device(to_pci_dev(dev));
> >  		dma_dev = bridge;
> > @@ -369,6 +372,9 @@ int dma_configure(struct device *dev)
> >  
> >  void dma_deconfigure(struct device *dev)
> >  {
> > +	if (dev->skip_dma_configure)
> > +		return;
> > +
> >  	of_dma_deconfigure(dev);
> >  	acpi_dma_deconfigure(dev);
> >  }
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 17681d5638ac..2a85d905b539 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -588,6 +588,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
> >  	 * Note: calling dma_set_mask() on a USB device would set the
> >  	 * mask for the entire HCD, so don't do that.
> >  	 */
> > +	dev->dev.skip_dma_configure = true;
> >  	dev->dev.dma_mask = bus->sysdev->dma_mask;
> >  	dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> 
> Why do we need to set the mask and offset if we are not going to
> configure the DMA for this device?

Take a look at the comment just above in usb_alloc_dev() that was added
by commit b44bbc46a8bb ("usb: core: setup dma_pfn_offset for USB devices
and, interfaces").

We've been setting the dma_mask like this since before git, and the
offset since the above mentioned commit, and other subsystems apparently
expect them to be set.

What this patch does is just to prevent driver core from doing what the
comment explicitly says must not be done, namely to set the dma_mask for
a USB device, something which is clearly broken.

Long term we should probably fix this up in some way, but this
regression should be addressed first.

> I feel like this is a random device quirk that you are trying to work
> around in a generic way when we haven't seen any other broken hardware
> like this :)

This device, and that other DMA-mask bug, only allowed us to detect this
broken dma_mask manipulation, which should not have been done in the
first place.

In fact, I guess since commit 09515ef5ddad ("of/acpi: Configure dma
operations at probe time for platform/amba/pci bus devices") all
controllers would have their masks reset to DMA_BIT_MASK(32) whenever we
have an OF node defined for any of its ports. I should probably have
mentioned that in the commit message as well.

Thanks,
Johan

Powered by blists - more mailing lists