RE: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.

From: Pawel Laszczak <pawell@cadence.com>
To: Peter Chen <peter.chen@nxp.com>
Cc: "felipe.balbi@linux.intel.com" <felipe.balbi@linux.intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"rogerq@ti.com" <rogerq@ti.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jbergsagel@ti.com" <jbergsagel@ti.com>,
	"nsekhar@ti.com" <nsekhar@ti.com>, "nm@ti.com" <nm@ti.com>,
	Suresh Punnoose <sureshp@cadence.com>,
	Rahul Kumar <kurahul@cadence.com>
Subject: RE: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
Date: Tue, 8 Oct 2019 07:57:16 +0000	[thread overview]
Message-ID: <BN7PR07MB47056F9852550657E9651DD7DD9A0@BN7PR07MB4705.namprd07.prod.outlook.com> (raw)
In-Reply-To: <20191008065619.GE5670@b29397-desktop>

Hi Peter,

>
>EXTERNAL MAIL
>
>
>On 19-10-07 07:39:11, Pawel Laszczak wrote:
>> Patch fixes issue with Halt Endnpoint Test observed
>
>%s/Endnpoint/Endpoint
>
>>
>> during using g_zero
>> driver as DUT. Bug occurred only on some testing board.
>
>g_zero is legacy, please use configfs function source_sink or loopback
>instead.

Yes, you are right, we are using both. 

>
>>
>> Endpoint can defer transition to Halted state if endpoint has pending
>> requests.
>
>The implementation of halt handling is a little complicated, you may
>consider return -EAGAIN for functional stall through usb_ep_set_halt
>from function driver if the requests are pending, it doesn't need to
>defer such kinds of functional stall.

Yes, I remember your suggestion regarding using EAGAIN, but f_mass_storage 
Driver simple check the status and try to stall endpoint again after 100 ms. 

What if Command Verifier ask for endpoint status before this time, or if 
the transfer will not be finished on time ? Then the CV test may fail. 

So solution from this patch should be more certain. 

>
>Peter
>> Patch add additional condition that allows to return correct endpoint
>> status during Get Endpoint Status request even if the halting endpoint
>> is in progress.
>>
>> Reported-by: Rahul Kumar <kurahul@cadence.com>
>> Signed-off-by: Rahul Kumar <kurahul@cadence.com>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>> ---
>>  drivers/usb/cdns3/ep0.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
>> index 44f652e8b5a2..10ae03430f34 100644
>> --- a/drivers/usb/cdns3/ep0.c
>> +++ b/drivers/usb/cdns3/ep0.c
>> @@ -234,9 +234,11 @@ static int cdns3_req_ep0_set_address(struct cdns3_device *priv_dev,
>>  static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev,
>>  				    struct usb_ctrlrequest *ctrl)
>>  {
>> +	struct cdns3_endpoint *priv_ep;
>>  	__le16 *response_pkt;
>>  	u16 usb_status = 0;
>>  	u32 recip;
>> +	u8 index;
>>
>>  	recip = ctrl->bRequestType & USB_RECIP_MASK;
>>
>> @@ -262,9 +264,13 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev,
>>  	case USB_RECIP_INTERFACE:
>>  		return cdns3_ep0_delegate_req(priv_dev, ctrl);
>>  	case USB_RECIP_ENDPOINT:
>> -		/* check if endpoint is stalled */
>> +		index = cdns3_ep_addr_to_index(ctrl->wIndex);
>> +		priv_ep = priv_dev->eps[index];
>> +
>> +		/* check if endpoint is stalled or stall is pending */
>>  		cdns3_select_ep(priv_dev, ctrl->wIndex);
>> -		if (EP_STS_STALL(readl(&priv_dev->regs->ep_sts)))
>> +		if (EP_STS_STALL(readl(&priv_dev->regs->ep_sts)) ||
>> +		    (priv_ep->flags & EP_STALL_PENDING))
>>  			usb_status =  BIT(USB_ENDPOINT_HALT);
>>  		break;
>>  	default:
>> --
>> 2.17.1
>>
>
>--
>
>Thanks,
>Peter Chen

Regards,
Pawel