Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-30 Thread Ming Lei
Hi Alan, Thanks for your comment. On Tue, Aug 30, 2011 at 12:33 AM, Alan Stern st...@rowland.harvard.edu wrote: Nevertheless, it remains true that you want to add a memory barrier instruction simply in order to speed up a cache writeback, not to force any sort of ordering.  This needs to be

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Russell King - ARM Linux
On Sun, Aug 28, 2011 at 09:51:10PM -0400, Alan Stern wrote: Hmmm. Although the semantics of the various mb() macros were originally defined only for inter-CPU synchronization, I believe they are also supposed to work for guaranteeing the order of accesses to DMA-coherent memory. If that's

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Alan Stern
On Mon, 29 Aug 2011, Russell King - ARM Linux wrote: You know better than I do what is needed to resolve the ordering issue. However, contrary to what the original patch description said, this isn't entirely a matter of making the write visible to the host controller: No doubt in time

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Ming Lei
Hi, On Mon, Aug 29, 2011 at 1:00 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sun, 28 Aug 2011, Ming Lei wrote: I've been following this whole discussion.  I didn't understand the idea behind the original patch, and I don't understand this.  What reason is there for adding a memory

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Alan Stern
On Mon, 29 Aug 2011, Ming Lei wrote: IMO, the dummy has been linked into queue pointed by qh-hw-hw_qtd_next, so EHCI will fetch dummy qtd and execute the transaction and will not have any delay on the transaction. Let me explain the problem again: On ARM, the wmb() before 'dummy-hw_token =

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Ming Lei
Hi, On Mon, Aug 29, 2011 at 11:03 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 29 Aug 2011, Ming Lei wrote: IMO, the dummy has been linked into queue pointed by qh-hw-hw_qtd_next, so EHCI will fetch dummy qtd and execute the transaction and will not have any delay on the

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Ming Lei
Hi, On Mon, Aug 29, 2011 at 9:57 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 29 Aug 2011, Russell King - ARM Linux wrote: You know better than I do what is needed to resolve the ordering issue. However, contrary to what the original patch description said, this isn't entirely

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Mark Salter
On Mon, 2011-08-29 at 23:55 +0800, Ming Lei wrote: If writing to coherent memory can't reach physical memory immediately on other ARCHs, the problem can still happen on these ARCHs. But I am not sure if there are these kind of ARCHs except for ARM. If there is write buffering which prevents

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-29 Thread Alan Stern
On Mon, 29 Aug 2011, Ming Lei wrote: Suppose HC can see the old value in hw_token, which has the ACTIVE bit clear. The qtd transaction will not be executed until the new token is flushed into memory. From view of CPU, the irq is still be delayed because ioc irq is generated after the qtd

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-28 Thread Alan Stern
On Sun, 28 Aug 2011, Ming Lei wrote: I've been following this whole discussion. �I didn't understand the idea behind the original patch, and I don't understand this. �What reason is there for adding a memory barrier after the dummy-hw_token = token line? �Such a barrier would not

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-28 Thread Russell King - ARM Linux
On Sun, Aug 28, 2011 at 01:00:07PM -0400, Alan Stern wrote: It won't do that. All it will do is guarantee that the CPU writes out dumy-hw_token before it writes out or reads in any values executed after the mb. You're right from the perspective of how things are defined today. However,

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-28 Thread Alan Stern
On Mon, 29 Aug 2011, Russell King - ARM Linux wrote: On Sun, Aug 28, 2011 at 01:00:07PM -0400, Alan Stern wrote: It won't do that. All it will do is guarantee that the CPU writes out dumy-hw_token before it writes out or reads in any values executed after the mb. You're right from

[PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread ming . lei
From: Ming Lei ming@canonical.com This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of https://bugs.launchpad.net/bugs/709245. In fact, one mb() on ARM is enough to

[PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread ming . lei
From: Ming Lei ming@canonical.com This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of https://bugs.launchpad.net/bugs/709245. In fact, one mb() on ARM is enough to

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Santosh
Hi, On Saturday 27 August 2011 08:18 PM, ming@canonical.com wrote: From: Ming Leiming@canonical.com This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Greg KH
On Sat, Aug 27, 2011 at 10:48:35PM +0800, ming@canonical.com wrote: From: Ming Lei ming@canonical.com This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Ming Lei
On Sat, Aug 27, 2011 at 11:03 PM, Santosh santosh.shilim...@ti.com wrote: Hi, On Saturday 27 August 2011 08:18 PM, ming@canonical.com wrote: From: Ming Leiming@canonical.com This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Ming Lei
Hi, On Sat, Aug 27, 2011 at 11:13 PM, Greg KH g...@kroah.com wrote: On Sat, Aug 27, 2011 at 10:48:35PM +0800, ming@canonical.com wrote: From: Ming Lei ming@canonical.com This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Santosh
On Saturday 27 August 2011 08:48 PM, Ming Lei wrote: On Sat, Aug 27, 2011 at 11:03 PM, Santoshsantosh.shilim...@ti.com wrote: Hi, On Saturday 27 August 2011 08:18 PM, ming@canonical.com wrote: From: Ming Leiming@canonical.com This patch fixs one performance bug on ARM Cortex A9

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Greg KH
On Sat, Aug 27, 2011 at 11:33:26PM +0800, Ming Lei wrote: Hi, On Sat, Aug 27, 2011 at 11:13 PM, Greg KH g...@kroah.com wrote: On Sat, Aug 27, 2011 at 10:48:35PM +0800, ming@canonical.com wrote: From: Ming Lei ming@canonical.com This patch fixs one performance bug on ARM Cortex

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Sergei Shtylyov
Hello. On 27-08-2011 18:48, ming@canonical.com wrote: From: Ming Leiming@canonical.com This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Ming Lei
Hi, Thanks for your comment. On Sun, Aug 28, 2011 at 12:07 AM, Greg KH g...@kroah.com wrote: As Santosh pointed out, mb on ARM will flush L2 write buffer. The description here is wrong. Then this can't be accepted as-is :) Yes, I will update it in v1, :-) I think the below should make

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Ming Lei
Hi, On Sun, Aug 28, 2011 at 12:57 AM, Ming Lei ming@canonical.com wrote: Are you sure?  Have you read the documentation about memory barriers to confirm this? I read the doc again, :-), and it mentions few about mb/wmb/rmb, I think my above description is still not correct. Generally

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Alan Stern
On Sun, 28 Aug 2011, Ming Lei wrote: I read the doc again, :-), and it mentions few about mb/wmb/rmb, I think my above description is still not correct. Generally speaking, mb only means there is a order between two accesses. Now I think only one mb() after 'dummy-hw_token = token;' is

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Alan Stern
On Sun, 28 Aug 2011, Ming Lei wrote: Looks like there is still another similar problem in qh_link_async(): the last wmb should be changed into mb, because HC will read 'head-hw-hw_next' from qh descriptor and this pointer in qh is read only for HC. But this problem can't be observed on

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Ming Lei
Hi, Thanks for your comment. On Sun, Aug 28, 2011 at 4:06 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sun, 28 Aug 2011, Ming Lei wrote: I read the doc again, :-), and it mentions few about mb/wmb/rmb, I think my above description is still not correct. Generally speaking, mb only

Re: [PATCH] usb: ehci: fix update qtd-token in qh_append_tds

2011-08-27 Thread Ming Lei
Hi, On Sun, Aug 28, 2011 at 4:11 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sun, 28 Aug 2011, Ming Lei wrote: Looks like there is still another similar problem in qh_link_async(): the last wmb should be changed into mb, because HC will read 'head-hw-hw_next' from qh descriptor and