usb: xhci: clean up error_bitmask usage

PATCH Superseded Lu Baolu 2 participants 3 messages Oct 19, 2016 – Oct 20, 2016

Subsystems (2)

USB USB xHCI driver

lore

Share & tools

lore

Lu Baolu

author

2016-10-19 00:54 UTC #0 lore raw

PATCH 1/1 usb: xhci: clean up error_bitmask usage

In xhci_handle_event(), when errors are detected, driver always sets a bit in error_bitmask (one member of the xhci private driver data). That means users have to retrieve and decode the value of error_bitmask in xhci private driver data if they want to know whether those erros ever happened in xhci_handle_event(). Otherwise, those errors are just ignored silently.

This patch cleans up this by replacing the setting of error_bitmask with the kernel print functions, so that users can easily check and report the errors happened in xhci_handle_event().

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/usb/host/xhci-ring.c | 42 +++++++++++++++++++-----------------------
 drivers/usb/host/xhci.h      |  2 --
 2 files changed, 19 insertions(+), 25 deletions(-)

drivers/usb/host/xhci-ring.c +19 ~~-23~~

index 797137e..a460423 100644
@@ -1183,7 +1183,7 @@ static void xhci_handle_cmd_nec_get_fw(struct xhci_hcd *xhci,
 		struct xhci_event_cmd *event)
 {
 	if (!(xhci->quirks & XHCI_NEC_HOST)) {
-		xhci->error_bitmask |= 1 << 6;
+		xhci_warn(xhci, "WARN NEC_GET_FW command on non-NEC host\n");
 		return;
 	}
 	xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,

@@ -1325,14 +1325,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
 	cmd_trb = xhci->cmd_ring->dequeue;
 	cmd_dequeue_dma = xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg,
 			cmd_trb);
-	/* Is the command ring deq ptr out of sync with the deq seg ptr? */
-	if (cmd_dequeue_dma == 0) {
-		xhci->error_bitmask |= 1 << 4;
-		return;
-	}
-	/* Does the DMA address match our internal dequeue pointer address? */
-	if (cmd_dma != (u64) cmd_dequeue_dma) {
-		xhci->error_bitmask |= 1 << 5;
+	/*
+	 * Check whether the completion event is for our internal kept
+	 * command.
+	 */
+	if (!cmd_dequeue_dma || cmd_dma != (u64)cmd_dequeue_dma) {
+		xhci_warn(xhci,
+			  "ERROR mismatched command completion event\n");
 		return;
 	}

@@ -1418,7 +1417,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
 		break;
 	default:
 		/* Skip over unknown commands on the event ring */
-		xhci->error_bitmask |= 1 << 6;
+		xhci_info(xhci, "INFO unknown command type %d\n", cmd_type);
 		break;
 	}

