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

Groups > linux.kernel > #1258542 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/1] usb: xhci: fix checking ep busy for CFC Lu Baolu <baolu.lu@linux.intel.com> - 2015-10-29 03:50 +0100
    Re: [PATCH v2 1/1] usb: xhci: fix checking ep busy for CFC Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-10-29 14:00 +0100
      Re: [PATCH v2 1/1] usb: xhci: fix checking ep busy for CFC "Lu, Baolu" <baolu.lu@linux.intel.com> - 2015-10-29 14:00 +0100
        Re: [PATCH v2 1/1] usb: xhci: fix checking ep busy for CFC Mathias Nyman <mathias.nyman@linux.intel.com> - 2015-10-29 15:10 +0100
          Re: [PATCH v2 1/1] usb: xhci: fix checking ep busy for CFC "Lu, Baolu" <baolu.lu@linux.intel.com> - 2015-10-29 15:30 +0100

#1258542 — [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.

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

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

#1258768

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

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

#1258769

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
>

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

#1258809

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

#1258820

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

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