RE: [PATCH] usb: cdnsp: fix for Link TRB with TC

[Pawel Laszczak]

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