On Mon, 10 Apr 2017, pierre kuo wrote: > hi Alan Hello. > >> > 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. > Got it and thanks for your kind explanation. ^^ > > > > >> 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)? > Per my understanding, > 1. this part of code is protected by ehci->lock, so there will be only > one processor update the data. > 2. Even subsequent write was observed before above (B), but all > necessary work is done on __this__ frame, and following write after > (1) will only affect next frame's data. > if I am wrong, please let me know. You are correct. But there's more to it. Even if a subsequent write was observed before B, it wouldn't matter. If the EHCI hardware sees the old value of ehci->periodic[frame] -- the value before B updated it -- then it will start reading the frame's data from the old starting point, the position after sitd. That's okay, skipping over the sitd that was just added won't hurt anything. But suppose you remove the wmb at (1). Then the EHCI hardware might see the new value of ehci->periodic[frame] and start reading the data from the new sitd, before it sees the value stored in sitd->hw_next. In other words, the hardware would read garbage from sitd->hw_next rather than the value stored two lines above (A), and the hardware would crash. That's why the wmb at (1) is necessary. 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
