EHCI: add to memory barrier to updating hw_next

RFC PATCH In review 김기오 3 participants 8 messages Jul 17, 2013 – Jul 19, 2013

Subsystems (2)

USB USB ehci driver

lore

Share & tools

lore

김기오

author

2013-07-17 05:03 UTC #0 lore raw

RFC PATCH 1/1 EHCI: add to memory barrier to updating hw_next

Hi,

I have a missing urb completion problem on ARMv7 based platform.

I thought the above problem was caused by coherent memory between the EHCI device and CPU so I tryied to allocates device type memory for EHCI via dma_declare_coherent_memory at machine initialization step so that EHCI always allocates from those device type memory. It seems to solve the issue because I didn't see any problem.

But I am not sure it is acceptable solution. So I applied the patch
https://lkml.org/lkml/2011/8/31/344.
But it could not solve the problem so that I added another wmb() as my patch, and now my platform works fine.

I am not sure what's the exact problem and what wmb I added could solve but I just think the problem is related to store buffer flush of hw_next. Anyway, important thing is that it fixed my problem so I expect you expert guys could find what I am missing and a right solution. IMHO, the patch might miss updating hw_next pointer.
Am I correct?

I understand the wmb() is just memory barrier, not write-buffer flush. But it is true that wmb() can flush write buffer in ARM. Anyhow I think that memory type, "normal memory, non-cacheable", may have a problem for some devices that needs device type memory.

I cannot get conclusion from the discussion at
https://lkml.org/lkml/2011/8/31/344.
Which can I do for my platform, wmb() or dma_coherent_write_sync()?

Signed-off-by Gioh Kim <[email protected]>

---
 drivers/usb/host/ehci-q.c |    1 +
 1 file changed, 1 insertion(+)

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

index d34b399..779d9e8 100644
@@ -501,6 +501,7 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
                        last = list_entry (qtd->qtd_list.prev,
                                        struct ehci_qtd, qtd_list);
                        last->hw_next = qtd->hw_next;
+                       wmb();
                }
 
                /* remove qtd; it's recycled after possible urb completion */

--
1.7.9.5

Ming Lei

2013-07-17 08:51 UTC #1 lore raw

Cc ARM list

Quoted 10 lines · ???

Hi,
I have a missing urb completion problem on ARMv7 based platform.
I thought the above problem was caused by coherent memory between the EHCI device and CPU so I tryied to allocates device type memory for EHCI via dma_declare_coherent_memory at machine initialization step so that EHCI always allocates from those device type memory. It seems to solve the issue because I didn't see any problem.

Quoted 4 lines

But I am not sure it is acceptable solution. So I applied the patch
https://lkml.org/lkml/2011/8/31/344.
But it could not solve the problem so that I added another wmb() as my patch, and now my platform works fine.

I remember the previous problem reported on Pandaboard firstly was fixed by Will's commit 11ed0ba1(ARM: 7161/1: errata: no automatic store buffer drain), so could you try to enable PL310_ERRATA_769419 and see if your problem can be fixed?

Quoted 3 lines

I am not sure what's the exact problem and what wmb I added could solve but I just think the problem is related to store buffer flush of hw_next.

Yes, per the above commit, but it might be one hardware problem.

Quoted 9 lines

Anyway, important thing is that it fixed my problem so I expect you expert guys could find what I am missing and a right solution. IMHO, the patch might miss updating hw_next pointer.
Am I correct?
I understand the wmb() is just memory barrier, not write-buffer flush. But it is true that wmb() can flush write buffer in ARM. Anyhow I think that memory type, "normal memory, non-cacheable", may have a problem for some devices that needs device type memory.

Currently on ARMv7 DMA coherent buffer has to be bufferable, in theory we might need one API to flush CPU write buffer, as described in Documentation/DMA-API-HOWTO.txt:

Also, on some platforms your driver may need to flush CPU write buffers in much the same way as it needs to flush write buffers found in PCI bridges (such as by reading a register's value after writing it).

But actually on hardware without the problem(such as A15), I don't see any effect without flushing store buffer explicitly.

Quoted 4 lines

I cannot get conclusion from the discussion at
https://lkml.org/lkml/2011/8/31/344.
Which can I do for my platform, wmb() or dma_coherent_write_sync()?

If your hardware is covered by PL310_ERRATA_769419, maybe it is better to enable the errata.

Quoted 21 lines

Signed-off-by Gioh Kim <[email protected]>

---
 drivers/usb/host/ehci-q.c |    1 +
 1 file changed, 1 insertion(+)

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

index d34b399..779d9e8 100644
@@ -501,6 +501,7 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
                        last = list_entry (qtd->qtd_list.prev,
                                        struct ehci_qtd, qtd_list);
                        last->hw_next = qtd->hw_next;
+                       wmb();
                }
 
                /* remove qtd; it's recycled after possible urb completion */
