Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
@ 2019-10-07  6:39 Pawel Laszczak
  2019-10-07  8:40 ` Roger Quadros
  2019-10-08  6:56 ` Peter Chen
  0 siblings, 2 replies; 11+ messages in thread
From: Pawel Laszczak @ 2019-10-07  6:39 UTC (permalink / raw)
  To: felipe.balbi
  Cc: gregkh, linux-usb, rogerq, linux-kernel, jbergsagel, nsekhar, nm,
	sureshp, peter.chen, kurahul, Pawel Laszczak

Patch fixes issue with Halt Endnpoint Test observed during using g_zero
driver as DUT. Bug occurred only on some testing board.

Endpoint can defer transition to Halted state if endpoint has pending
requests.
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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-07  6:39 [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver Pawel Laszczak
@ 2019-10-07  8:40 ` Roger Quadros
  2019-10-08  6:56 ` Peter Chen
  1 sibling, 0 replies; 11+ messages in thread
From: Roger Quadros @ 2019-10-07  8:40 UTC (permalink / raw)
  To: Pawel Laszczak, felipe.balbi
  Cc: gregkh, linux-usb, linux-kernel, jbergsagel, nsekhar, nm,
	sureshp, peter.chen, kurahul



On 07/10/2019 09:39, Pawel Laszczak wrote:
> Patch fixes issue with Halt Endnpoint Test observed during using g_zero
> driver as DUT. Bug occurred only on some testing board.
> 
> Endpoint can defer transition to Halted state if endpoint has pending
> requests.
> 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")

Tested-by: Roger Quadros <rogerq@ti.com>


cheers,
-roger

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-07  6:39 [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver Pawel Laszczak
  2019-10-07  8:40 ` Roger Quadros
@ 2019-10-08  6:56 ` Peter Chen
  2019-10-08  7:57   ` Pawel Laszczak
  2019-10-08 12:58   ` Felipe Balbi
  1 sibling, 2 replies; 11+ messages in thread
From: Peter Chen @ 2019-10-08  6:56 UTC (permalink / raw)
  To: Pawel Laszczak
  Cc: felipe.balbi, gregkh, linux-usb, rogerq, linux-kernel,
	jbergsagel, nsekhar, nm, sureshp, kurahul

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.

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-08  6:56 ` Peter Chen
@ 2019-10-08  7:57   ` Pawel Laszczak
  2019-10-08  8:44     ` Peter Chen
  2019-10-08 12:58   ` Felipe Balbi
  1 sibling, 1 reply; 11+ messages in thread
From: Pawel Laszczak @ 2019-10-08  7:57 UTC (permalink / raw)
  To: Peter Chen
  Cc: felipe.balbi, gregkh, linux-usb, rogerq, linux-kernel,
	jbergsagel, nsekhar, nm, Suresh Punnoose, Rahul Kumar

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-08  7:57   ` Pawel Laszczak
@ 2019-10-08  8:44     ` Peter Chen
  2019-10-08  9:43       ` Roger Quadros
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Chen @ 2019-10-08  8:44 UTC (permalink / raw)
  To: Pawel Laszczak
  Cc: felipe.balbi, gregkh, linux-usb, rogerq, linux-kernel,
	jbergsagel, nsekhar, nm, Suresh Punnoose, Rahul Kumar

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

Your description is reasonable, since I have not met this issue, I would like
to know more about it: 
It occurs at CV9 or MSC test? For CV9 test, it may not try to stall non-ep0.
Which test case was failed?

Peter

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-08  8:44     ` Peter Chen
@ 2019-10-08  9:43       ` Roger Quadros
  0 siblings, 0 replies; 11+ messages in thread
From: Roger Quadros @ 2019-10-08  9:43 UTC (permalink / raw)
  To: Peter Chen, Pawel Laszczak
  Cc: felipe.balbi, gregkh, linux-usb, linux-kernel, jbergsagel,
	nsekhar, nm, Suresh Punnoose, Rahul Kumar

Peter,

On 08/10/2019 11:44, Peter Chen wrote:
>   
>>>> 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.
>>
> 
> Your description is reasonable, since I have not met this issue, I would like
> to know more about it:
> It occurs at CV9 or MSC test? For CV9 test, it may not try to stall non-ep0.
> Which test case was failed?

Since g_zero was used it would be CV9.

cheers,
-roger

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-08  6:56 ` Peter Chen
  2019-10-08  7:57   ` Pawel Laszczak
@ 2019-10-08 12:58   ` Felipe Balbi
  2019-10-09  2:28     ` Peter Chen
  2019-10-09  3:58     ` Pawel Laszczak
  1 sibling, 2 replies; 11+ messages in thread
From: Felipe Balbi @ 2019-10-08 12:58 UTC (permalink / raw)
  To: Peter Chen, Pawel Laszczak
  Cc: gregkh\, linux-usb\, rogerq\, linux-kernel\, jbergsagel\,
	nsekhar\, nm\, sureshp\, kurahul\

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> 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.

We still want fixes for those ;-)

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

-EAGAIN, IIRC is only supposed to be used for IN endpoint stalls.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-08 12:58   ` Felipe Balbi
@ 2019-10-09  2:28     ` Peter Chen
  2019-10-09  3:58     ` Pawel Laszczak
  1 sibling, 0 replies; 11+ messages in thread
From: Peter Chen @ 2019-10-09  2:28 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Pawel Laszczak, gregkh, linux-usb, rogerq, linux-kernel,
	jbergsagel, nsekhar, nm, sureshp, kurahul

On 19-10-08 15:58:57, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> writes:
> > 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.
> 
> We still want fixes for those ;-)

Yes, g_zero also uses source_sink and loopback, we just suggest the user
configfs instead of legacy modules.

> 
> >> 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.
> 
> -EAGAIN, IIRC is only supposed to be used for IN endpoint stalls.
> 

Yes, you are right.

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-08 12:58   ` Felipe Balbi
  2019-10-09  2:28     ` Peter Chen
@ 2019-10-09  3:58     ` Pawel Laszczak
  2019-10-09 13:50       ` Roger Quadros
  1 sibling, 1 reply; 11+ messages in thread
From: Pawel Laszczak @ 2019-10-09  3:58 UTC (permalink / raw)
  To: Felipe Balbi, Peter Chen
  Cc: gregkh, linux-usb, rogerq, linux-kernel, jbergsagel, nsekhar, nm,
	Suresh Punnoose, Rahul Kumar

Hi,

>
>Hi,
>
>Peter Chen <peter.chen@nxp.com> writes:
>> 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.
>
>We still want fixes for those ;-)

So, if my understanding is correct in new production systems
using legacy gadget drivers is not recommended and we are not going to fix
any more bugs there. 

The legacy gadget drivers remains in the kernel only to support the old 
products. 

So because we introduce new driver we should not worry  about legacy 
gadget drivers.

Is my understanding correct ?

pawell
 
>
>>> 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.
>
>-EAGAIN, IIRC is only supposed to be used for IN endpoint stalls.
>
>--
>balbi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-09  3:58     ` Pawel Laszczak
@ 2019-10-09 13:50       ` Roger Quadros
  2019-10-10 10:17         ` Felipe Balbi
  0 siblings, 1 reply; 11+ messages in thread
From: Roger Quadros @ 2019-10-09 13:50 UTC (permalink / raw)
  To: Pawel Laszczak, Felipe Balbi, Peter Chen
  Cc: gregkh, linux-usb, linux-kernel, jbergsagel, nsekhar, nm,
	Suresh Punnoose, Rahul Kumar

Pawel,

On 09/10/2019 06:58, Pawel Laszczak wrote:
> Hi,
> 
>>
>> Hi,
>>
>> Peter Chen <peter.chen@nxp.com> writes:
>>> 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.
>>
>> We still want fixes for those ;-)
> 
> So, if my understanding is correct in new production systems
> using legacy gadget drivers is not recommended and we are not going to fix
> any more bugs there.

