| Message ID | 20191130165055.GA46622@sx9 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: Fix incorrect DMA allocations for local memory pool drivers | expand |
On Sat, Nov 30, 2019 at 05:50:55PM +0100, Fredrik Noring wrote: > Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of > guestimating DMA capabilities") where local memory USB drivers > erroneously allocate DMA memory instead of pool memory, causing > > OHCI Unrecoverable Error, disabled > HC died; cleaning up > > The order between hcd_uses_dma() and hcd->localmem_pool is now > arranged as in hcd_buffer_alloc() and hcd_buffer_free(), with the > test for hcd->localmem_pool placed first. > > As an alternative, one might consider adjusting hcd_uses_dma() with > > static inline bool hcd_uses_dma(struct usb_hcd *hcd) > { > - return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA); > + return IS_ENABLED(CONFIG_HAS_DMA) && > + (hcd->driver->flags & HCD_DMA) && > + (hcd->localmem_pool == NULL); > } > > One can also consider unsetting HCD_DMA for local memory pool drivers. That seems like a good idea to me, as the "local DMA pool" really isn't DMA in the traditional sense. The host has to copy data into it by MMIO, and it then is accessed by the device. But if the usb maintainers prefer your current variant that is ok with me as well: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Sat, Nov 30, 2019 at 05:50:55PM +0100, Fredrik Noring wrote: > Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of > guestimating DMA capabilities") where local memory USB drivers > erroneously allocate DMA memory instead of pool memory, causing > > OHCI Unrecoverable Error, disabled > HC died; cleaning up > Fixes: 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers Looks like you copied a bit too much text here for the fixes tag. Johan
Hi Christoph, > > One can also consider unsetting HCD_DMA for local memory pool drivers. > > That seems like a good idea to me, as the "local DMA pool" really isn't > DMA in the traditional sense. The host has to copy data into it by MMIO, > and it then is accessed by the device. Perhaps we can combine several enhancements, given that the local memory pool drivers frequently break with peculiar errors? For example: 1. Arrange localmem_pool and hcd_uses_dma() statements consistently. Inconsistent ordering was a source of this bug. 2. Make localmem_pool and hcd_uses_dma() mutually exclusive. Allocating DMA memory was a source of this bug. 3. Introduce hcd_uses_localmem_pool(), as show below, to have it on the same abstraction level as hcd_uses_dma(). The current localmem_pool pointer tests throughout the code are not quite as readable. A minor note: The commit sequence 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") 5d6ff300f011 ("usb/max3421: remove the dummy {un,}map_urb_for_dma methods") bd5defaee872 ("dma-mapping: remove is_device_dma_capable") cdfee5623290 ("driver core: initialize a default DMA mask for platform device") broke bisection because the first commit 7b81cb6bddd2 crashes with "DMA map on device without dma_mask", that is fixed in the fourth commit cdfee5623290. Too late to do anything about that now, though. Fredrik diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -423,6 +423,11 @@ static inline bool hcd_periodic_completion_in_progress(struct usb_hcd *hcd, return hcd->high_prio_bh.completing_ep == ep; } +static inline bool hcd_uses_localmem_pool(struct usb_hcd *hcd) +{ + return hcd->localmem_pool != NULL; +} + static inline bool hcd_uses_dma(struct usb_hcd *hcd) { return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA);
Hi Johan, > > Fixes: 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers > > Looks like you copied a bit too much text here for the fixes tag. Indeed, thanks for noticing! Fredrik
On Sat, Nov 30, 2019 at 05:50:55PM +0100, Fredrik Noring wrote: > Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of > guestimating DMA capabilities") where local memory USB drivers > erroneously allocate DMA memory instead of pool memory, causing > > OHCI Unrecoverable Error, disabled > HC died; cleaning up > > The order between hcd_uses_dma() and hcd->localmem_pool is now > arranged as in hcd_buffer_alloc() and hcd_buffer_free(), with the > test for hcd->localmem_pool placed first. > > As an alternative, one might consider adjusting hcd_uses_dma() with > > static inline bool hcd_uses_dma(struct usb_hcd *hcd) > { > - return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA); > + return IS_ENABLED(CONFIG_HAS_DMA) && > + (hcd->driver->flags & HCD_DMA) && > + (hcd->localmem_pool == NULL); > } > > One can also consider unsetting HCD_DMA for local memory pool drivers. > > Fixes: 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") > Cc: stable <stable@vger.kernel.org> > Signed-off-by: Fredrik Noring <noring@nocrew.org> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > drivers/usb/core/hcd.c | 42 +++++++++++++++++----------------- > drivers/usb/storage/scsiglue.c | 3 ++- > 2 files changed, 23 insertions(+), 22 deletions(-) This patch doesn't apply against 5.5-rc1, can you refresh it and resend? thanks, greg k-h
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index f225eaa98ff8..d0f45600b669 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1409,7 +1409,17 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, if (usb_endpoint_xfer_control(&urb->ep->desc)) { if (hcd->self.uses_pio_for_control) return ret; - if (hcd_uses_dma(hcd)) { + if (hcd->localmem_pool) { + 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; + } else if (hcd_uses_dma(hcd)) { if (is_vmalloc_addr(urb->setup_packet)) { WARN_ONCE(1, "setup packet is not dma capable\n"); return -EAGAIN; @@ -1427,23 +1437,22 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, urb->setup_dma)) return -EAGAIN; urb->transfer_flags |= URB_SETUP_MAP_SINGLE; - } else if (hcd->localmem_pool) { - 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; } } dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE; if (urb->transfer_buffer_length != 0 && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) { - if (hcd_uses_dma(hcd)) { + if (hcd->localmem_pool) { + ret = hcd_alloc_coherent( + urb->dev->bus, mem_flags, + &urb->transfer_dma, + &urb->transfer_buffer, + urb->transfer_buffer_length, + dir); + if (ret == 0) + urb->transfer_flags |= URB_MAP_LOCAL; + } else if (hcd_uses_dma(hcd)) { if (urb->num_sgs) { int n; @@ -1497,15 +1506,6 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, else urb->transfer_flags |= URB_DMA_MAP_SINGLE; } - } else if (hcd->localmem_pool) { - ret = hcd_alloc_coherent( - urb->dev->bus, mem_flags, - &urb->transfer_dma, - &urb->transfer_buffer, - urb->transfer_buffer_length, - dir); - if (ret == 0) - urb->transfer_flags |= URB_MAP_LOCAL; } if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL))) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index 54a3c8195c96..2adcabe060c5 100644 --- a/drivers/usb/storage/scsiglue.c +++ b/drivers/usb/storage/scsiglue.c @@ -135,7 +135,8 @@ static int slave_configure(struct scsi_device *sdev) * For such controllers we need to make sure the block layer sets * up bounce buffers in addressable memory. */ - if (!hcd_uses_dma(bus_to_hcd(us->pusb_dev->bus))) + if (!hcd_uses_dma(bus_to_hcd(us->pusb_dev->bus)) || + (bus_to_hcd(us->pusb_dev->bus)->localmem_pool != NULL)) blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_HIGH); /*
Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers erroneously allocate DMA memory instead of pool memory, causing OHCI Unrecoverable Error, disabled HC died; cleaning up The order between hcd_uses_dma() and hcd->localmem_pool is now arranged as in hcd_buffer_alloc() and hcd_buffer_free(), with the test for hcd->localmem_pool placed first. As an alternative, one might consider adjusting hcd_uses_dma() with static inline bool hcd_uses_dma(struct usb_hcd *hcd) { - return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA); + return IS_ENABLED(CONFIG_HAS_DMA) && + (hcd->driver->flags & HCD_DMA) && + (hcd->localmem_pool == NULL); } One can also consider unsetting HCD_DMA for local memory pool drivers. Fixes: 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers Signed-off-by: Fredrik Noring <noring@nocrew.org> --- drivers/usb/core/hcd.c | 42 +++++++++++++++++----------------- drivers/usb/storage/scsiglue.c | 3 ++- 2 files changed, 23 insertions(+), 22 deletions(-)