--

1.7.9.5

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks,

Signature · 1 line

Ming Lei

Gioh Kim

author

2013-07-18 01:30 UTC #2 lore raw

Thanks for your reply.

Quoted 27 lines

-----Original Message-----
From: Ming Lei [mailto:tom.leiming at gmail.com]
Sent: Wednesday, July 17, 2013 5:52 PM
To: ???
Cc: Alan Stern; linux-usb at vger.kernel.org; linux-kernel at vger.kernel.org; Mark Salter; namhyung.kim at lge.com; Minchan Kim; Chanho Min; Jong-Sung Kim; linux-arm-kernel
Subject: Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next
Cc ARM list
Quoted 10 lines · ???
Hi,
I have a missing urb completion problem on ARMv7 based platform.
I thought the above problem was caused by coherent memory between the EHCI device and CPU so I tryied to allocates device type memory for EHCI via dma_declare_coherent_memory at machine initialization step so that EHCI always allocates from those device type memory. It seems to solve the issue because I didn't see any problem.
Quoted 4 lines
But I am not sure it is acceptable solution. So I applied the patch
https://lkml.org/lkml/2011/8/31/344.
But it could not solve the problem so that I added another wmb() as my patch, and now my platform works fine.

Quoted 4 lines

I remember the previous problem reported on Pandaboard firstly was fixed by Will's commit 11ed0ba1(ARM: 7161/1: errata: no automatic store buffer drain), so could you try to enable PL310_ERRATA_769419 and see if your problem can be fixed?

I also know the errata but it didn't work for my platform.

Quoted 22 lines

Quoted 4 lines
I am not sure what's the exact problem and what wmb I added could solve but I just think the problem is related to store buffer flush of
hw_next.
Yes, per the above commit, but it might be one hardware problem.
Quoted 9 lines
Anyway, important thing is that it fixed my problem so I expect you expert guys could find what I am missing and a right solution. IMHO, the patch might miss updating hw_next pointer.
Am I correct?
I understand the wmb() is just memory barrier, not write-buffer flush. But it is true that wmb() can flush write buffer in ARM. Anyhow I think that memory type, "normal memory, non-cacheable", may have a problem for some devices that needs device type memory.
Currently on ARMv7 DMA coherent buffer has to be bufferable, in theory we might need one API to flush CPU write buffer, as described in Documentation/DMA-API-HOWTO.txt:

Quoted 7 lines

