Skip to contentSkip to conversation

usb: xhci: fix checking ep busy for CFC

PATCH Superseded Lu Baolu 2 participants 3 messages Oct 28, 2015 – Oct 29, 2015

Subsystems (2)
USB USB xHCI driver

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Lu Baolu

author
#0 lore raw

PATCH 1/1 usb: xhci: fix checking ep busy for CFC

Function ep_ring_is_processing() checks the dequeue pointer in endpoint context to know whether an endpoint is busy with processing TRBs. This is not correct since dequeue pointer field in an endpoint context is only valid when the endpoint is in Halted or Stopped states. This buggy code causes audio noise when playing sound with USB headset connected to host controllers which support CFC (one of xhci 1.1 features).

This patch should exist in stable kernel since v4.3.

Reported-and-tested-by YD Tseng <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/usb/host/xhci-ring.c +1 -4
index fa83625..2b50d45 100644
@@ -3903,7 +3903,6 @@ static int ep_ring_is_processing(struct xhci_hcd *xhci,
 	struct xhci_ring *ep_ring;
 	struct xhci_ep_ctx *ep_ctx;
 	struct xhci_virt_ep *xep;
-	dma_addr_t hw_deq;
 
 	xdev = xhci->devs[slot_id];
 	xep = &xhci->devs[slot_id]->eps[ep_index];

@@ -3913,9 +3912,7 @@ static int ep_ring_is_processing(struct xhci_hcd *xhci,
 	if ((le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK) != EP_STATE_RUNNING)
 		return 0;
 
-	hw_deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
-	return (hw_deq !=
-		xhci_trb_virt_to_dma(ep_ring->enq_seg, ep_ring->enqueue));
+	return !list_empty(&ep_ring->td_list);
 }
 
 /*
Signature · 1 line
2.1.4
maintainer
#1 lore raw
Quoted 29 lines · Lu Baolu

Function ep_ring_is_processing() checks the dequeue pointer in endpoint context to know whether an endpoint is busy with processing TRBs. This is not correct since dequeue pointer field in an endpoint context is only valid when the endpoint is in Halted or Stopped states. This buggy code causes audio noise when playing sound with USB headset connected to host controllers which support CFC (one of xhci 1.1 features).

This patch should exist in stable kernel since v4.3.

Reported-and-tested-by YD Tseng <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
  drivers/usb/host/xhci-ring.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)
drivers/usb/host/xhci-ring.c +0 -1
index fa83625..2b50d45 100644
@@ -3903,7 +3903,6 @@ static int ep_ring_is_processing(struct xhci_hcd *xhci,
  	struct xhci_ring *ep_ring;
  	struct xhci_ep_ctx *ep_ctx;
  	struct xhci_virt_ep *xep;
-	dma_addr_t hw_deq;
 
  	xdev = xhci->devs[slot_id];
  	xep = &xhci->devs[slot_id]->eps[ep_index];

drivers/usb/host/xhci-ring.c static int ep_ring_is_processing(struct xhci_hcd *xhci, · line 3916

Quoted 8 lines
drivers/usb/host/xhci-ring.c +1 -3
@@ -3913,9 +3912,7 @@ static int ep_ring_is_processing(struct xhci_hcd *xhci,
  	if ((le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK) != EP_STATE_RUNNING)
  		return 0;
 
-	hw_deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
-	return (hw_deq !=
-		xhci_trb_virt_to_dma(ep_ring->enq_seg, ep_ring->enqueue));
+	return !list_empty(&ep_ring->td_list);
  }

Would it make sense to remove the ep_ring_is_processing() function completely?

It is only called in one place, and the main use is checking list_empty(&ep_ring->td_list). This could be checked directly in xhci_queue_isoc_tx_prepare()

-Mathias

author
#2 lore raw
Quoted 44 lines · Mathias Nyman
Quoted 30 lines · Lu Baolu

Function ep_ring_is_processing() checks the dequeue pointer in endpoint context to know whether an endpoint is busy with processing TRBs. This is not correct since dequeue pointer field in an endpoint context is only valid when the endpoint is in Halted or Stopped states. This buggy code causes audio noise when playing sound with USB headset connected to host controllers which support CFC (one of xhci 1.1 features).

This patch should exist in stable kernel since v4.3.

Reported-and-tested-by YD Tseng <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
  drivers/usb/host/xhci-ring.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)
drivers/usb/host/xhci-ring.c +0 -1
index fa83625..2b50d45 100644
@@ -3903,7 +3903,6 @@ static int ep_ring_is_processing(struct 
 xhci_hcd *xhci,
      struct xhci_ring *ep_ring;
      struct xhci_ep_ctx *ep_ctx;
      struct xhci_virt_ep *xep;
-    dma_addr_t hw_deq;
 
      xdev = xhci->devs[slot_id];

      xep = &xhci->devs[slot_id]->eps[ep_index];

drivers/usb/host/xhci-ring.c static int ep_ring_is_processing(struct · line 3918

Quoted 10 lines
drivers/usb/host/xhci-ring.c +1 -3
@@ -3913,9 +3912,7 @@ static int ep_ring_is_processing(struct 
 xhci_hcd *xhci,
      if ((le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK) != 
 EP_STATE_RUNNING)
          return 0;
 
-    hw_deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
-    return (hw_deq !=
-        xhci_trb_virt_to_dma(ep_ring->enq_seg, ep_ring->enqueue));
+    return !list_empty(&ep_ring->td_list);
  }

Would it make sense to remove the ep_ring_is_processing() function completely?

Quoted 2 lines

It is only called in one place, and the main use is checking list_empty(&ep_ring->td_list). This could be checked directly in xhci_queue_isoc_tx_prepare()

Fair enough. I will make this change and send out the v2.

Quoted 2 lines

-Mathias

Thanks,
Baolu

Quoted 7 lines
Signature · 5 lines
-- 
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/