On Sat, 25 Mar 2017, pierre kuo wrote:
> hi Oliver:
>
> 2017-03-23 17:33 GMT+08:00 Oliver Neukum <[email protected]>:
> > 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 <[email protected]>
> >> ---
> >> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