Also, on some platforms your driver may need to flush CPU write buffers in much the same way as it needs to flush write buffers found in PCI bridges (such as by reading a register's value after writing it).
But actually on hardware without the problem(such as A15), I don't see any effect without flushing store buffer explicitly.

The problem of my platform is occurring when it has very heavy traffic such as HD video streaming service or very many small images display.
I guess that HC could have a use-after-free problem like following situation.

1. A qtd which is not at the queue head should be removed in qh_completions().
2. The last->hw_next become be pointing at the next qtd but the hw_next value is delayed in write-buffer.
3. The qtd is removed in the list.
4. The qtd is freed into DMA pool and re-allocated for another urb.
5. HC try to process last->hw_next and it is pointing re-allocated qtd.

What do you think about it? Is it possible?

Quoted 41 lines

Quoted 5 lines
I cannot get conclusion from the discussion at
https://lkml.org/lkml/2011/8/31/344.
Which can I do for my platform, wmb() or dma_coherent_write_sync()?
If your hardware is covered by PL310_ERRATA_769419, maybe it is better to enable the errata.
drivers/usb/host/ehci-q.c qh_completions (struct ehci_hcd *ehci, struct
Quoted 11 lines
Signed-off-by Gioh Kim <[email protected]>
---
 drivers/usb/host/ehci-q.c |    1 +
 1 file changed, 1 insertion(+)
drivers/usb/host/ehci-q.c +0 -0
index d34b399..779d9e8 100644
@@ -501,6 +501,7 @@ qh_completions (struct ehci_hcd *ehci, struct
ehci_qh *qh)
Quoted 11 lines
drivers/usb/host/ehci-q.c +1 -0
                        last = list_entry (qtd->qtd_list.prev,
                                        struct ehci_qtd, qtd_list);
                        last->hw_next = qtd->hw_next;
+                       wmb();
                }
 
                /* remove qtd; it's recycled after possible urb
completion */
--
1.7.9.5
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
--
Ming Lei

Ming Lei

2013-07-18 10:07 UTC #3 lore raw

Quoted 65 lines · Gioh Kim

Thanks for your reply.
Quoted 27 lines
-----Original Message-----
From: Ming Lei [mailto:tom.leiming at gmail.com]
Sent: Wednesday, July 17, 2013 5:52 PM
To: ???
Cc: Alan Stern; linux-usb at vger.kernel.org; linux-kernel at vger.kernel.org; Mark Salter; namhyung.kim at lge.com; Minchan Kim; Chanho Min; Jong-Sung Kim; linux-arm-kernel
Subject: Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next
Cc ARM list
Quoted 10 lines · ???
Hi,
I have a missing urb completion problem on ARMv7 based platform.
I thought the above problem was caused by coherent memory between the EHCI device and CPU so I tryied to allocates device type memory for EHCI via dma_declare_coherent_memory at machine initialization step so that EHCI always allocates from those device type memory. It seems to solve the issue because I didn't see any problem.
Quoted 4 lines
But I am not sure it is acceptable solution. So I applied the patch
https://lkml.org/lkml/2011/8/31/344.
But it could not solve the problem so that I added another wmb() as my patch, and now my platform works fine.
Quoted 4 lines
I remember the previous problem reported on Pandaboard firstly was fixed by Will's commit 11ed0ba1(ARM: 7161/1: errata: no automatic store buffer drain), so could you try to enable PL310_ERRATA_769419 and see if your problem can be fixed?
I also know the errata but it didn't work for my platform.
Quoted 27 lines
Quoted 4 lines
I am not sure what's the exact problem and what wmb I added could solve but I just think the problem is related to store buffer flush of
hw_next.
Yes, per the above commit, but it might be one hardware problem.
Quoted 9 lines
Anyway, important thing is that it fixed my problem so I expect you expert guys could find what I am missing and a right solution. IMHO, the patch might miss updating hw_next pointer.
Am I correct?
I understand the wmb() is just memory barrier, not write-buffer flush. But it is true that wmb() can flush write buffer in ARM. Anyhow I think that memory type, "normal memory, non-cacheable", may have a problem for some devices that needs device type memory.
Currently on ARMv7 DMA coherent buffer has to be bufferable, in theory we might need one API to flush CPU write buffer, as described in Documentation/DMA-API-HOWTO.txt:
Also, on some platforms your driver may need to flush CPU write buffers in much the same way as it needs to flush write buffers found in PCI bridges (such as by reading a register's value after writing it).

Quoted 6 lines

Quoted 2 lines
But actually on hardware without the problem(such as A15), I don't see any effect without flushing store buffer explicitly.
The problem of my platform is occurring when it has very heavy traffic such as HD video streaming service or very many small images display.

Could you describe your test case in a bit detail? Which USB applications/drivers
may trigger your URB completion miss problem? Under what kind of video streaming service(USB video or only kind of heavy load)?

Quoted 9 lines

I guess that HC could have a use-after-free problem like following situation.
1. A qtd which is not at the queue head should be removed in qh_completions().
2. The last->hw_next become be pointing at the next qtd but the hw_next value is delayed in write-buffer.
3. The qtd is removed in the list.
4. The qtd is freed into DMA pool and re-allocated for another urb.
5. HC try to process last->hw_next and it is pointing re-allocated qtd.
What do you think about it? Is it possible?

I understand it might not be possible because: when 'stopped' is set, that said the HC might not advance the queue. But I don't understand why 'last->hw_next' is patched here under 'stopped' situation.

Even the 'stopped' case may be seldom triggered, do you know under which condition the stopped is triggered in your problem?(stall, short read or others)

Thanks,

Signature · 1 line

Ming Lei

Alan Stern

maintainer

2013-07-18 14:08 UTC #4 lore raw

Quoted 8 lines · Ming Lei

Quoted 8 lines
I guess that HC could have a use-after-free problem like following situation.
1. A qtd which is not at the queue head should be removed in qh_completions().
2. The last->hw_next become be pointing at the next qtd but the hw_next value is delayed in write-buffer.
3. The qtd is removed in the list.
4. The qtd is freed into DMA pool and re-allocated for another urb.
5. HC try to process last->hw_next and it is pointing re-allocated qtd.

Quoted 5 lines

Quoted 1 line
What do you think about it? Is it possible?
I understand it might not be possible because: when 'stopped' is set, that said the HC might not advance the queue. But I don't understand why 'last->hw_next' is patched here under 'stopped' situation.

It should not be possible. When "stopped" is set, the QH gets unlinked and relinked before it can start up again. Relinking involves some memory barriers, so the qTD will not be accessed again by the HC.

last->hw_next gets patched because the qTD might belong to some URB in the middle of the queue that is being unlinked. The URBs before it and after it will still be active, so the queue link has to be updated.

Quoted 3 lines

Even the 'stopped' case may be seldom triggered, do you know under which condition the stopped is triggered in your problem?(stall, short read or others)

I was going to ask the same question. This particular piece of code gets executed _only_ when an URB is unlinked. Not during any other kind of error.

Alan Stern

Ming Lei

2013-07-19 03:50 UTC #5 lore raw

Quoted 16 lines · Alan Stern

Quoted 8 lines · Ming Lei
Quoted 8 lines
I guess that HC could have a use-after-free problem like following situation.
1. A qtd which is not at the queue head should be removed in qh_completions().
2. The last->hw_next become be pointing at the next qtd but the hw_next value is delayed in write-buffer.
3. The qtd is removed in the list.
4. The qtd is freed into DMA pool and re-allocated for another urb.
5. HC try to process last->hw_next and it is pointing re-allocated qtd.
Quoted 5 lines
Quoted 1 line
What do you think about it? Is it possible?
I understand it might not be possible because: when 'stopped' is set, that said the HC might not advance the queue. But I don't understand why 'last->hw_next' is patched here under 'stopped' situation.

Quoted 7 lines

It should not be possible. When "stopped" is set, the QH gets unlinked and relinked before it can start up again. Relinking involves some memory barriers, so the qTD will not be accessed again by the HC.
last->hw_next gets patched because the qTD might belong to some URB in the middle of the queue that is being unlinked. The URBs before it and after it will still be active, so the queue link has to be updated.

'stopped' is set under below situations:

- unlink over or shutdown
- halt
- short packet(URB_SHORT_NOT_OK)

Looks EHCI won't advance the queue(qh) any more on above situations, so I think last->hw_next might not need patching.

Quoted 8 lines

Quoted 4 lines
Even the 'stopped' case may be seldom triggered, do you know under which condition the stopped is triggered in your problem?(stall, short read or others)
I was going to ask the same question. This particular piece of code gets executed _only_ when an URB is unlinked. Not during any other kind of error.

The code may run under 'halt' or short packet(URB_SHORT_NOT_OK) too. If Gioh's problem falls to these two situations, below patch might be helpful.

Because the qh will be unlinked when there is fault in the endpoint(halt, short packet), maybe it is safer to complete these URBs after the qh becomes unlinked, like what the blew patch does:

drivers/usb/host/ehci-q.c +5 ~~-10~~

index b637a65..6a65e0a 100644
@@ -454,6 +454,10 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 			}
 		}
 
