On Wed, Nov 13, 2013 at 10:24:00AM -0000, David Laight wrote: > > Greg KH wrote: > > On Tue, Nov 12, 2013 at 01:58:05PM -0000, David Laight wrote: > > > > > > There needs to be a wmb() barrier between the write to the cycle bit > > > in the first TRB and the write to the doorbell register. > > > > > > Since it isn't needed in the other places the doobell is rung > > > (because the ring contents haven't been changed) add it to > > > giveback_first_trb() rather than somewhere later. > > > > > > Signed-off-by: David Laight <david.laight@xxxxxxxxxx> > > > --- > > > This patch will only apply cleanly if my earlier patch that affects > > > the previous line has also been applied. > > > > > > drivers/usb/host/xhci-ring.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > > > index 35dfed0..8bce4c3 100644 > > > --- a/drivers/usb/host/xhci-ring.c > > > +++ b/drivers/usb/host/xhci-ring.c > > > @@ -3136,6 +3136,7 @@ static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id, > > > */ > > > wmb(); > > > start_trb->field[3] ^= cpu_to_le32(TRB_CYCLE); > > > + wmb(); > > > > I don't understand, why is this needed? field[] isn't being used > > elsewhere at this point in time, is it? > > > > When adding barriers, you need to also add a big comment as to exactly > > why this is needed, in the code itself, which this patch does not do, so > > we can't take it (well, I will not take it, Sarah might, but then I'll > > complain to her...) > > The sequence is: > 1) Write all the ring entries, but leave the first with its old TRB_CYCLE value. > 2) The first wmb() in giveback_first_trb() ensures that all the ring > entries are written before we... > 3) Invert the TRB_CYCLE bit on the first TRB, this allows the hardware to > process that entry. > 4) The extra wmb() ensures that the ring entry write happens before... > 5) We write to the doorbell register telling the hardware it can read > the first ring entry. > > Without the extra wmb() the hardware could read the ring entry before > the cpu has inverted its TRB_CYCLE - so the TD wouldn't be processed > until the next URB setup is completed. All right, you've convinced me that the extra wmb() is necessary. Can you please add the above explanation as a function comment above giveback_first_trb() and re-submit this patch? > Given the size of the function that writes to the doorbell register > this is probably unlikely. > OTOH the doorbell register address and value to write could be > cached in the ep_ring making it a very short (and inlined) function. The reason we haven't run into this is probably because the xHCI driver has only been thoroughly tested on x86 platforms. PCI memory writes aren't reordered on that platform, so wmb is a no-op. Given that no one has complained about this bug that has been around since the xHCI driver was first submitted, I'm disinclined to queue this for stable. Sarah Sharp -- 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
