>On 24-08-21 06:07:42, Pawel Laszczak wrote:
>> Stop Endpoint command on LINK TRB with TC bit set to 1 causes that
>> internal cycle bit can have incorrect state after command complete.
>
>You mean this issue is: the transfer ring is on the LINK TRB when stop endpoint
>command is going to execute?
Yes, but also TC bit must be set in such LINK TRB.
>
>What's the use case we could find this issue?
I was not able to recreate this issue with standard linux and windows drivers.
To recreate this issue I used:
1. device with 2 x ACM connected to Windows OS host
2. UartAssist V5.0.10.exe
3. Vendor specific ACM driver
Test scenario will open & close port many times during testing.
Host driver should send clear feature (halt) request to device
when application open the acm function port.
Thanks,
Pawel
>
>Peter
>
>> In consequence empty transfer ring can be incorrectly detected when EP
>> is resumed.
>> NOP TRB before LINK TRB avoid such scenario. Stop Endpoint command is
>> then on NOP TRB and internal cycle bit is not changed and have correct
>> value.
>>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> USBSSP DRD Driver")
>> cc: <stable@xxxxxxxxxxxxxxx>
>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>> ---
>> drivers/usb/cdns3/cdnsp-gadget.h | 3 +++
>> drivers/usb/cdns3/cdnsp-ring.c | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h
>> b/drivers/usb/cdns3/cdnsp-gadget.h
>> index e1b5801fdddf..9a5577a772af 100644
>> --- a/drivers/usb/cdns3/cdnsp-gadget.h
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.h
>> @@ -811,6 +811,7 @@ struct cdnsp_stream_info {
>> * generate Missed Service Error Event.
>> * Set skip flag when receive a Missed Service Error Event and
>> * process the missed tds on the endpoint ring.
>> + * @wa1_nop_trb: hold pointer to NOP trb.
>> */
>> struct cdnsp_ep {
>> struct usb_ep endpoint;
>> @@ -838,6 +839,8 @@ struct cdnsp_ep {
>> #define EP_UNCONFIGURED BIT(7)
>>
>> bool skip;
>> + union cdnsp_trb *wa1_nop_trb;
>> +
>> };
>>
>> /**
>> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> b/drivers/usb/cdns3/cdnsp-ring.c index 275a6a2fa671..75724e60653c
>> 100644
>> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> @@ -1904,6 +1904,23 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device
>*pdev, struct cdnsp_request *preq)
>> if (ret)
>> return ret;
>>
>> + /*
>> + * workaround 1: STOP EP command on LINK TRB with TC bit set to 1
>> + * causes that internal cycle bit can have incorrect state after
>> + * command complete. In consequence empty transfer ring can be
>> + * incorrectly detected when EP is resumed.
>> + * NOP TRB before LINK TRB avoid such scenario. STOP EP command is
>> + * then on NOP TRB and internal cycle bit is not changed and have
>> + * correct value.
>> + */
>> + if (pep->wa1_nop_trb) {
>> + field = le32_to_cpu(pep->wa1_nop_trb->trans_event.flags);
>> + field ^= TRB_CYCLE;
>> +
>> + pep->wa1_nop_trb->trans_event.flags = cpu_to_le32(field);
>> + pep->wa1_nop_trb = NULL;
>> + }
>> +
>> /*
>> * Don't give the first TRB to the hardware (by toggling the cycle bit)
>> * until we've finished creating all the other TRBs. The ring's
>> cycle @@ -1999,6 +2016,17 @@ int cdnsp_queue_bulk_tx(struct
>cdnsp_device *pdev, struct cdnsp_request *preq)
>> send_addr = addr;
>> }
>>
>> + if (cdnsp_trb_is_link(ring->enqueue + 1)) {
>> + field = TRB_TYPE(TRB_TR_NOOP) | TRB_IOC;
>> + if (!ring->cycle_state)
>> + field |= TRB_CYCLE;
>> +
>> + pep->wa1_nop_trb = ring->enqueue;
>> +
>> + cdnsp_queue_trb(pdev, ring, 0, 0x0, 0x0,
>> + TRB_INTR_TARGET(0), field);
>> + }
>> +
>> cdnsp_check_trb_math(preq, enqd_len);
>> ret = cdnsp_giveback_first_trb(pdev, pep, preq->request.stream_id,
>> start_cycle, start_trb);
>> --
>> 2.43.0
>>