+		/* complete URBs after unlinking */
+		if (stopped && state != QH_STATE_IDLE)
+			goto exit;
+
 		/* unless we already know the urb's status, collect qtd status
 		 * and update count of bytes transferred.  in common short read
 		 * cases with only one data qtd (including control transfers),

@@ -489,15 +493,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 			}
 		}
 
-		/* if we're removing something not at the queue head,
-		 * patch the hardware queue pointer.
-		 */
-		if (stopped && qtd->qtd_list.prev != &qh->qtd_list) {
-			last = list_entry (qtd->qtd_list.prev,
-					struct ehci_qtd, qtd_list);
-			last->hw_next = qtd->hw_next;
-		}
-
 		/* remove qtd; it's recycled after possible urb completion */
 		list_del (&qtd->qtd_list);
 		last = qtd;

@@ -520,7 +515,7 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 
 		/* Otherwise the caller must unlink the QH. */
 	}
-
+ exit:
 	/* restore original state; caller must unlink or relink */
 	qh->qh_state = state;

Thanks,

Signature · 1 line

Ming Lei

Gioh Kim

author

2013-07-19 10:45 UTC #6 lore raw

In reply to Alan Stern

Thanks a lot for your replay.

Quoted 11 lines

-----Original Message-----
From: Alan Stern [mailto:stern at rowland.harvard.edu]
Sent: Thursday, July 18, 2013 11:09 PM
To: Ming Lei
Cc: Gioh Kim; linux-usb at vger.kernel.org; linux-kernel at vger.kernel.org; Mark Salter; namhyung.kim at lge.com; Minchan Kim; Chanho Min; Jong-Sung Kim; linux-arm-kernel
Subject: Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next
On Thu, 18 Jul 2013, Ming Lei wrote:

