[PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[PATCH] [RFC] 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 <gioh.kim@lge.com>
---
 drivers/usb/host/ehci-q.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index d34b399..779d9e8 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -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

Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[Ming Lei]

Cc: ARM list

On Wed, Jul 17, 2013 at 1:03 PM, 김기오 <gioh.kim@lge.com> wrote:
> 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 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 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.

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

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

>
> Signed-off-by: Gioh Kim <gioh.kim@lge.com>
> ---
>  drivers/usb/host/ehci-q.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index d34b399..779d9e8 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Thanks,
-- 
Ming Lei

RE: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[Gioh Kim]

Thanks for your reply.

> -----Original Message-----
> From: Ming Lei [mailto:tom.leiming@gmail.com]
> Sent: Wednesday, July 17, 2013 5:52 PM
> To: 김기오
> Cc: Alan Stern; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> Mark Salter; namhyung.kim@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
> 
> On Wed, Jul 17, 2013 at 1:03 PM, 김기오 <gioh.kim@lge.com> wrote:
> > 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 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.


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


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?


> 
> >
> > 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.
> 
> >
> > Signed-off-by: Gioh Kim <gioh.kim@lge.com>
> > ---
> >  drivers/usb/host/ehci-q.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> > index d34b399..779d9e8 100644
> > --- a/drivers/usb/host/ehci-q.c
> > +++ b/drivers/usb/host/ehci-q.c
> > @@ -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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Thanks,
> --
> Ming Lei

Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[Ming Lei]

On Thu, Jul 18, 2013 at 9:30 AM, Gioh Kim <gioh.kim@lge.com> wrote:
> Thanks for your reply.
>
>> -----Original Message-----
>> From: Ming Lei [mailto:tom.leiming@gmail.com]
>> Sent: Wednesday, July 17, 2013 5:52 PM
>> To: 김기오
>> Cc: Alan Stern; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
>> Mark Salter; namhyung.kim@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
>>
>> On Wed, Jul 17, 2013 at 1:03 PM, 김기오 <gioh.kim@lge.com> wrote:
>> > 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 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.
>
>
>>
>> >
>> > 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.
>>
>> > 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.
>
>
> 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)?

> 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,
-- 
Ming Lei

Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[Alan Stern]

On Thu, 18 Jul 2013, Ming Lei wrote:

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

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.

> 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

Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[Ming Lei]

On Thu, Jul 18, 2013 at 10:08 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 18 Jul 2013, Ming Lei wrote:
>
>> > 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.
>
> 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.

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

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index b637a65..6a65e0a 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -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,
-- 
Ming Lei

RE: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[Gioh Kim]

Thanks a lot for your replay.

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Thursday, July 18, 2013 11:09 PM
> To: Ming Lei
> Cc: Gioh Kim; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> Mark Salter; namhyung.kim@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:
> 
> > > 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.
> 
> 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.


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





> 
> Alan Stern

RE: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next

[Alan Stern]

On Fri, 19 Jul 2013, Gioh Kim wrote:

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

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

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

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

> 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