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

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

[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 <yd_tseng@asmedia.com.tw>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

---
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(-)

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

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

[Sergei Shtylyov]

Hello.

On 10/29/2015 5:46 AM, 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>
>
> ---
> 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(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index fa83625..8edc286 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
[...]
> @@ -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;

    Forgot {}?

>   	}
>
>   	start_frame = readl(&xhci->run_regs->microframe_index);

MBR, Sergei

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

[Lu, Baolu]

On 10/29/2015 08:51 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 10/29/2015 5:46 AM, 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>
>>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index fa83625..8edc286 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
> [...]
>> @@ -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;
>
>    Forgot {}?

Oh, I am sorry. I am wondering how it passed my test.

I will send v3 patch soon any way.

>
>>       }
>>
>>       start_frame = readl(&xhci->run_regs->microframe_index);
>
> MBR, Sergei
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Mathias Nyman]

On 29.10.2015 14:58, Lu, Baolu wrote:
>
>
> On 10/29/2015 08:51 PM, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 10/29/2015 5:46 AM, 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>
>>>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index fa83625..8edc286 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>> [...]
>>> @@ -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;
>>
>>    Forgot {}?
>
> 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

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

[Lu, Baolu]

On 10/29/2015 10:08 PM, Mathias Nyman wrote:
> On 29.10.2015 14:58, Lu, Baolu wrote:
>>
>>
>> On 10/29/2015 08:51 PM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 10/29/2015 5:46 AM, 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>
>>>>
>>>> ---
>>>> 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(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-ring.c 
>>>> b/drivers/usb/host/xhci-ring.c
>>>> index fa83625..8edc286 100644
>>>> --- a/drivers/usb/host/xhci-ring.c
>>>> +++ b/drivers/usb/host/xhci-ring.c
>>> [...]
>>>> @@ -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;
>>>
>>>    Forgot {}?
>>
>> 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_...

Done.

>
> 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
> -- 
> 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/
>