Quoted 10 lines

Quoted 1 line
Quoted 1 line
I guess that HC could have a use-after-free problem like following
situation.
Quoted 2 lines
Quoted 2 lines
1. A qtd which is not at the queue head should be removed in
qh_completions().
Quoted 1 line
Quoted 1 line
2. The last->hw_next become be pointing at the next qtd but the
hw_next value is delayed in write-buffer.
Quoted 3 lines
Quoted 3 lines
3. The qtd is removed in the list.
4. The qtd is freed into DMA pool and re-allocated for another urb.
5. HC try to process last->hw_next and it is pointing re-allocated

qtd.

Quoted 7 lines

Quoted 6 lines
Quoted 2 lines
What do you think about it? Is it possible?
I understand it might not be possible because: when 'stopped' is set, that said the HC might not advance the queue. But I don't understand why 'last->hw_next' is patched here under 'stopped' situation.

Quoted 8 lines

It should not be possible. When "stopped" is set, the QH gets unlinked and relinked before it can start up again. Relinking involves some memory barriers, so the qTD will not be accessed again by the HC.
last->hw_next gets patched because the qTD might belong to some URB in the middle of the queue that is being unlinked. The URBs before it and after it will still be active, so the queue link has to be updated.

You're right. I misunderstand those codes. Please forget about it.

Quoted 7 lines

Quoted 3 lines
Even the 'stopped' case may be seldom triggered, do you know under which condition the stopped is triggered in your problem?(stall, short read or others)
I was going to ask the same question. This particular piece of code gets executed _only_ when an URB is unlinked. Not during any other kind of error.

I've got the problem when I listened to the mp3 file of USB HDD. I checked the urb data when the problem occurred, the last-status value of urb was EINPROGRESS and
urb->unlinked was ECONNRESET.
I think the 'stopped' case was occurred by the reset of USB port. The block device driver did reset USB port because there is no return from USB device.
If I made block device driver could not reset USB port, the EHCI driver codes were not executed.
Finally the halt of HC makes 'stopped' case.

I think halt of the HC might be caused that store-buffer delays command for HC.
When I applied the patch from https://lkml.org/lkml/2011/8/31/344 and added a mb() into hw_next updating
to remove delay of store-buffer, My platform works well.

Can the store-buffer delay halt HC? Is it possible?

IMHO, if the qTD list is broken the HC think there is no qTD to send. So I added mb() at hw_next update code.

Quoted 2 lines

Alan Stern

Alan Stern

maintainer

2013-07-19 15:26 UTC #7 lore raw

Quoted 9 lines · Gioh Kim

Quoted 3 lines
I was going to ask the same question. This particular piece of code gets executed _only_ when an URB is unlinked. Not during any other kind of error.
I've got the problem when I listened to the mp3 file of USB HDD. I checked the urb data when the problem occurred, the last-status value of urb was EINPROGRESS and
urb->unlinked was ECONNRESET.

Ah, so the URB _was_ unlinked.

Quoted 3 lines

I think the 'stopped' case was occurred by the reset of USB port. The block device driver did reset USB port because there is no return from USB device.

Okay.

Quoted 3 lines

If I made block device driver could not reset USB port, the EHCI driver codes were not executed.
Finally the halt of HC makes 'stopped' case.

Why was the HC halted? That should happen only when there is an extremely severe error.

Quoted 7 lines

I think halt of the HC might be caused that store-buffer delays command for HC.
When I applied the patch from https://lkml.org/lkml/2011/8/31/344 and added a mb() into hw_next updating
to remove delay of store-buffer, My platform works well.
Can the store-buffer delay halt HC? Is it possible?

I don't see how. It could slow things down but it should not cause any errors.

Quoted 2 lines

IMHO, if the qTD list is broken the HC think there is no qTD to send. So I added mb() at hw_next update code.

At the time when the hw_next update gets executed, what is the value of "state"? It should be QH_STATE_IDLE.

Alan Stern