[PATCH] USB UHCI: protect DMA-able fields with barriers

This is a revised patch to fix a problem in the UHCI driver, in which the
compiler incorrectly optimizes certain accesses to DMA-able memory
addresses.  The patch reorganizes the code to use special accessor
routines including a compiler optimization barrier, and stores the results
in local variables to help prevent repeated accesses.  No use is made of
the "volatile" keyword.  :-)
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

drivers/usb/host/uhci-debug.c

@@ -95,24 +95,25 @@ static int uhci_show_qh(struct uhci_qh *qh, char *buf, int len, int space)
 	struct list_head *head, *tmp;
 	struct uhci_td *td;
 	int i = 0, checked = 0, prevactive = 0;
+	__le32 element = qh_element(qh);
 
 	/* Try to make sure there's enough memory */
 	if (len < 80 * 6)
 		return 0;
 
 	out += sprintf(out, "%*s[%p] link (%08x) element (%08x)\n", space, "",
-			qh, le32_to_cpu(qh->link), le32_to_cpu(qh->element));
+			qh, le32_to_cpu(qh->link), le32_to_cpu(element));
 
-	if (qh->element & UHCI_PTR_QH)
+	if (element & UHCI_PTR_QH)
 		out += sprintf(out, "%*s  Element points to QH (bug?)\n", space, "");
 
-	if (qh->element & UHCI_PTR_DEPTH)
+	if (element & UHCI_PTR_DEPTH)
 		out += sprintf(out, "%*s  Depth traverse\n", space, "");
 
-	if (qh->element & cpu_to_le32(8))
+	if (element & cpu_to_le32(8))
 		out += sprintf(out, "%*s  Bit 3 set (bug?)\n", space, "");
 
