usb: xhci: fix checking ep busy for CFC
Subsystems (2)
PATCH v2 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.
---v1->v2:
Implement the logic in xhci_queue_isoc_tx_prepare() instead of a seperated function as suggested by Mathias.
--- drivers/usb/host/xhci-ring.c | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-)
drivers/usb/host/xhci-ring.c +6 -26
@@ -3896,28 +3896,6 @@ cleanup:return ret; } -static int ep_ring_is_processing (struct xhci_hcd *xhci, -int slot_id,unsigned int ep_index) -{ -struct xhci_virt_device *xdev; -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]; - ep_ring = xep->ring; - ep_ctx =xhci_get_ep_ctx (xhci, xdev->out_ctx, ep_index); - -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)); -} -/* * Check transfer ring to guarantee there is enough room for the urb. * Update ISO URB start_frame and interval.
@@ -3983,10 +3961,12 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, }/* Calculate the start frame and put it in urb->start_frame. */ -if (HCC_CFC (xhci->hcc_params) && -ep_ring_is_processing (xhci, slot_id, ep_index)) { - urb->start_frame = xep->next_frame_id; - goto skip_start_over; +if (HCC_CFC (xhci->hcc_params)) { +if ((le32_to_cpu (ep_ctx->ep_info) &EP_STATE_MASK ) + ==EP_STATE_RUNNING && + !list_empty (&ep_ring->td_list)) + urb->start_frame = xep->next_frame_id; + goto skip_start_over; } start_frame =readl (&xhci->run_regs->microframe_index);
Signature · 1 line
2.1.4
Hello.
Quoted 26 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]>---
v1->v2:
Implement the logic inxhci_queue_isoc_tx_prepare()instead of a seperated function as suggested by Mathias.--- drivers/usb/host/xhci-ring.c | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-)drivers/usb/host/xhci-ring.c +0
-0
[...]
Quoted 4 lines
drivers/usb/host/xhci-ring.c +0
-0@@ -3983,10 +3961,12 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, }/* Calculate the start frame and put it in urb->start_frame. */
drivers/usb/host/xhci-ring.c int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, · line 3969
Quoted 10 lines
drivers/usb/host/xhci-ring.c +6
-4-if (HCC_CFC (xhci->hcc_params) && -ep_ring_is_processing (xhci, slot_id, ep_index)) { - urb->start_frame = xep->next_frame_id; - goto skip_start_over; +if (HCC_CFC (xhci->hcc_params)) { +if ((le32_to_cpu (ep_ctx->ep_info) &EP_STATE_MASK ) + ==EP_STATE_RUNNING && + !list_empty (&ep_ring->td_list)) + urb->start_frame = xep->next_frame_id; + goto skip_start_over;
Forgot {}?
drivers/usb/host/xhci-ring.c int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, · line 3972
Quoted 3 lines
drivers/usb/host/xhci-ring.c +0
-0} start_frame =readl (&xhci->run_regs->microframe_index);
MBR, Sergei
Quoted 35 lines · Sergei Shtylyov
Hello.
Quoted 26 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]>---
v1->v2:
Implement the logic inxhci_queue_isoc_tx_prepare()instead of a seperated function as suggested by Mathias.--- drivers/usb/host/xhci-ring.c | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-)drivers/usb/host/xhci-ring.c +0
-0[...]
Quoted 4 lines
drivers/usb/host/xhci-ring.c +0
-0@@ -3983,10 +3961,12 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, }
Quoted 13 lines
drivers/usb/host/xhci-ring.c int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, · line 3969
Quoted 11 lines
drivers/usb/host/xhci-ring.c +6
-4/* Calculate the start frame and put it in urb->start_frame. */ -if (HCC_CFC (xhci->hcc_params) && -ep_ring_is_processing (xhci, slot_id, ep_index)) { - urb->start_frame = xep->next_frame_id; - goto skip_start_over; +if (HCC_CFC (xhci->hcc_params)) { +if ((le32_to_cpu (ep_ctx->ep_info) &EP_STATE_MASK ) + ==EP_STATE_RUNNING && + !list_empty (&ep_ring->td_list)) + urb->start_frame = xep->next_frame_id; + goto skip_start_over;Forgot {}?
Oh, I am sorry. I am wondering how it passed my test.
I will send v3 patch soon any way.
Quoted 12 lines
Quoted 4 lines
drivers/usb/host/xhci-ring.c +0
-0}start_frame = readl(&xhci->run_regs->microframe_index);MBR, Sergei
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 51 lines · Baolu
Quoted 34 lines · Sergei Shtylyov
Hello.
Quoted 26 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]>---
v1->v2:
Implement the logic inxhci_queue_isoc_tx_prepare()instead of a seperated function as suggested by Mathias.--- drivers/usb/host/xhci-ring.c | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-)drivers/usb/host/xhci-ring.c +0
-0[...]
Quoted 3 lines
drivers/usb/host/xhci-ring.c +0
-0@@ -3983,10 +3961,12 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, }Quoted 13 lines
drivers/usb/host/xhci-ring.c int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, · line 3969
Quoted 11 lines
drivers/usb/host/xhci-ring.c +6
-4/* Calculate the start frame and put it in urb->start_frame. */ -if (HCC_CFC (xhci->hcc_params) && -ep_ring_is_processing (xhci, slot_id, ep_index)) { - urb->start_frame = xep->next_frame_id; - goto skip_start_over; +if (HCC_CFC (xhci->hcc_params)) { +if ((le32_to_cpu (ep_ctx->ep_info) &EP_STATE_MASK ) + ==EP_STATE_RUNNING && + !list_empty (&ep_ring->td_list)) + urb->start_frame = xep->next_frame_id; + goto skip_start_over;Forgot {}?
Quoted 5 lines
Oh, I am sorry. I am wondering how it passed my test.
I will send v3 patch soon any way.
If you are anyway making a v3 then maybe one more change, just for readability, no (real) functional change:
if (HCC_CFC(xhci->hcc_params) && !list_empty(&ep_ring->td_list)) {
if (le32_to_cpu(ep_...
While thinking about code cleanup I also think we should use a local variable
u32 ep_info = le32_to_cpu(ep_ctx->ep_info) as it's used several times in xhci_queue_isoc_tx_preapare(), causing a lot of line splitting.
It should be ok as we are under the same spinlock so ep_ctx should not change.
But that is not a fix sent to a rc and stable, I can make a separate cleanup patch for it later.
-Mathias
Quoted 59 lines · Mathias Nyman
Quoted 52 lines · Baolu
Quoted 35 lines · Sergei Shtylyov
Hello.
Quoted 26 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]>---
v1->v2:
Implement the logic inxhci_queue_isoc_tx_prepare()instead of a seperated function as suggested by Mathias.--- drivers/usb/host/xhci-ring.c | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-)drivers/usb/host/xhci-ring.c +0
-0[...]
Quoted 4 lines
drivers/usb/host/xhci-ring.c +0
-0@@ -3983,10 +3961,12 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, }Quoted 13 lines
drivers/usb/host/xhci-ring.c int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags, · line 3969
Quoted 11 lines
drivers/usb/host/xhci-ring.c +6
-4/* Calculate the start frame and put it in urb->start_frame. */ -if (HCC_CFC (xhci->hcc_params) && -ep_ring_is_processing (xhci, slot_id, ep_index)) { - urb->start_frame = xep->next_frame_id; - goto skip_start_over; +if (HCC_CFC (xhci->hcc_params)) { +if ((le32_to_cpu (ep_ctx->ep_info) &EP_STATE_MASK ) + ==EP_STATE_RUNNING && + !list_empty (&ep_ring->td_list)) + urb->start_frame = xep->next_frame_id; + goto skip_start_over;Forgot {}?Quoted 5 lines
Oh, I am sorry. I am wondering how it passed my test.
I will send v3 patch soon any way.
Quoted 5 lines
If you are anyway making a v3 then maybe one more change, just for readability, no (real) functional change:
if (
HCC_CFC(xhci->hcc_params)&& !list_empty(&ep_ring->td_list)) {
if (le32_to_cpu(ep_...
Done.
Quoted 16 lines
While thinking about code cleanup I also think we should use a local variable
u32ep_info=le32_to_cpu(ep_ctx->ep_info)as it's used several times inxhci_queue_isoc_tx_preapare(), causing a lot of line splitting.It should be ok as we are under the same spinlock so
ep_ctxshould not change.But that is not a fix sent to a rc and stable, I can make a separate cleanup patch for it later.
-Mathias
Signature · 4 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/