USB: fix usbmon and DMA mapping for scatter-gather URBs
This patch (as1368) fixes a rather obscure bug in usbmon: When tracing
URBs sent by the scatter-gather library, it accesses the data buffers
while they are still mapped for DMA.
The solution is to move the mapping and unmapping out of the s-g
library and into the usual place in hcd.c. This requires the addition
of new URB flag bits to describe the kind of mapping needed, since we
have to call dma_map_sg() if the HCD supports native scatter-gather
operation and dma_map_page() if it doesn't. The nice thing about
having the new flags is that they simplify the testing for unmapping.
The patch removes the only caller of usb_buffer_[un]map_sg(), so those
functions are #if'ed out. A later patch will remove them entirely.
As a result of this change, urb->sg will be set in situations where
it wasn't set previously. Hence the xhci and whci drivers are
adjusted to test urb->num_sgs instead, which retains its original
meaning and is nonzero only when the HCD has to handle a scatterlist.
Finally, even when a submission error occurs we don't want to hand
URBs to usbmon before they are unmapped. The submission path is
rearranged so that map_urb_for_dma() is called only for non-root-hub
URBs and unmap_urb_for_dma() is called immediately after a submission
error. This simplifies the error handling.
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: <stable@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 38d4700..6a05e69 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1259,6 +1259,51 @@
*dma_handle = 0;
}
+static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+ enum dma_data_direction dir;
+
+ if (urb->transfer_flags & URB_SETUP_MAP_SINGLE)
+ dma_unmap_single(hcd->self.controller,
+ urb->setup_dma,
+ sizeof(struct usb_ctrlrequest),
+ DMA_TO_DEVICE);
+ else if (urb->transfer_flags & URB_SETUP_MAP_LOCAL)
+ hcd_free_coherent(urb->dev->bus,
+ &urb->setup_dma,
+ (void **) &urb->setup_packet,
+ sizeof(struct usb_ctrlrequest),
+ DMA_TO_DEVICE);
+
+ dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+ if (urb->transfer_flags & URB_DMA_MAP_SG)
+ dma_unmap_sg(hcd->self.controller,
+ urb->sg->sg,
+ urb->num_sgs,
+ dir);
+ else if (urb->transfer_flags & URB_DMA_MAP_PAGE)
+ dma_unmap_page(hcd->self.controller,
+ urb->transfer_dma,
+ urb->transfer_buffer_length,
+ dir);
+ else if (urb->transfer_flags & URB_DMA_MAP_SINGLE)
+ dma_unmap_single(hcd->self.controller,
+ urb->transfer_dma,
+ urb->transfer_buffer_length,
+ dir);
+ else if (urb->transfer_flags & URB_MAP_LOCAL)
+ hcd_free_coherent(urb->dev->bus,
+ &urb->transfer_dma,
+ &urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ dir);
+
+ /* Make it safe to call this routine more than once */
+ urb->transfer_flags &= ~(URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL |
+ URB_DMA_MAP_SG | URB_DMA_MAP_PAGE |
+ URB_DMA_MAP_SINGLE | URB_MAP_LOCAL);
+}
+
static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
gfp_t mem_flags)
{
@@ -1270,8 +1315,6 @@
* unless it uses pio or talks to another transport,
* or uses the provided scatter gather list for bulk.
*/
- if (is_root_hub(urb->dev))
- return 0;
if (usb_endpoint_xfer_control(&urb->ep->desc)
&& !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP)) {
@@ -1284,6 +1327,7 @@
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,
@@ -1291,20 +1335,57 @@
(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 (ret == 0 && urb->transfer_buffer_length != 0
+ if (urb->transfer_buffer_length != 0
&& !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
if (hcd->self.uses_dma) {
- urb->transfer_dma = dma_map_single (
- hcd->self.controller,
- urb->transfer_buffer,
- urb->transfer_buffer_length,
- dir);
- if (dma_mapping_error(hcd->self.controller,
+ if (urb->num_sgs) {
+ int n = dma_map_sg(
+ hcd->self.controller,
+ urb->sg->sg,
+ urb->num_sgs,
+ dir);
+ if (n <= 0)
+ ret = -EAGAIN;
+ else
+ urb->transfer_flags |= URB_DMA_MAP_SG;
+ if (n != urb->num_sgs) {
+ urb->num_sgs = n;
+ urb->transfer_flags |=
+ URB_DMA_SG_COMBINED;
+ }
+ } else if (urb->sg) {
+ struct scatterlist *sg;
+
+ sg = (struct scatterlist *) urb->sg;
+ urb->transfer_dma = dma_map_page(
+ hcd->self.controller,
+ sg_page(sg),
+ sg->offset,
+ urb->transfer_buffer_length,
+ dir);
+ if (dma_mapping_error(hcd->self.controller,
urb->transfer_dma))
- return -EAGAIN;
+ ret = -EAGAIN;
+ else
+ urb->transfer_flags |= URB_DMA_MAP_PAGE;
+ } else {
+ urb->transfer_dma = dma_map_single(
+ hcd->self.controller,
+ urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ dir);
+ if (dma_mapping_error(hcd->self.controller,
+ urb->transfer_dma))
+ ret = -EAGAIN;
+ else
+ urb->transfer_flags |= URB_DMA_MAP_SINGLE;
+ }
} else if (hcd->driver->flags & HCD_LOCAL_MEM) {
ret = hcd_alloc_coherent(
urb->dev->bus, mem_flags,
@@ -1312,55 +1393,16 @@
&urb->transfer_buffer,
urb->transfer_buffer_length,
dir);
-
- if (ret && usb_endpoint_xfer_control(&urb->ep->desc)
- && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
- hcd_free_coherent(urb->dev->bus,
- &urb->setup_dma,
- (void **)&urb->setup_packet,
- sizeof(struct usb_ctrlrequest),
- DMA_TO_DEVICE);
+ if (ret == 0)
+ urb->transfer_flags |= URB_MAP_LOCAL;
}
+ if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
+ URB_SETUP_MAP_LOCAL)))
+ unmap_urb_for_dma(hcd, urb);
}
return ret;
}
-static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
-{
- enum dma_data_direction dir;
-
- if (is_root_hub(urb->dev))
- return;
-
- if (usb_endpoint_xfer_control(&urb->ep->desc)
- && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP)) {
- if (hcd->self.uses_dma)
- dma_unmap_single(hcd->self.controller, urb->setup_dma,
- sizeof(struct usb_ctrlrequest),
- DMA_TO_DEVICE);
- else if (hcd->driver->flags & HCD_LOCAL_MEM)
- hcd_free_coherent(urb->dev->bus, &urb->setup_dma,
- (void **)&urb->setup_packet,
- sizeof(struct usb_ctrlrequest),
- DMA_TO_DEVICE);
- }
-
- 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->self.uses_dma)
- dma_unmap_single(hcd->self.controller,
- urb->transfer_dma,
- urb->transfer_buffer_length,
- dir);
- else if (hcd->driver->flags & HCD_LOCAL_MEM)
- hcd_free_coherent(urb->dev->bus, &urb->transfer_dma,
- &urb->transfer_buffer,
- urb->transfer_buffer_length,
- dir);
- }
-}
-
/*-------------------------------------------------------------------------*/
/* may be called in any context with a valid urb->dev usecount
@@ -1389,21 +1431,20 @@
* URBs must be submitted in process context with interrupts
* enabled.
*/
- status = map_urb_for_dma(hcd, urb, mem_flags);
- if (unlikely(status)) {
- usbmon_urb_submit_error(&hcd->self, urb, status);
- goto error;
+
+ if (is_root_hub(urb->dev)) {
+ status = rh_urb_enqueue(hcd, urb);
+ } else {
+ status = map_urb_for_dma(hcd, urb, mem_flags);
+ if (likely(status == 0)) {
+ status = hcd->driver->urb_enqueue(hcd, urb, mem_flags);
+ if (unlikely(status))
+ unmap_urb_for_dma(hcd, urb);
+ }
}
- if (is_root_hub(urb->dev))
- status = rh_urb_enqueue(hcd, urb);
- else
- status = hcd->driver->urb_enqueue(hcd, urb, mem_flags);
-
if (unlikely(status)) {
usbmon_urb_submit_error(&hcd->self, urb, status);
- unmap_urb_for_dma(hcd, urb);
- error:
urb->hcpriv = NULL;
INIT_LIST_HEAD(&urb->urb_list);
atomic_dec(&urb->use_count);
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 619c44f..79d1cdf 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -259,9 +259,6 @@
kfree(io->urbs);
io->urbs = NULL;
}
- if (io->dev->dev.dma_mask != NULL)
- usb_buffer_unmap_sg(io->dev, usb_pipein(io->pipe),
- io->sg, io->nents);
io->dev = NULL;
}
@@ -364,7 +361,6 @@
{
int i;
int urb_flags;
- int dma;
int use_sg;
if (!io || !dev || !sg
@@ -378,21 +374,9 @@
io->pipe = pipe;
io->sg = sg;
io->nents = nents;
-
- /* not all host controllers use DMA (like the mainstream pci ones);
- * they can use PIO (sl811) or be software over another transport.
- */
- dma = (dev->dev.dma_mask != NULL);
- if (dma)
- io->entries = usb_buffer_map_sg(dev, usb_pipein(pipe),
- sg, nents);
- else
- io->entries = nents;
+ io->entries = nents;
/* initialize all the urbs we'll use */
- if (io->entries <= 0)
- return io->entries;
-
if (dev->bus->sg_tablesize > 0) {
io->urbs = kmalloc(sizeof *io->urbs, mem_flags);
use_sg = true;
@@ -404,8 +388,6 @@
goto nomem;
urb_flags = 0;
- if (dma)
- urb_flags |= URB_NO_TRANSFER_DMA_MAP;
if (usb_pipein(pipe))
urb_flags |= URB_SHORT_NOT_OK;
@@ -423,12 +405,13 @@
io->urbs[0]->complete = sg_complete;
io->urbs[0]->context = io;
+
/* A length of zero means transfer the whole sg list */
io->urbs[0]->transfer_buffer_length = length;
if (length == 0) {
for_each_sg(sg, sg, io->entries, i) {
io->urbs[0]->transfer_buffer_length +=
- sg_dma_len(sg);
+ sg->length;
}
}
io->urbs[0]->sg = io;
@@ -454,26 +437,16 @@
io->urbs[i]->context = io;
/*
- * Some systems need to revert to PIO when DMA is temporarily
- * unavailable. For their sakes, both transfer_buffer and
- * transfer_dma are set when possible.
- *
- * Note that if IOMMU coalescing occurred, we cannot
- * trust sg_page anymore, so check if S/G list shrunk.
+ * Some systems can't use DMA; they use PIO instead.
+ * For their sakes, transfer_buffer is set whenever
+ * possible.
*/
- if (io->nents == io->entries && !PageHighMem(sg_page(sg)))
+ if (!PageHighMem(sg_page(sg)))
io->urbs[i]->transfer_buffer = sg_virt(sg);
else
io->urbs[i]->transfer_buffer = NULL;
- if (dma) {
- io->urbs[i]->transfer_dma = sg_dma_address(sg);
- len = sg_dma_len(sg);
- } else {
- /* hc may use _only_ transfer_buffer */
- len = sg->length;
- }
-
+ len = sg->length;
if (length) {
len = min_t(unsigned, len, length);
length -= len;
@@ -481,6 +454,8 @@
io->entries = i + 1;
}
io->urbs[i]->transfer_buffer_length = len;
+
+ io->urbs[i]->sg = (struct usb_sg_request *) sg;
}
io->urbs[--i]->transfer_flags &= ~URB_NO_INTERRUPT;
}
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 2532a09..a760e46 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -333,9 +333,12 @@
is_out = usb_endpoint_dir_out(&ep->desc);
}
- /* Cache the direction for later use */
- urb->transfer_flags = (urb->transfer_flags & ~URB_DIR_MASK) |
- (is_out ? URB_DIR_OUT : URB_DIR_IN);
+ /* Clear the internal flags and cache the direction for later use */
+ urb->transfer_flags &= ~(URB_DIR_MASK | URB_DMA_MAP_SINGLE |
+ URB_DMA_MAP_PAGE | URB_DMA_MAP_SG | URB_MAP_LOCAL |
+ URB_SETUP_MAP_SINGLE | URB_SETUP_MAP_LOCAL |
+ URB_DMA_SG_COMBINED);
+ urb->transfer_flags |= (is_out ? URB_DIR_OUT : URB_DIR_IN);
if (xfertype != USB_ENDPOINT_XFER_CONTROL &&
dev->state < USB_STATE_CONFIGURED)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 097172e..8180ce5 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -881,6 +881,7 @@
EXPORT_SYMBOL_GPL(usb_buffer_unmap);
#endif /* 0 */
+#if 0
/**
* usb_buffer_map_sg - create scatterlist DMA mapping(s) for an endpoint
* @dev: device to which the scatterlist will be mapped
@@ -924,6 +925,7 @@
is_in ? DMA_FROM_DEVICE : DMA_TO_DEVICE) ? : -ENOMEM;
}
EXPORT_SYMBOL_GPL(usb_buffer_map_sg);
+#endif
/* XXX DISABLED, no users currently. If you wish to re-enable this
* XXX please determine whether the sync is to transfer ownership of
@@ -960,6 +962,7 @@
EXPORT_SYMBOL_GPL(usb_buffer_dmasync_sg);
#endif
+#if 0
/**
* usb_buffer_unmap_sg - free DMA mapping(s) for a scatterlist
* @dev: device to which the scatterlist will be mapped
@@ -985,6 +988,7 @@
is_in ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
}
EXPORT_SYMBOL_GPL(usb_buffer_unmap_sg);
+#endif
/* To disable USB, kernel command line is 'nousb' not 'usbcore.nousb' */
#ifdef MODULE
diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c
index 141d049..b388dd1 100644
--- a/drivers/usb/host/whci/qset.c
+++ b/drivers/usb/host/whci/qset.c
@@ -646,7 +646,7 @@
wurb->urb = urb;
INIT_WORK(&wurb->dequeue_work, urb_dequeue_work);
- if (urb->sg) {
+ if (urb->num_sgs) {
ret = qset_add_urb_sg(whc, qset, urb, mem_flags);
if (ret == -EINVAL) {
qset_free_stds(qset, urb);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 407d33f..c1359ed 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1962,7 +1962,7 @@
int running_total, trb_buff_len, ret;
u64 addr;
- if (urb->sg)
+ if (urb->num_sgs)
return queue_bulk_sg_tx(xhci, mem_flags, urb, slot_id, ep_index);
ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ddf7f9a..8a7968d 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -416,7 +416,7 @@
} else {
/* If IOMMU coalescing occurred, we cannot trust sg_page */
- if (urb->sg->nents != urb->num_sgs) {
+ if (urb->transfer_flags & URB_DMA_SG_COMBINED) {
*flag = 'D';
return length;
}
diff --git a/drivers/usb/mon/mon_text.c b/drivers/usb/mon/mon_text.c
index 4d0be13..d562602 100644
--- a/drivers/usb/mon/mon_text.c
+++ b/drivers/usb/mon/mon_text.c
@@ -161,9 +161,7 @@
} else {
struct scatterlist *sg = urb->sg->sg;
- /* If IOMMU coalescing occurred, we cannot trust sg_page */
- if (urb->sg->nents != urb->num_sgs ||
- PageHighMem(sg_page(sg)))
+ if (PageHighMem(sg_page(sg)))
return 'D';
/* For the text interface we copy only the first sg buffer */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 739f1fd..9983302 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -965,10 +965,19 @@
* needed */
#define URB_FREE_BUFFER 0x0100 /* Free transfer buffer with the URB */
+/* The following flags are used internally by usbcore and HCDs */
#define URB_DIR_IN 0x0200 /* Transfer from device to host */
#define URB_DIR_OUT 0
#define URB_DIR_MASK URB_DIR_IN
+#define URB_DMA_MAP_SINGLE 0x00010000 /* Non-scatter-gather mapping */
+#define URB_DMA_MAP_PAGE 0x00020000 /* HCD-unsupported S-G */
+#define URB_DMA_MAP_SG 0x00040000 /* HCD-supported S-G */
+#define URB_MAP_LOCAL 0x00080000 /* HCD-local-memory mapping */
+#define URB_SETUP_MAP_SINGLE 0x00100000 /* Setup packet DMA mapped */
+#define URB_SETUP_MAP_LOCAL 0x00200000 /* HCD-local setup packet */
+#define URB_DMA_SG_COMBINED 0x00400000 /* S-G entries were combined */
+
struct usb_iso_packet_descriptor {
unsigned int offset;
unsigned int length; /* expected length */