xhci: Add rmb() between reading event validity & event data access.
Subsystems (2)
PATCH 3/5 xhci: Add rmb() between reading event validity & event data access.
On weakly-ordered systems, the reading of an event's content must occur after reading the event's validity.
--- drivers/usb/host/xhci-ring.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/usb/host/xhci-ring.c +1 -1
@@ -2151,7 +2151,7 @@ static void xhci_handle_event(struct xhci_hcd *xhci)return ; }xhci_dbg (xhci,"%s - OS owns TRB\n" , __func__); - +rmb ();/* FIXME: Handle more event types. */ switch ((le32_to_cpu (event->event_cmd.flags) &TRB_TYPE_BITMASK )) {case TRB_TYPE (TRB_COMPLETION ):
Signature · 1 line
1.7.0.4
Quoted 2 lines
On weakly-ordered systems, the reading of an event's content must occur after reading the event's validity.
Put a comment on the rmb() saying what it orders?
Segher
PATCH v2 3/5 xhci: Add rmb() between reading event validity & event data access.
On weakly-ordered systems, the reading of an event's content must occur after reading the event's validity.
---
Segher, thanks for the comment; explanation added.
drivers/usb/host/xhci-ring.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/usb/host/xhci-ring.c +5 -0
@@ -2152,6 +2152,11 @@ static void xhci_handle_event(struct xhci_hcd *xhci) }xhci_dbg (xhci,"%s - OS owns TRB\n" , __func__); +/* +* Barrier between reading the TRB_CYCLE (valid) flag above and any +* speculative reads of the event's flags/data below. +*/ +rmb ();/* FIXME: Handle more event types. */ switch ((le32_to_cpu (event->event_cmd.flags) &TRB_TYPE_BITMASK )) {case TRB_TYPE (TRB_COMPLETION ):
Signature · 1 line
1.7.0.4
This patch looks fine, thanks!
Sarah Sharp
Quoted 29 lines · Matt Evans
On weakly-ordered systems, the reading of an event's content must occur after reading the event's validity.
Signed-off-by Matt Evans <[email protected]>---
Segher, thanks for the comment; explanation added.drivers/usb/host/xhci-ring.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)drivers/usb/host/xhci-ring.c +5
-0@@ -2152,6 +2152,11 @@ static void xhci_handle_event(struct xhci_hcd *xhci) }xhci_dbg (xhci,"%s - OS owns TRB\n" , __func__); +/* +* Barrier between reading the TRB_CYCLE (valid) flag above and any +* speculative reads of the event's flags/data below. +*/ +rmb ();/* FIXME: Handle more event types. */ switch ((le32_to_cpu (event->event_cmd.flags) &TRB_TYPE_BITMASK )) {case TRB_TYPE (TRB_COMPLETION ):Signature · 1 line
1.7.0.4
Quoted 15 lines · Matt Evans
On weakly-ordered systems, the reading of an event's content must occur after reading the event's validity.
Signed-off-by Matt Evans <[email protected]>---
Segher, thanks for the comment; explanation added.drivers/usb/host/xhci-ring.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)drivers/usb/host/xhci-ring.c +0
-0
drivers/usb/host/xhci-ring.c static void xhci_handle_event(struct xhci_hcd · line 2161
Quoted 10 lines
drivers/usb/host/xhci-ring.c +5
-0@@ -2152,6 +2152,11 @@ static void xhci_handle_event(struct xhci_hcd *xhci) }xhci_dbg (xhci,"%s - OS owns TRB\n" , __func__); +/* +* Barrier between reading the TRB_CYCLE (valid) flag above and any +* speculative reads of the event's flags/data below. +*/ +rmb ();/* FIXME: Handle more event types. */ switch ((le32_to_cpu (event->event_cmd.flags) &TRB_TYPE_BITMASK )) {
Isn't it the same memory that is being read the first time around? How reordering could happen here?
Thanks.
Signature · 1 line
Dmitry
Quoted 8 lines · Dmitry Torokhov
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* + * Barrier between reading the TRB_CYCLE (valid) flag aboveand any
drivers/usb/host/xhci-ring.c static void xhci_handle_event(struct xhci_hcd *xhci)
Quoted 5 lines
drivers/usb/host/xhci-ring.c +3
-0+* speculative reads of the event's flags/data below. +*/ +rmb ();/* FIXME: Handle more event types. */ switch ((le32_to_cpu(event->event_cmd.flags) &
Quoted 4 lines
TRB_TYPE_BITMASK)) {Isn't it the same memory that is being read the first time around? How reordering could happen here?
We don't try to enforce ordering specifically with the subsequent read of the flags that is visible in the snipped above (indeed that's the same address and so hopefully should be in program order), but all subsequent reads which could have been performed speculatively such as the rest of the event and including whatever other in memory information the chip might have updated prior to sending the event.
Cheers,
Ben.