We can't really control who uses what. So we need to support legacy
drivers as well.

The issue detected by using g_zero is a controller driver issue and not an
issue with g_zero itself.

cheers,
-roger

> 
> The legacy gadget drivers remains in the kernel only to support the old
> products.
> 
> So because we introduce new driver we should not worry  about legacy
> gadget drivers.
> 
> Is my understanding correct ?
> 
> pawell
>   
>>
>>>> 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.
>>
>> -EAGAIN, IIRC is only supposed to be used for IN endpoint stalls.
>>
>> --
>> balbi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver.
  2019-10-09 13:50       ` Roger Quadros
@ 2019-10-10 10:17         ` Felipe Balbi
  0 siblings, 0 replies; 11+ messages in thread
From: Felipe Balbi @ 2019-10-10 10:17 UTC (permalink / raw)
  To: Roger Quadros, Pawel Laszczak, Peter Chen
  Cc: gregkh\, linux-usb\, linux-kernel\, jbergsagel\, nsekhar\, nm\,
	Suresh Punnoose, Rahul Kumar


hi,

Roger Quadros <rogerq@ti.com> writes:
>>> Peter Chen <peter.chen@nxp.com> writes:
>>>> 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.
>>>
>>> We still want fixes for those ;-)
>> 
>> So, if my understanding is correct in new production systems
>> using legacy gadget drivers is not recommended and we are not going to fix
>> any more bugs there.
>
> We can't really control who uses what. So we need to support legacy
> drivers as well.
>
> The issue detected by using g_zero is a controller driver issue and not an
> issue with g_zero itself.

thanks for clarifying, Roger.

Pawel, Roger is correct here. But in any case, if *real* bugs are found
on the legacy gadget drivers, let's see if they happen with
configfs-based interface as well. They should behave exactly the same
nowadays since legacy driver were converted to simply programmatically
spawn the configfs interface.

If there are bugs in legacy gadget drivers, we *WANT* to fix those.

-- 
balbi

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07  6:39 [PATCH] usb:cdns3: Fix for CV CH9 running with g_zero driver Pawel Laszczak
2019-10-07  8:40 ` Roger Quadros
2019-10-08  6:56 ` Peter Chen
2019-10-08  7:57   ` Pawel Laszczak
2019-10-08  8:44     ` Peter Chen
2019-10-08  9:43       ` Roger Quadros
2019-10-08 12:58   ` Felipe Balbi
2019-10-09  2:28     ` Peter Chen
2019-10-09  3:58     ` Pawel Laszczak
2019-10-09 13:50       ` Roger Quadros
2019-10-10 10:17         ` Felipe Balbi

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox