usb: fix number of mapped SG DMA entries

PATCH In review Clemens Ladisch 3 participants 7 messages Dec 3, 2011 – Dec 15, 2011

Subsystems (4)

USB USB xHCI driver USB ehci driver USB uhci driver

lore

Share & tools

lore

Clemens Ladisch

author

2011-12-03 22:41 UTC #0 lore raw

PATCH 1/1 usb: fix number of mapped SG DMA entries

Add a new field num_mapped_sgs to struct urb so that we have a place to store the number of mapped entries and can also retain the original value of entries in num_sgs. Previously, usb_hcd_map_urb_for_dma() would overwrite this with the number of mapped entries, which would break dma_unmap_sg() because it requires the original number of entries.

This fixes warnings like the following when using USB storage devices:

 ------------[ cut here ]------------
 WARNING: at lib/dma-debug.c:902 check_unmap+0x4e4/0x695()
 ehci_hcd 0000:00:12.2: DMA-API: device driver frees DMA sg list with different entry count [map count=4] [unmap count=1]
 Modules linked in: ohci_hcd ehci_hcd
 Pid: 0, comm: kworker/0:1 Not tainted 3.2.0-rc2+ #319
 Call Trace:
  <IRQ>  [<ffffffff81036d3b>] warn_slowpath_common+0x80/0x98
  [<ffffffff81036de7>] warn_slowpath_fmt+0x41/0x43
  [<ffffffff811fa5ae>] check_unmap+0x4e4/0x695
  [<ffffffff8105e92c>] ? trace_hardirqs_off+0xd/0xf
  [<ffffffff8147208b>] ? _raw_spin_unlock_irqrestore+0x33/0x50
  [<ffffffff811fa84a>] debug_dma_unmap_sg+0xeb/0x117
  [<ffffffff8137b02f>] usb_hcd_unmap_urb_for_dma+0x71/0x188
  [<ffffffff8137b166>] unmap_urb_for_dma+0x20/0x22
  [<ffffffff8137b1c5>] usb_hcd_giveback_urb+0x5d/0xc0
  [<ffffffffa0000d02>] ehci_urb_done+0xf7/0x10c [ehci_hcd]
  [<ffffffffa0001140>] qh_completions+0x429/0x4bd [ehci_hcd]
  [<ffffffffa000340a>] ehci_work+0x95/0x9c0 [ehci_hcd]
  ...
 ---[ end trace f29ac88a5a48c580 ]---
 Mapped at:
  [<ffffffff811faac4>] debug_dma_map_sg+0x45/0x139
  [<ffffffff8137bc0b>] usb_hcd_map_urb_for_dma+0x22e/0x478
  [<ffffffff8137c494>] usb_hcd_submit_urb+0x63f/0x6fa
  [<ffffffff8137d01c>] usb_submit_urb+0x2c7/0x2de
  [<ffffffff8137dcd4>] usb_sg_wait+0x55/0x161

Signed-off-by Clemens Ladisch <[email protected]>

---
 drivers/usb/core/hcd.c       |    5 ++---
 drivers/usb/host/ehci-q.c    |    2 +-
 drivers/usb/host/uhci-q.c    |    2 +-
 drivers/usb/host/whci/qset.c |    4 ++--
 drivers/usb/host/xhci-ring.c |    4 ++--
 include/linux/usb.h          |    1 +
 6 files changed, 9 insertions(+), 9 deletions(-)

drivers/usb/core/hcd.c +2 -3

index 13222d3..179e364 100644
@@ -1412,11 +1412,10 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 					ret = -EAGAIN;
 				else
 					urb->transfer_flags |= URB_DMA_MAP_SG;
