Re: [PATCH 10/15] usb: dwc3: gadget: Set maxpacket size for ep0 IN

[Felipe Balbi]

Hi,

Thinh Nguyen  writes:
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index bdf2a2014ebd..6ae924d95cbc 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2751,6 +2751,8 @@ static void dwc3_gadget_conndone_interrupt(struct 
> dwc3 *dwc)
>   break;
>   }
>
> + dwc->eps[1]->endpoint.maxpacket = dwc->gadget.ep0->maxpacket;

 thanks :-) I've had that on my list for a while but never got to it
 since it has no real effects other than reporting properly on
 tracepoints :-)

>>>
>>> Just to clarify, this is a bug. We found this issue when we test for
>>> HS/FS 3-stage control read transfer for ZLP.
>> 
>> interesting... I have never seen such bug before. Got some tracepoints
>> of the problem? Also, if it's a bug, why wasn't it sent as a separate
>> patch with a Cc stable and a Fixes tag?
>> 
>
> DWC3 gadget starts with max packet size (mps) 512 as default for ep0. 
> That value should be updated after ConnectDone event when DWC3 checks 
> for the connected speed. In HS/FS, the 3-stage control read transfer 
> sends ZLP based on the mps for IN ep0, but that value is not updated.
>
> Here's a quick test with the 3-stage HS control read.
>
> Setup:
> * mass_storage driver
> * Modify driver string description to 128 bytes
> * Enumerate device in highspeed (mps = 64)
>
> Below is the snippet of trace with GetDescriptor(STRING) request for the 
> driver string description. Device did not prepare and send ZLP packet.
>
>   41.787092: dwc3_ctrl_req: Get String Descriptor(Index = 4, Length = 255)
>   41.787093: dwc3_prepare_trb: ep0in: 0/0 trb 70a29ad1 buf 
> b8d67800 size 128 ctrl 0c53 (HLcs:SC:data)
>   41.787094: dwc3_readl: addr 8008901e value 2400
>   41.787095: dwc3_writel: addr 831c6d00 value 
>   41.787095: dwc3_writel: addr 3e4167b3 value 3782
>   41.787095: dwc3_writel: addr f504c84e value 
>   41.787095: dwc3_writel: addr ca003fac value 0406
>   41.787095: dwc3_readl: addr ca003fac value 0406
>   41.787098: dwc3_readl: addr ca003fac value 00010006
>   41.787100: dwc3_gadget_ep_cmd: ep0in: cmd 'Start Transfer' [1030] 
> params  3782  --> status: Successful
>   41.787101: dwc3_readl: addr ca003fac value 00010006
>   41.787103: dwc3_readl: addr 7bcb1278 value 80001000
>   41.787105: dwc3_writel: addr 7bcb1278 value 1000
>   41.787107: dwc3_readl: addr 8eea6144 value 
>   41.787216: dwc3_readl: addr 8eea6144 value 0008
>   41.787218: dwc3_readl: addr 7bcb1278 value 1000
>   41.787219: dwc3_writel: addr 7bcb1278 value 80001000
>   41.787220: dwc3_writel: addr 8eea6144 value 0008
>   41.787220: dwc3_event: event (c042): ep0in: Transfer Complete 
> [Data Phase]
>   41.787220: dwc3_complete_trb: ep0out: 0/1 trb 70a29ad1 buf 
> b8d67800 size 0 ctrl 0c52 (hLcs:SC:data)
>   41.787221: dwc3_complete_trb: ep0out: 0/1 trb cba79c85 buf 
>  size 0 ctrl  (hlcs:sc:UNKNOWN)
>   41.787221: dwc3_gadget_giveback: ep0out: req fd996780 length 
> 128/128 ZsI ==> 0
>   41.787221: dwc3_event: event (10c2): ep0in: Transfer Not Ready 
> (Not Active) [Data Phase]
>   41.787221: dwc3_readl: addr 7bcb1278 value 80001000
>   41.787223: dwc3_writel: addr 7bcb1278 value 1000
>   41.787223: dwc3_readl: addr 8eea6144 value 
>   46.294697: dwc3_readl: addr 8eea6144 value 0004
>   46.294698: dwc3_readl: addr 7bcb1278 value 1000
>   46.294699: dwc3_writel: addr 7bcb1278 value 80001000
>   46.294700: dwc3_writel: addr 8eea6144 value 0004
>
>
>
>
> Below is the snippet of the trace with the fix:
>
>   34.079018: dwc3_ctrl_req: Get String Descriptor(Index = 4, Length = 255)
>   34.079020: dwc3_prepare_trb: ep0in: 0/0 trb 6ece9b4d buf 
> b8d67800 size 128 ctrl 0455 (HlCs:Sc:data)
>   34.079020: dwc3_prepare_trb: ep0in: 0/0 trb 1b323efa buf 
> 37821000 size 0 ctrl 0c53 (HLcs:SC:data)
>   34.079020: dwc3_readl: addr a90978f8 value 2400
>   34.079021: dwc3_writel: addr c8f7ea9f value 
>   34.079021: dwc3_writel: addr 0217f085 value 3782
>   34.079021: dwc3_writel: addr b5593ec9 value 
>   34.079021: dwc3_writel: addr 8b9e5492 value 0406
>   34.079022: dwc3_readl: addr 8b9e5492 value 0406
>   34.079024: dwc3_readl: addr 8b9e5492 value 00010006
>   34.079027: dwc3_gadget_ep_cmd: ep0in: cmd 'Start Transfer' [1030] 
> params  3782  --> status: Successful
>   34.079027: dwc3_readl: addr 8b9e5492 value 00010006
>   34.079029: dwc3_readl: addr e9897a1c value 80001000
>   

