[PATCH] usb: fix number of mapped SG DMA entries

[PATCH] usb: fix number of mapped SG DMA entries

[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]
 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 <clemens@ladisch.de>
---
 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(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 13222d3..179e364 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -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(
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 4e4066c..fef1db3 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -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);
diff --git a/drivers/usb/host/uhci-q.c b/drivers/usb/host/uhci-q.c
index f6ca80e..d2c6f5a 100644
--- a/drivers/usb/host/uhci-q.c
+++ b/drivers/usb/host/uhci-q.c
@@ -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);
diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c
index d6e1754..a91712c 100644
--- a/drivers/usb/host/whci/qset.c
+++ b/drivers/usb/host/whci/qset.c
@@ -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;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9f1d4b1..d28c586 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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));

diff --git a/include/linux/usb.h b/include/linux/usb.h
index d3d0c13..7503352 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -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 */

Re: [PATCH] usb: fix number of mapped SG DMA entries

[Alan Stern]

On Sat, 3 Dec 2011, Clemens Ladisch wrote:

> 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

Re: [PATCH] usb: fix number of mapped SG DMA entries

[Clemens Ladisch]

Alan Stern wrote:
> On Sat, 3 Dec 2011, Clemens Ladisch wrote:
>> 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?

CONFIG_DMA_API_DEBUG; its help warns of performance degradation.


Regards,
Clemens

Re: [PATCH] usb: fix number of mapped SG DMA entries

[Sebastian Andrzej Siewior]

* Alan Stern | 2011-12-04 11:59:32 [-0500]:

>On Sat, 3 Dec 2011, Clemens Ladisch wrote:
>
>> 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?

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?

>Alan Stern

Sebastian

Re: [PATCH] usb: fix number of mapped SG DMA entries

[Clemens Ladisch]

Sebastian Andrzej Siewior wrote:
> * Alan Stern | 2011-12-04 11:59:32 [-0500]:
>> On Sat, 3 Dec 2011, Clemens Ladisch wrote:
>>> 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?
>
> 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).

> 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)

> 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

Re: [PATCH] usb: fix number of mapped SG DMA entries

[Alan Stern]

On Thu, 15 Dec 2011, Sebastian Andrzej Siewior wrote:

> * Alan Stern | 2011-12-04 11:59:32 [-0500]:
> 
> >On Sat, 3 Dec 2011, Clemens Ladisch wrote:
> >
> >> 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?
> 
> 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.

> 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

Re: [PATCH] usb: fix number of mapped SG DMA entries

[Sebastian Andrzej Siewior]

On 12/15/2011 05:01 PM, Alan Stern wrote:

>> 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.

>
> Alan Stern

Sebastian