+				urb->num_mapped_sgs = n;
-				if (n != urb->num_sgs) {
+				if (n != urb->num_sgs)
-					urb->num_sgs = n;
 					urb->transfer_flags |=
 							URB_DMA_SG_COMBINED;
-				}
 			} else if (urb->sg) {
 				struct scatterlist *sg = urb->sg;
 				urb->transfer_dma = dma_map_page(

drivers/usb/host/ehci-q.c +1 -1

index 4e4066c..fef1db3 100644
@@ -647,7 +647,7 @@ qh_urb_transaction (
 	/*
 	 * data transfer stage:  buffer setup
 	 */
-	i = urb->num_sgs;
+	i = urb->num_mapped_sgs;
 	if (len > 0 && i > 0) {
 		sg = urb->sg;
 		buf = sg_dma_address(sg);

drivers/usb/host/uhci-q.c +1 -1

index f6ca80e..d2c6f5a 100644
@@ -943,7 +943,7 @@ static int uhci_submit_common(struct uhci_hcd *uhci, struct urb *urb,
 	if (usb_pipein(urb->pipe))
 		status |= TD_CTRL_SPD;
 
-	i = urb->num_sgs;
+	i = urb->num_mapped_sgs;
 	if (len > 0 && i > 0) {
 		sg = urb->sg;
 		data = sg_dma_address(sg);

drivers/usb/host/whci/qset.c +2 -2

index d6e1754..a91712c 100644
@@ -443,7 +443,7 @@ static int qset_add_urb_sg(struct whc *whc, struct whc_qset *qset, struct urb *u
 
 	remaining = urb->transfer_buffer_length;
 
-	for_each_sg(urb->sg, sg, urb->num_sgs, i) {
+	for_each_sg(urb->sg, sg, urb->num_mapped_sgs, i) {
 		dma_addr_t dma_addr;
 		size_t dma_remaining;
 		dma_addr_t sp, ep;

@@ -561,7 +561,7 @@ static int qset_add_urb_sg_linearize(struct whc *whc, struct whc_qset *qset,
 
 	remaining = urb->transfer_buffer_length;
 
-	for_each_sg(urb->sg, sg, urb->num_sgs, i) {
+	for_each_sg(urb->sg, sg, urb->num_mapped_sgs, i) {
 		size_t len;
 		size_t sg_remaining;
 		void *orig;

drivers/usb/host/xhci-ring.c +2 -2

index 9f1d4b1..d28c586 100644
@@ -2561,7 +2561,7 @@ static unsigned int count_sg_trbs_needed(struct xhci_hcd *xhci, struct urb *urb)
 	struct scatterlist *sg;
 
 	sg = NULL;
-	num_sgs = urb->num_sgs;
+	num_sgs = urb->num_mapped_sgs;
 	temp = urb->transfer_buffer_length;
 
 	xhci_dbg(xhci, "count sg list trbs: \n");

@@ -2745,7 +2745,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		return -EINVAL;
 
 	num_trbs = count_sg_trbs_needed(xhci, urb);
-	num_sgs = urb->num_sgs;
+	num_sgs = urb->num_mapped_sgs;
 	total_packet_count = roundup(urb->transfer_buffer_length,
 			usb_endpoint_maxp(&urb->ep->desc));

include/linux/usb.h +1 -0

index d3d0c13..7503352 100644
@@ -1221,6 +1221,7 @@ struct urb {
 	void *transfer_buffer;		/* (in) associated data buffer */
 	dma_addr_t transfer_dma;	/* (in) dma addr for transfer_buffer */
 	struct scatterlist *sg;		/* (in) scatter gather buffer list */
+	int num_mapped_sgs;		/* (internal) mapped sg entries */
 	int num_sgs;			/* (in) number of entries in the sg list */
 	u32 transfer_buffer_length;	/* (in) data buffer length */
 	u32 actual_length;		/* (return) actual transfer length */

Alan Stern

maintainer

2011-12-04 16:59 UTC #1 lore raw

Quoted 10 lines · Clemens Ladisch

Add a new field num_mapped_sgs to struct urb so that we have a place to store the number of mapped entries and can also retain the original value of entries in num_sgs. Previously, usb_hcd_map_urb_for_dma() would overwrite this with the number of mapped entries, which would break dma_unmap_sg() because it requires the original number of entries.
This fixes warnings like the following when using USB storage devices:
 ------------[ cut here ]------------
 WARNING: at lib/dma-debug.c:902 check_unmap+0x4e4/0x695()
 ehci_hcd 0000:00:12.2: DMA-API: device driver frees DMA sg list with different entry count [map count=4] [unmap count=1]

Interesting. Do you have any idea why this warning didn't show up earlier? Is there perhaps a Kconfig option I ought to be using?

Alan Stern

Clemens Ladisch

author

2011-12-04 17:05 UTC #2 lore raw

Quoted 7 lines · Alan Stern

Quoted 6 lines · Clemens Ladisch
Add a new field num_mapped_sgs to struct urb so that we have a place to store the number of mapped entries and can also retain the original value of entries in num_sgs. Previously, usb_hcd_map_urb_for_dma() would overwrite this with the number of mapped entries, which would break dma_unmap_sg() because it requires the original number of entries.

Quoted 7 lines

Quoted 4 lines
This fixes warnings like the following when using USB storage devices:
 ------------[ cut here ]------------
 WARNING: at lib/dma-debug.c:902 check_unmap+0x4e4/0x695()
 ehci_hcd 0000:00:12.2: DMA-API: device driver frees DMA sg list with different entry count [map count=4] [unmap count=1]
Interesting. Do you have any idea why this warning didn't show up earlier? Is there perhaps a Kconfig option I ought to be using?

CONFIG_DMA_API_DEBUG; its help warns of performance degradation.

Regards,
Clemens

Sebastian Andrzej Siewior

2011-12-15 12:39 UTC #3 lore raw

In reply to Alan Stern

Quoted 8 lines · * Alan Stern | 2011-12-04 11:59:32 [-0500]:

Quoted 6 lines · Clemens Ladisch
Add a new field num_mapped_sgs to struct urb so that we have a place to store the number of mapped entries and can also retain the original value of entries in num_sgs. Previously, usb_hcd_map_urb_for_dma() would overwrite this with the number of mapped entries, which would break dma_unmap_sg() because it requires the original number of entries.

Quoted 7 lines

Quoted 4 lines
This fixes warnings like the following when using USB storage devices:
 ------------[ cut here ]------------
 WARNING: at lib/dma-debug.c:902 check_unmap+0x4e4/0x695()
 ehci_hcd 0000:00:12.2: DMA-API: device driver frees DMA sg list with different entry count [map count=4] [unmap count=1]
Interesting. Do you have any idea why this warning didn't show up earlier? Is there perhaps a Kconfig option I ought to be using?

My question here is why don't we map the complete sg list but only one entry. What happens to the remaining few sg list entries? Don't we sent too less data?
Is there anything special you do to get into this kind of situation? Like 16GiB of memory on 32bit with highmem and a brutal disk/io test case so?

Quoted 1 line

Alan Stern

Sebastian

Clemens Ladisch

author

2011-12-15 15:36 UTC #4 lore raw

Quoted 13 lines · Sebastian Andrzej Siewior

Quoted 12 lines · * Alan Stern | 2011-12-04 11:59:32 [-0500]:
Quoted 10 lines · Clemens Ladisch
Add a new field num_mapped_sgs to struct urb so that we have a place to store the number of mapped entries and can also retain the original value of entries in num_sgs. Previously, usb_hcd_map_urb_for_dma() would overwrite this with the number of mapped entries, which would break dma_unmap_sg() because it requires the original number of entries.
This fixes warnings like the following when using USB storage devices:
 ------------[ cut here ]------------
 WARNING: at lib/dma-debug.c:902 check_unmap+0x4e4/0x695()
 ehci_hcd 0000:00:12.2: DMA-API: device driver frees DMA sg list with different entry count [map count=4] [unmap count=1]

Quoted 6 lines

Quoted 2 lines
Interesting. Do you have any idea why this warning didn't show up earlier? Is there perhaps a Kconfig option I ought to be using?
My question here is why don't we map the complete sg list but only one entry. What happens to the remaining few sg list entries? Don't we sent too less data?

Documentation/DMA-API.txt says:

| Returns: the number of physical segments mapped (this may be shorter
| than <nents> passed in if some elements of the scatter/gather list are
| physically or virtually adjacent and an IOMMU maps them with a single
| entry).

Quoted 1 line

Is there anything special you do to get into this kind of situation?

It appears that enabling CONFIG_DMA_API_DEBUG is quite special. :o)

Quoted 2 lines

Like 16GiB of memory on 32bit with highmem and a brutal disk/io test case so?

Plain x86-64; very few file operations done by hand.

Regards,
Clemens

Alan Stern

maintainer

2011-12-15 16:01 UTC #5 lore raw

In reply to Sebastian Andrzej Siewior

Quoted 15 lines · Sebastian Andrzej Siewior

Quoted 13 lines · * Alan Stern | 2011-12-04 11:59:32 [-0500]:
Quoted 10 lines · Clemens Ladisch
Add a new field num_mapped_sgs to struct urb so that we have a place to store the number of mapped entries and can also retain the original value of entries in num_sgs. Previously, usb_hcd_map_urb_for_dma() would overwrite this with the number of mapped entries, which would break dma_unmap_sg() because it requires the original number of entries.
This fixes warnings like the following when using USB storage devices:
 ------------[ cut here ]------------
 WARNING: at lib/dma-debug.c:902 check_unmap+0x4e4/0x695()
 ehci_hcd 0000:00:12.2: DMA-API: device driver frees DMA sg list with different entry count [map count=4] [unmap count=1]

Quoted 6 lines

Quoted 2 lines
Interesting. Do you have any idea why this warning didn't show up earlier? Is there perhaps a Kconfig option I ought to be using?
My question here is why don't we map the complete sg list but only one entry. What happens to the remaining few sg list entries? Don't we sent too less data?

I don't know exactly what happened in this particular case -- probably there's no way to find out. However, all the data is supposed to be mapped, regardless of the number of entries.

Quoted 3 lines

Is there anything special you do to get into this kind of situation? Like 16GiB of memory on 32bit with highmem and a brutal disk/io test case so?

As I understand it, the typical case is that two SG entries in a row refer to adjacent pages of physical memory. The mapping routine then collapses them into a single entry referring to all the pages.

However, this is not the sort of thing you can deliberately cause, unless you set up your SG list by hand.

Alan Stern

Sebastian Andrzej Siewior

2011-12-15 16:23 UTC #6 lore raw

Quoted 10 lines · Alan Stern

Quoted 3 lines
Is there anything special you do to get into this kind of situation? Like 16GiB of memory on 32bit with highmem and a brutal disk/io test case so?
As I understand it, the typical case is that two SG entries in a row refer to adjacent pages of physical memory. The mapping routine then collapses them into a single entry referring to all the pages.
However, this is not the sort of thing you can deliberately cause, unless you set up your SG list by hand.

Thanks, both of you for the explanation.

Quoted 2 lines

Alan Stern

Sebastian