Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1257557 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] usb: xhci: fix checking ep busy for CFC Lu Baolu <baolu.lu@linux.intel.com> - 2015-10-28 02:40 +0100
    Re: [PATCH 1/1] usb: xhci: fix checking ep busy for CFC Mathias Nyman <mathias.nyman@intel.com> - 2015-10-28 14:30 +0100
      Re: [PATCH 1/1] usb: xhci: fix checking ep busy for CFC "Lu, Baolu" <baolu.lu@linux.intel.com> - 2015-10-29 02:50 +0100

#1257557 — [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 <yd_tseng@asmedia.com.tw>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fa83625..2b50d45 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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);
 }
 
 /*
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1258018

On 28.10.2015 03:36, Lu Baolu wrote:
> 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 <yd_tseng@asmedia.com.tw>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/usb/host/xhci-ring.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index fa83625..2b50d45 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -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);
>   }

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258517

On 10/28/2015 09:27 PM, Mathias Nyman wrote:
> On 28.10.2015 03:36, Lu Baolu wrote:
>> 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 <yd_tseng@asmedia.com.tw>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/usb/host/xhci-ring.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index fa83625..2b50d45 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -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);
>>   }
>
> 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()

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

>
> -Mathias

Thanks,
Baolu

>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web