Re: [PATCH 10/15] usb: dwc3: gadget: Set maxpacket size for ep0 IN

[Felipe Balbi]

Hi,

Thinh Nguyen  writes:
> Hi,
>
> On 1/8/2018 4:06 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Thinh Nguyen  writes:
>>> There are 2 control endpoint structures for DWC3. However, the driver
>>> only updates the OUT direction control endpoint structure during
>>> ConnectDone event. DWC3 driver needs to update the endpoint max packet
>>> size for control IN endpoint as well. If the max packet size is not
>>> properly set, then the driver will incorrectly calculate the data
>>> transfer size and fail to send ZLP for HS/FS 3-stage control read
>>> transfer.
>>>
>>> The fix is simply to update the max packet size for the ep0 IN direction
>>> during ConnectDone event.
>>>
>>> Signed-off-by: Thinh Nguyen 
>>> ---
>>>   drivers/usb/dwc3/gadget.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index bdf2a2014ebd..6ae924d95cbc 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2751,6 +2751,8 @@ static void dwc3_gadget_conndone_interrupt(struct 
>>> dwc3 *dwc)
>>> break;
>>> }
>>>   
>>> +   dwc->eps[1]->endpoint.maxpacket = dwc->gadget.ep0->maxpacket;
>> 
>> thanks :-) I've had that on my list for a while but never got to it
>> since it has no real effects other than reporting properly on
>> tracepoints :-)
>> 
>
> Just to clarify, this is a bug. We found this issue when we test for 
> HS/FS 3-stage control read transfer for ZLP.

interesting... I have never seen such bug before. Got some tracepoints
of the problem? Also, if it's a bug, why wasn't it sent as a separate
patch with a Cc stable and a Fixes tag?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 10/15] usb: dwc3: gadget: Set maxpacket size for ep0 IN

[Thinh Nguyen]

Hi,

On 1/8/2018 4:06 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen  writes:
>> There are 2 control endpoint structures for DWC3. However, the driver
>> only updates the OUT direction control endpoint structure during
>> ConnectDone event. DWC3 driver needs to update the endpoint max packet
>> size for control IN endpoint as well. If the max packet size is not
>> properly set, then the driver will incorrectly calculate the data
>> transfer size and fail to send ZLP for HS/FS 3-stage control read
>> transfer.
>>
>> The fix is simply to update the max packet size for the ep0 IN direction
>> during ConnectDone event.
>>
>> Signed-off-by: Thinh Nguyen 
>> ---
>>   drivers/usb/dwc3/gadget.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index bdf2a2014ebd..6ae924d95cbc 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2751,6 +2751,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 
>> *dwc)
>>  break;
>>  }
>>   
>> +dwc->eps[1]->endpoint.maxpacket = dwc->gadget.ep0->maxpacket;
> 
> thanks :-) I've had that on my list for a while but never got to it
> since it has no real effects other than reporting properly on
> tracepoints :-)
> 

Just to clarify, this is a bug. We found this issue when we test for 
HS/FS 3-stage control read transfer for ZLP.

BR,
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/15] usb: dwc3: gadget: Set maxpacket size for ep0 IN

[Felipe Balbi]

Hi,

Thinh Nguyen  writes:
> There are 2 control endpoint structures for DWC3. However, the driver
> only updates the OUT direction control endpoint structure during
> ConnectDone event. DWC3 driver needs to update the endpoint max packet
> size for control IN endpoint as well. If the max packet size is not
> properly set, then the driver will incorrectly calculate the data
> transfer size and fail to send ZLP for HS/FS 3-stage control read
> transfer.
>
> The fix is simply to update the max packet size for the ep0 IN direction
> during ConnectDone event.
>
> Signed-off-by: Thinh Nguyen 
> ---
>  drivers/usb/dwc3/gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index bdf2a2014ebd..6ae924d95cbc 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2751,6 +2751,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 
> *dwc)
>   break;
>   }
>  
> + dwc->eps[1]->endpoint.maxpacket = dwc->gadget.ep0->maxpacket;

thanks :-) I've had that on my list for a while but never got to it
since it has no real effects other than reporting properly on
tracepoints :-)

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 10/15] usb: dwc3: gadget: Set maxpacket size for ep0 IN

[Thinh Nguyen]

There are 2 control endpoint structures for DWC3. However, the driver
only updates the OUT direction control endpoint structure during
ConnectDone event. DWC3 driver needs to update the endpoint max packet
size for control IN endpoint as well. If the max packet size is not
properly set, then the driver will incorrectly calculate the data
transfer size and fail to send ZLP for HS/FS 3-stage control read
transfer.

The fix is simply to update the max packet size for the ep0 IN direction
during ConnectDone event.

Signed-off-by: Thinh Nguyen 
---
 drivers/usb/dwc3/gadget.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index bdf2a2014ebd..6ae924d95cbc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2751,6 +2751,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 
*dwc)
break;
}
 
+   dwc->eps[1]->endpoint.maxpacket = dwc->gadget.ep0->maxpacket;
+
/* Enable USB2 LPM Capability */
 
if ((dwc->revision > DWC3_REVISION_194A) &&
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html