On Sat, 25 Mar 2017, pierre kuo wrote: > hi Oliver: > > 2017-03-23 17:33 GMT+08:00 Oliver Neukum <oneukum@xxxxxxxx>: > > Am Donnerstag, den 23.03.2017, 16:39 +0800 schrieb pierre Kuo: > > > > Hi, > > > >> for sitd_link and itd_link, put wmb after the last memory access to make > >> sure ehci->period[frame] is visible by host before scheduling the > >> descriptors > > > > Please explain what the problem you are trying to fix is. > I just trace this part of code and try to make sure host will see data > structure correctly before scheduling. > > > > >> Signed-off-by: pierre Kuo <vichy.kuo@xxxxxxxxx> > >> --- > >> drivers/usb/host/ehci-sched.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c > >> index 980a6b3..11b517c 100644 > >> --- a/drivers/usb/host/ehci-sched.c > >> +++ b/drivers/usb/host/ehci-sched.c > >> @@ -1754,8 +1754,9 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci, > >> itd->hw_next = *hw_p; > >> prev->itd = itd; > >> itd->frame = frame; > >> - wmb(); > > > > This looks very wrong. You need to make sure that a data structure > > is written to RAM before you announce it to the hardware. > > The code before the wmb() initializes a structure and the write > > after it announces it to the hardware. This wmb() is exactly where > > it needs to be. > > > >> *hw_p = cpu_to_hc32(ehci, itd->itd_dma | Q_TYPE_ITD); > >> + /* make sure host see periodic frame change before scheduling */ > >> + wmb(); > > > > If you need to flush something, wmb() is the wrong primitive. > > It merely ensures ordering. It is not guaranteed to flush writes > Not guaranteed to flush writes?that makes me curious. > As far as I know, wmb() make sure any write operation before the wmb() > is completed and are observed before prior to the execution of any > subsequent write. No, that's not correct. wmb() makes sure that any write operation before the wmb() is completed prior to the execution of any subsequent write. It does _not_ guarantee that the earlier write is _observed_ prior to the execution of subsequent writes. However, it _does_ guarantee that the earlier write is observed before any subsequent write is observed. In other words, if we have A, wmb(), B, then: A completes before B is executed, and A is observed before B is observed. But we do not have: A is observed before B is executed. > I can understand ur opinion that below (1) wmb is needed, since it > preserve the write order of (A) and (B). > But shouldn't we add wmb in (2) to make sure the ordering of (B) and > following writes? > sitd->hw_next = ehci->periodic[frame]; > ehci->pshadow[frame].sitd = sitd; > sitd->frame = frame; --------------------> (A) > - wmb(); ------------------------------------> (1) > ehci->periodic[frame] = cpu_to_hc32(ehci, sitd->sitd_dma | > Q_TYPE_SITD); -----> (B) > + /* make sure host see periodic frame change before scheduling */ > + wmb(); ------------------------------------> (2) No. (B) does not need to be ordered before following writes. Think about it -- what would happen if a subsequent write was observed before (B)? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
