Re: [PATCH] usb: cdnsp: Fix issue with detecting command completion event

["Peter Chen (CIX)" <peter.chen@kernel.org>]

On 25-05-08 06:44:20, 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
> >introduce 3 retries to check with delay about 100us before returning error
> >code
> >
> >Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP
> >DRD Driver")
> >cc: stable@vger.kernel.org
> >Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> >---
> > 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 f773518185c9..0eb11b5dd9d3 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 = 3;
> > 	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--) {
> >+				usleep_range(90, 100);
> 
> I was guided by the warning from checkpatch.pl script and changed udelay to usleep_range.
> It was wrong. In this place must be used udelay. 
> I will give some time linux community for commenting  and  I will send it again in a few days.
> 

Hi Pawel,

In the normal guide, the udelay is used for the delay less than 10us. 
Checkpatch.pl may not check the execution environment (atomic vs
non-atomic), so you get that warning. 

Please increase retry counter, and decrease the udelay value, the
atomic environment is expected to exit as soon as possible once the
hardware status has changed.

-- 

Best regards,
Peter