linux-kernel - RE: [PATCH v2] usb: cdnsp: Fix issue with detecting command completion event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <PH7PR07MB9538ADAC2E132FA1AD380EB5DD91A@PH7PR07MB9538.namprd07.prod.outlook.com>
Date: Wed, 14 May 2025 04:54:58 +0000
From: Pawel Laszczak <pawell@...ence.com>
To: "Peter Chen (CIX)" <peter.chen@...nel.org>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH v2] usb: cdnsp: Fix issue with detecting command
 completion event

>
>
>On 25-05-13 05:30:09, Pawel Laszczak wrote:
>> In some cases, there is a small-time gap in which CMD_RING_BUSY can be
>> cleared by controller but adding command completion event to event
>> ring will be delayed. As the result driver will return error code.
>> This behavior has been detected on usbtest driver (test 9) with
>> configuration including ep1in/ep1out bulk and ep2in/ep2out isoc
>> endpoint.
>> Probably this gap occurred because controller was busy with adding
>> some other events to event ring.
>> The CMD_RING_BUSY is cleared to '0' when the Command Descriptor has
>> been executed and not when command completion event has been added to
>> event ring.
>>
>> To fix this issue for this test the small delay is sufficient less
>> than 10us) but to make sure the problem doesn't happen again in the
>> future the patch introduces 10 retries to check with delay about 20us
>> before returning error code.
>
>Does the ./scripts/checkpatch.pl report warning if the delay time is 20us for
>udelay?

Yes, it is reported:
CHECK: usleep_range is preferred over udelay; ...

But only for checkpatch.pl  with --strict option

Pawel

>
>Peter
>>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> USBSSP DRD Driver")
>> cc: stable@...r.kernel.org
>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>> ---
>> Changelog:
>> v2:
>> - replaced usleep_range with udelay
>> - increased retry counter and decreased the udelay value
>>
>>  drivers/usb/cdns3/cdnsp-gadget.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
>> b/drivers/usb/cdns3/cdnsp-gadget.c
>> index 4824a10df07e..58650b7f4173 100644
>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>> @@ -547,6 +547,7 @@ int cdnsp_wait_for_cmd_compl(struct cdnsp_device
>*pdev)
>>  	dma_addr_t cmd_deq_dma;
>>  	union cdnsp_trb *event;
>>  	u32 cycle_state;
>> +	u32 retry = 10;
>>  	int ret, val;
>>  	u64 cmd_dma;
>>  	u32  flags;
>> @@ -578,8 +579,23 @@ int cdnsp_wait_for_cmd_compl(struct cdnsp_device
>*pdev)
>>  		flags = le32_to_cpu(event->event_cmd.flags);
>>
>>  		/* Check the owner of the TRB. */
>> -		if ((flags & TRB_CYCLE) != cycle_state)
>> +		if ((flags & TRB_CYCLE) != cycle_state) {
>> +			/*
>> +			 * Give some extra time to get chance controller
>> +			 * to finish command before returning error code.
>> +			 * Checking CMD_RING_BUSY is not sufficient because
>> +			 * this bit is cleared to '0' when the Command
>> +			 * Descriptor has been executed by controller
>> +			 * and not when command completion event has
>> +			 * be added to event ring.
>> +			 */
>> +			if (retry--) {
>> +				udelay(20);
>> +				continue;
>> +			}
>> +
>>  			return -EINVAL;
>> +		}
>>
>>  		cmd_dma = le64_to_cpu(event->event_cmd.cmd_trb);
>>
>> --
>> 2.43.0
>>
>
>--
>
>Best regards,
>Peter