@@ -1521,7 +1520,7 @@ static void handle_port_status(struct xhci_hcd *xhci,
 	/* Port status change events always have a successful completion code */
 	if (GET_COMP_CODE(le32_to_cpu(event->generic.field[2])) != COMP_SUCCESS) {
 		xhci_warn(xhci, "WARN: xHC returned failed port status event\n");
-		xhci->error_bitmask |= 1 << 8;
+		return;
 	}
 	port_id = GET_PORT_ID(le32_to_cpu(event->generic.field[0]));
 	xhci_dbg(xhci, "Port Status Change Event for port %d\n", port_id);

@@ -2642,20 +2641,18 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
 {
 	union xhci_trb *event;
 	int update_ptrs = 1;
-	int ret;
 
+	/* Event ring hasn't been allocated yet. */
 	if (!xhci->event_ring || !xhci->event_ring->dequeue) {
-		xhci->error_bitmask |= 1 << 1;
-		return 0;
+		xhci_err(xhci, "ERROR event ring not ready\n");
+		return -ENOMEM;
 	}
 
 	event = xhci->event_ring->dequeue;
 	/* Does the HC or OS own the TRB? */
 	if ((le32_to_cpu(event->event_cmd.flags) & TRB_CYCLE) !=
-	    xhci->event_ring->cycle_state) {
-		xhci->error_bitmask |= 1 << 2;
+	    xhci->event_ring->cycle_state)
 		return 0;
-	}
 
 	/*
 	 * Barrier between reading the TRB_CYCLE (valid) flag above and any

@@ -2663,7 +2660,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
 	 */
 	rmb();
 	/* FIXME: Handle more event types. */
-	switch ((le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK)) {
+	switch (le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK) {
 	case TRB_TYPE(TRB_COMPLETION):
 		handle_cmd_completion(xhci, &event->event_cmd);
 		break;

@@ -2672,10 +2669,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
 		update_ptrs = 0;
 		break;
 	case TRB_TYPE(TRB_TRANSFER):
-		ret = handle_tx_event(xhci, &event->trans_event);
-		if (ret < 0)
-			xhci->error_bitmask |= 1 << 9;
-		else
+		if (!handle_tx_event(xhci, &event->trans_event))
 			update_ptrs = 0;
 		break;
 	case TRB_TYPE(TRB_DEV_NOTE):

@@ -2686,7 +2680,9 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
 		    TRB_TYPE(48))
 			handle_vendor_event(xhci, event);
 		else
-			xhci->error_bitmask |= 1 << 3;
+			xhci_warn(xhci, "ERROR unknown event type %d\n",
+				  le32_to_cpu(event->event_cmd.flags) &
+				  TRB_TYPE_BITMASK);
 	}
 	/* Any of the above functions may drop and re-acquire the lock, so check
 	 * to make sure a watchdog timer didn't mark the host as non-responsive.

drivers/usb/host/xhci.h +0 -2

index b2c1dc5..ab0393d 100644
@@ -1616,8 +1616,6 @@ struct xhci_hcd {
 #define XHCI_STATE_DYING	(1 << 0)
 #define XHCI_STATE_HALTED	(1 << 1)
 #define XHCI_STATE_REMOVING	(1 << 2)
-	/* Statistics */
-	int			error_bitmask;
 	unsigned int		quirks;
 #define	XHCI_LINK_TRB_QUIRK	(1 << 0)
 #define XHCI_RESET_EP_QUIRK	(1 << 1)

Signature · 1 line

2.1.4

Mathias Nyman

2016-10-19 06:52 UTC #1 lore raw

Quoted 7 lines · Lu Baolu

In xhci_handle_event(), when errors are detected, driver always sets a bit in error_bitmask (one member of the xhci private driver data). That means users have to retrieve and decode the value of error_bitmask in xhci private driver data if they want to know whether those erros ever happened in xhci_handle_event(). Otherwise, those errors are just ignored silently.

Quoted 5 lines

This patch cleans up this by replacing the setting of error_bitmask with the kernel print functions, so that users can easily check and report the errors happened in xhci_handle_event().
Signed-off-by Lu Baolu <[email protected]>

Nice to get this cleaned out. I think the error_bitmask was something used during driver development but has no function anymore.

A few minor comments below

Quoted 50 lines

---
  drivers/usb/host/xhci-ring.c | 42 +++++++++++++++++++-----------------------
  drivers/usb/host/xhci.h      |  2 --
  2 files changed, 19 insertions(+), 25 deletions(-)

drivers/usb/host/xhci-ring.c +9 ~~-10~~

index 797137e..a460423 100644
@@ -1183,7 +1183,7 @@ static void xhci_handle_cmd_nec_get_fw(struct xhci_hcd *xhci,
  		struct xhci_event_cmd *event)
  {
  	if (!(xhci->quirks & XHCI_NEC_HOST)) {
-		xhci->error_bitmask |= 1 << 6;
+		xhci_warn(xhci, "WARN NEC_GET_FW command on non-NEC host\n");
  		return;
  	}
  	xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,

@@ -1325,14 +1325,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
  	cmd_trb = xhci->cmd_ring->dequeue;
  	cmd_dequeue_dma = xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg,
  			cmd_trb);
-	/* Is the command ring deq ptr out of sync with the deq seg ptr? */
-	if (cmd_dequeue_dma == 0) {
-		xhci->error_bitmask |= 1 << 4;
-		return;
-	}
-	/* Does the DMA address match our internal dequeue pointer address? */
-	if (cmd_dma != (u64) cmd_dequeue_dma) {
-		xhci->error_bitmask |= 1 << 5;
+	/*
+	 * Check whether the completion event is for our internal kept
+	 * command.
+	 */
+	if (!cmd_dequeue_dma || cmd_dma != (u64)cmd_dequeue_dma) {
+		xhci_warn(xhci,
+			  "ERROR mismatched command completion event\n");
  		return;
  	}

@@ -1418,7 +1417,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
  		break;
  	default:
  		/* Skip over unknown commands on the event ring */
-		xhci->error_bitmask |= 1 << 6;
+		xhci_info(xhci, "INFO unknown command type %d\n", cmd_type);
  		break;
  	}

drivers/usb/host/xhci-ring.c static void handle_port_status(struct xhci_hcd *xhci, · line 1523

Quoted 5 lines

drivers/usb/host/xhci-ring.c +1 -1

@@ -1521,7 +1520,7 @@ static void handle_port_status(struct xhci_hcd *xhci,
  	/* Port status change events always have a successful completion code */
  	if (GET_COMP_CODE(le32_to_cpu(event->generic.field[2])) != COMP_SUCCESS) {
  		xhci_warn(xhci, "WARN: xHC returned failed port status event\n");
-		xhci->error_bitmask |= 1 << 8;
+		return;

I don't think we should return here, it will mess up the event ring dequeue pointer. And a non-expected completion code here should not prevent us from working normally

Quoted 38 lines

drivers/usb/host/xhci-ring.c +5 -7

  	}
  	port_id = GET_PORT_ID(le32_to_cpu(event->generic.field[0]));
  	xhci_dbg(xhci, "Port Status Change Event for port %d\n", port_id);

@@ -2642,20 +2641,18 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
  {
  	union xhci_trb *event;
  	int update_ptrs = 1;
-	int ret;
 
+	/* Event ring hasn't been allocated yet. */
  	if (!xhci->event_ring || !xhci->event_ring->dequeue) {
-		xhci->error_bitmask |= 1 << 1;
-		return 0;
+		xhci_err(xhci, "ERROR event ring not ready\n");
+		return -ENOMEM;
  	}
 
  	event = xhci->event_ring->dequeue;
  	/* Does the HC or OS own the TRB? */
  	if ((le32_to_cpu(event->event_cmd.flags) & TRB_CYCLE) !=
-	    xhci->event_ring->cycle_state) {
-		xhci->error_bitmask |= 1 << 2;
+	    xhci->event_ring->cycle_state)
  		return 0;
-	}
 
  	/*
  	 * Barrier between reading the TRB_CYCLE (valid) flag above and any

@@ -2663,7 +2660,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
  	 */
  	rmb();
  	/* FIXME: Handle more event types. */
-	switch ((le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK)) {
+	switch (le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK) {
  	case TRB_TYPE(TRB_COMPLETION):
  		handle_cmd_completion(xhci, &event->event_cmd);
  		break;

drivers/usb/host/xhci-ring.c static int xhci_handle_event(struct xhci_hcd *xhci) · line 2673

Quoted 9 lines

drivers/usb/host/xhci-ring.c +1 -4

@@ -2672,10 +2669,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
  		update_ptrs = 0;
  		break;
  	case TRB_TYPE(TRB_TRANSFER):
-		ret = handle_tx_event(xhci, &event->trans_event);
-		if (ret < 0)
-			xhci->error_bitmask |= 1 << 9;
-		else
+		if (!handle_tx_event(xhci, &event->trans_event))
  			update_ptrs = 0;

had to check this, it works because handle_tx_event() doesn't return positive values.

drivers/usb/host/xhci-ring.c static int xhci_handle_event(struct xhci_hcd *xhci) · line 2685

Quoted 10 lines

drivers/usb/host/xhci-ring.c +3 -1

  		break;
  	case TRB_TYPE(TRB_DEV_NOTE):

@@ -2686,7 +2680,9 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
  		    TRB_TYPE(48))
  			handle_vendor_event(xhci, event);
  		else
-			xhci->error_bitmask |= 1 << 3;
+			xhci_warn(xhci, "ERROR unknown event type %d\n",
+				  le32_to_cpu(event->event_cmd.flags) &
+				  TRB_TYPE_BITMASK);

This trb type is still shifted 10 bits when printing it out. There are not yet any proper helpers or macros for this it seems

-Mathias

Lu Baolu

author

2016-10-20 00:35 UTC #2 lore raw

Hi,

Quoted 14 lines · Mathias Nyman

Hi
Quoted 11 lines · Lu Baolu
In xhci_handle_event(), when errors are detected, driver always sets a bit in error_bitmask (one member of the xhci private driver data). That means users have to retrieve and decode the value of error_bitmask in xhci private driver data if they want to know whether those erros ever happened in xhci_handle_event(). Otherwise, those errors are just ignored silently.
This patch cleans up this by replacing the setting of error_bitmask with the kernel print functions, so that users can easily check and report the errors happened in xhci_handle_event().

Quoted 6 lines

Quoted 1 line
Signed-off-by Lu Baolu <[email protected]>
Nice to get this cleaned out. I think the error_bitmask was something used during driver development but has no function anymore.
A few minor comments below

Thanks for your comments.

Quoted 50 lines

---
  drivers/usb/host/xhci-ring.c | 42 +++++++++++++++++++-----------------------
  drivers/usb/host/xhci.h      |  2 --
  2 files changed, 19 insertions(+), 25 deletions(-)

drivers/usb/host/xhci-ring.c +9 ~~-10~~

index 797137e..a460423 100644
@@ -1183,7 +1183,7 @@ static void xhci_handle_cmd_nec_get_fw(struct xhci_hcd *xhci,
          struct xhci_event_cmd *event)
  {
      if (!(xhci->quirks & XHCI_NEC_HOST)) {
-        xhci->error_bitmask |= 1 << 6;
+        xhci_warn(xhci, "WARN NEC_GET_FW command on non-NEC host\n");
          return;
      }
      xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,

@@ -1325,14 +1325,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
      cmd_trb = xhci->cmd_ring->dequeue;
      cmd_dequeue_dma = xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg,
              cmd_trb);
-    /* Is the command ring deq ptr out of sync with the deq seg ptr? */
-    if (cmd_dequeue_dma == 0) {
-        xhci->error_bitmask |= 1 << 4;
-        return;
-    }
-    /* Does the DMA address match our internal dequeue pointer address? */
-    if (cmd_dma != (u64) cmd_dequeue_dma) {
-        xhci->error_bitmask |= 1 << 5;
+    /*
+     * Check whether the completion event is for our internal kept
+     * command.
+     */
+    if (!cmd_dequeue_dma || cmd_dma != (u64)cmd_dequeue_dma) {
+        xhci_warn(xhci,
+              "ERROR mismatched command completion event\n");
          return;
      }

@@ -1418,7 +1417,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
          break;
      default:
          /* Skip over unknown commands on the event ring */
-        xhci->error_bitmask |= 1 << 6;
+        xhci_info(xhci, "INFO unknown command type %d\n", cmd_type);
          break;
      }

Quoted 9 lines

drivers/usb/host/xhci-ring.c static void handle_port_status(struct xhci_hcd *xhci, · line 1523
Quoted 6 lines
drivers/usb/host/xhci-ring.c +1 -1
@@ -1521,7 +1520,7 @@ static void handle_port_status(struct xhci_hcd *xhci,
      /* Port status change events always have a successful completion code */
      if (GET_COMP_CODE(le32_to_cpu(event->generic.field[2])) != COMP_SUCCESS) {
          xhci_warn(xhci, "WARN: xHC returned failed port status event\n");
-        xhci->error_bitmask |= 1 << 8;
+        return;
I don't think we should return here, it will mess up the event ring dequeue pointer. And a non-expected completion code here should not prevent us from working normally

Ah! yes.

I though we don't need to handle a unsuccessful command. Thanks for pointing this out. I will fix it in the v2 patch.

Quoted 43 lines

drivers/usb/host/xhci-ring.c +5 -7

      }
      port_id = GET_PORT_ID(le32_to_cpu(event->generic.field[0]));
      xhci_dbg(xhci, "Port Status Change Event for port %d\n", port_id);

@@ -2642,20 +2641,18 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
  {
      union xhci_trb *event;
      int update_ptrs = 1;
-    int ret;
 
+    /* Event ring hasn't been allocated yet. */
      if (!xhci->event_ring || !xhci->event_ring->dequeue) {
-        xhci->error_bitmask |= 1 << 1;
-        return 0;
+        xhci_err(xhci, "ERROR event ring not ready\n");
+        return -ENOMEM;
      }
 
      event = xhci->event_ring->dequeue;
      /* Does the HC or OS own the TRB? */
      if ((le32_to_cpu(event->event_cmd.flags) & TRB_CYCLE) !=
-        xhci->event_ring->cycle_state) {
-        xhci->error_bitmask |= 1 << 2;
+        xhci->event_ring->cycle_state)
          return 0;
-    }
 
      /*
       * Barrier between reading the TRB_CYCLE (valid) flag above and any

@@ -2663,7 +2660,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
       */
      rmb();
      /* FIXME: Handle more event types. */
-    switch ((le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK)) {
+    switch (le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK) {
      case TRB_TYPE(TRB_COMPLETION):
          handle_cmd_completion(xhci, &event->event_cmd);
          break;

@@ -2672,10 +2669,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
          update_ptrs = 0;
          break;
      case TRB_TYPE(TRB_TRANSFER):

Quoted 8 lines

drivers/usb/host/xhci-ring.c static int xhci_handle_event(struct xhci_hcd *xhci) · line 2673
Quoted 6 lines
drivers/usb/host/xhci-ring.c +1 -4
-        ret = handle_tx_event(xhci, &event->trans_event);
-        if (ret < 0)
-            xhci->error_bitmask |= 1 << 9;
-        else
+        if (!handle_tx_event(xhci, &event->trans_event))
              update_ptrs = 0;
had to check this, it works because handle_tx_event() doesn't return positive values.

Yes. How about below code?

        ret = handle_tx_event(xhci, &event->trans_event);
        if (ret >=0)
             update_ptrs = 0;

Quoted 14 lines

drivers/usb/host/xhci-ring.c static int xhci_handle_event(struct xhci_hcd *xhci) · line 2685
Quoted 11 lines
drivers/usb/host/xhci-ring.c +3 -1
          break;
      case TRB_TYPE(TRB_DEV_NOTE):
@@ -2686,7 +2680,9 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
              TRB_TYPE(48))
              handle_vendor_event(xhci, event);
          else
-            xhci->error_bitmask |= 1 << 3;
+            xhci_warn(xhci, "ERROR unknown event type %d\n",
+                  le32_to_cpu(event->event_cmd.flags) &
+                  TRB_TYPE_BITMASK);
This trb type is still shifted 10 bits when printing it out. There are not yet any proper helpers or macros for this it seems

Okay, I will fix this.

Quoted 3 lines

-Mathias

Best regards,
Lu Baolu