-	if (!(qh->element & ~(UHCI_PTR_QH | UHCI_PTR_DEPTH)))
+	if (!(element & ~(UHCI_PTR_QH | UHCI_PTR_DEPTH)))
 		out += sprintf(out, "%*s  Element is NULL (bug?)\n", space, "");
 
 	if (!qh->urbp) {
@@ -127,7 +128,7 @@ static int uhci_show_qh(struct uhci_qh *qh, char *buf, int len, int space)
 
 	td = list_entry(tmp, struct uhci_td, list);
 
-	if (cpu_to_le32(td->dma_handle) != (qh->element & ~UHCI_PTR_BITS))
+	if (cpu_to_le32(td->dma_handle) != (element & ~UHCI_PTR_BITS))
 		out += sprintf(out, "%*s Element != First TD\n", space, "");
 
 	while (tmp != head) {
@@ -447,7 +448,7 @@ static int uhci_sprint_schedule(struct uhci_hcd *uhci, char *buf, int len)
 			if (qh->link != UHCI_PTR_TERM)
 				out += sprintf(out, "    bandwidth reclamation on!\n");
 
-			if (qh->element != cpu_to_le32(uhci->term_td->dma_handle))
+			if (qh_element(qh) != cpu_to_le32(uhci->term_td->dma_handle))
 				out += sprintf(out, "    skel_term_qh element is not set to term_td!\n");
 
 			continue;

drivers/usb/host/uhci-hcd.c

@@ -236,7 +236,7 @@ static void uhci_insert_tds_in_qh(struct uhci_qh *qh, struct urb *urb, __le32 br
 {
 	struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
 	struct uhci_td *td;
-	u32 *plink;
+	__le32 *plink;
 
 	/* Ordering isn't important here yet since the QH hasn't been */
 	/* inserted into the schedule yet */
@@ -637,8 +637,9 @@ static void uhci_dec_fsbr(struct uhci_hcd *uhci, struct urb *urb)
 /*
  * Map status to standard result codes
  *
- * <status> is (td->status & 0xF60000) [a.k.a. uhci_status_bits(td->status)]
- * Note: status does not include the TD_CTRL_NAK bit.
+ * <status> is (td_status(td) & 0xF60000), a.k.a.
+ * uhci_status_bits(td_status(td)).
+ * Note: <status> does not include the TD_CTRL_NAK bit.
  * <dir_out> is True for output TDs and False for input TDs.
  */
 static int uhci_map_status(int status, int dir_out)
@@ -843,21 +844,24 @@ static int uhci_result_control(struct uhci_hcd *uhci, struct urb *urb)
 	/* The rest of the TD's (but the last) are data */
 	tmp = tmp->next;
 	while (tmp != head && tmp->next != head) {
-		td = list_entry(tmp, struct uhci_td, list);
+		unsigned int ctrlstat;
 
+		td = list_entry(tmp, struct uhci_td, list);
 		tmp = tmp->next;
 
-		status = uhci_status_bits(td_status(td));
+		ctrlstat = td_status(td);
+		status = uhci_status_bits(ctrlstat);
 		if (status & TD_CTRL_ACTIVE)
 			return -EINPROGRESS;
 
-		urb->actual_length += uhci_actual_length(td_status(td));
+		urb->actual_length += uhci_actual_length(ctrlstat);
 
 		if (status)
 			goto td_error;
 
 		/* Check to see if we received a short packet */
-		if (uhci_actual_length(td_status(td)) < uhci_expected_length(td_token(td))) {
+		if (uhci_actual_length(ctrlstat) <
+				uhci_expected_length(td_token(td))) {
 			if (urb->transfer_flags & URB_SHORT_NOT_OK) {
 				ret = -EREMOTEIO;
 				goto err;
@@ -1031,16 +1035,19 @@ static int uhci_result_common(struct uhci_hcd *uhci, struct urb *urb)
 	urb->actual_length = 0;
 
 	list_for_each_entry(td, &urbp->td_list, list) {
-		status = uhci_status_bits(td_status(td));
+		unsigned int ctrlstat = td_status(td);
+
+		status = uhci_status_bits(ctrlstat);
 		if (status & TD_CTRL_ACTIVE)
 			return -EINPROGRESS;
 
-		urb->actual_length += uhci_actual_length(td_status(td));
+		urb->actual_length += uhci_actual_length(ctrlstat);
 
 		if (status)
 			goto td_error;
 
-		if (uhci_actual_length(td_status(td)) < uhci_expected_length(td_token(td))) {
+		if (uhci_actual_length(ctrlstat) <
+				uhci_expected_length(td_token(td))) {
 			if (urb->transfer_flags & URB_SHORT_NOT_OK) {
 				ret = -EREMOTEIO;
 				goto err;
@@ -1209,15 +1216,16 @@ static int uhci_result_isochronous(struct uhci_hcd *uhci, struct urb *urb)
 	i = 0;
 	list_for_each_entry(td, &urbp->td_list, list) {
 		int actlength;
+		unsigned int ctrlstat = td_status(td);
 
-		if (td_status(td) & TD_CTRL_ACTIVE)
+		if (ctrlstat & TD_CTRL_ACTIVE)
 			return -EINPROGRESS;
 
-		actlength = uhci_actual_length(td_status(td));
+		actlength = uhci_actual_length(ctrlstat);
 		urb->iso_frame_desc[i].actual_length = actlength;
 		urb->actual_length += actlength;
 
-		status = uhci_map_status(uhci_status_bits(td_status(td)),
+		status = uhci_map_status(uhci_status_bits(ctrlstat),
 				usb_pipeout(urb->pipe));
 		urb->iso_frame_desc[i].status = status;
 		if (status) {
@@ -1423,19 +1431,21 @@ static void uhci_unlink_generic(struct uhci_hcd *uhci, struct urb *urb)
 	 */
 	head = &urbp->td_list;
 	list_for_each_entry(td, head, list) {
-		if (!(td_status(td) & TD_CTRL_ACTIVE) &&
-				(uhci_actual_length(td_status(td)) <
+		unsigned int ctrlstat = td_status(td);
+
+		if (!(ctrlstat & TD_CTRL_ACTIVE) &&
+				(uhci_actual_length(ctrlstat) <
 				 uhci_expected_length(td_token(td)) ||
 				td->list.next == head))
 			usb_settoggle(urb->dev, uhci_endpoint(td_token(td)),
 				uhci_packetout(td_token(td)),
 				uhci_toggle(td_token(td)) ^ 1);
-		else if ((td_status(td) & TD_CTRL_ACTIVE) && !prevactive)
+		else if ((ctrlstat & TD_CTRL_ACTIVE) && !prevactive)
 			usb_settoggle(urb->dev, uhci_endpoint(td_token(td)),
 				uhci_packetout(td_token(td)),
 				uhci_toggle(td_token(td)));
 
-		prevactive = td_status(td) & TD_CTRL_ACTIVE;
+		prevactive = ctrlstat & TD_CTRL_ACTIVE;
 	}
 
 	uhci_delete_queued_urb(uhci, urb);

drivers/usb/host/uhci-hcd.h

@@ -118,10 +118,20 @@ struct uhci_qh {
 	struct list_head remove_list;	/* P: uhci->remove_list_lock */
 } __attribute__((aligned(16)));
 
+/*
+ * We need a special accessor for the element pointer because it is
+ * subject to asynchronous updates by the controller
+ */
+static __le32 inline qh_element(struct uhci_qh *qh) {
+	__le32 element = qh->element;
+
+	barrier();
+	return element;
+}
+
 /*
  * for TD <status>:
  */
-#define td_status(td)		le32_to_cpu((td)->status)
 #define TD_CTRL_SPD		(1 << 29)	/* Short Packet Detect */
 #define TD_CTRL_C_ERR_MASK	(3 << 27)	/* Error Counter bits */
 #define TD_CTRL_C_ERR_SHIFT	27
@@ -203,6 +213,18 @@ struct uhci_td {
 	struct list_head fl_list;	/* P: uhci->frame_list_lock */
 } __attribute__((aligned(16)));
 
+/*
+ * We need a special accessor for the control/status word because it is
+ * subject to asynchronous updates by the controller
+ */
+static u32 inline td_status(struct uhci_td *td) {
+	__le32 status = td->status;
+
+	barrier();
+	return le32_to_cpu(status);
+}
+
+
 /*
  * The UHCI driver places Interrupt, Control and Bulk into QH's both
  * to group together TD's for one transfer, and also to faciliate queuing