Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[Felipe Balbi]

Hi,

John Youn  writes:
> On 2/12/2016 2:05 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> John Youn  writes:
>>> On 2/10/2016 1:07 PM, Felipe Balbi wrote:
 John Youn  writes:
>>> Basically assign all the resources in advance.
>>
>> I thought about that, but wouldn't this, essentially, enable all
>> endpoints unconditionally ? This could, potentially, increase power
>> consumption on some systems, right ? This could also cause "spurious"
>> interrupts if a bogus host tries to move data on an endpoint which
>> hasn't been enabled yet.
>
> No, I mean to just assign resources withouth configuring or enabling
> the endpoint. I have tested this approach and it works. But I still

 oh ok. 

> need to verify that it won't conflict with anything, such as streams.

 yeah, we would probably have an issue with streams. IIRC, we allocate
 one transfer resource per stream, right ?
>>>
>>> Ends up that is not a concern. Streams always use a single resource
>>> per endpoint, not stream.
>> 
>> hey, that's great. So what's the idea ? static resource assignment on
>> endpoint initialization ?
>> 
>
> Yes that's it. I will go ahead and submit this fix. See the commit
> message for details. I verified with engineers and did a round of
> testing and so far no problems.
>
> If you prefer to only assign resources as needed, I have a separate
> fix that I can submit if you want.
>
> Also, I think we need to handle backporting separately as neither
> patch applies cleanly to 4.3.

when backporting, we will receive emails that Greg couldn't apply. All
we have to do is reply with the proper backport ;-)

--
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[John Youn]

On 2/12/2016 2:05 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> On 2/10/2016 1:07 PM, Felipe Balbi wrote:
>>> John Youn  writes:
>> Basically assign all the resources in advance.
>
> I thought about that, but wouldn't this, essentially, enable all
> endpoints unconditionally ? This could, potentially, increase power
> consumption on some systems, right ? This could also cause "spurious"
> interrupts if a bogus host tries to move data on an endpoint which
> hasn't been enabled yet.

 No, I mean to just assign resources withouth configuring or enabling
 the endpoint. I have tested this approach and it works. But I still
>>>
>>> oh ok. 
>>>
 need to verify that it won't conflict with anything, such as streams.
>>>
>>> yeah, we would probably have an issue with streams. IIRC, we allocate
>>> one transfer resource per stream, right ?
>>
>> Ends up that is not a concern. Streams always use a single resource
>> per endpoint, not stream.
> 
> hey, that's great. So what's the idea ? static resource assignment on
> endpoint initialization ?
> 

Yes that's it. I will go ahead and submit this fix. See the commit
message for details. I verified with engineers and did a round of
testing and so far no problems.

If you prefer to only assign resources as needed, I have a separate
fix that I can submit if you want.

Also, I think we need to handle backporting separately as neither
patch applies cleanly to 4.3.

Regards,
John
--
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] usb: dwc3: Fix assignment of EP transfer resources

[Felipe Balbi]

Hi,

John Youn  writes:
> On 2/10/2016 1:07 PM, Felipe Balbi wrote:
>> John Youn  writes:
> Basically assign all the resources in advance.

 I thought about that, but wouldn't this, essentially, enable all
 endpoints unconditionally ? This could, potentially, increase power
 consumption on some systems, right ? This could also cause "spurious"
 interrupts if a bogus host tries to move data on an endpoint which
 hasn't been enabled yet.
>>>
>>> No, I mean to just assign resources withouth configuring or enabling
>>> the endpoint. I have tested this approach and it works. But I still
>> 
>> oh ok. 
>> 
>>> need to verify that it won't conflict with anything, such as streams.
>> 
>> yeah, we would probably have an issue with streams. IIRC, we allocate
>> one transfer resource per stream, right ?
>
> Ends up that is not a concern. Streams always use a single resource
> per endpoint, not stream.

hey, that's great. So what's the idea ? static resource assignment on
endpoint initialization ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[John Youn]

On 2/10/2016 1:07 PM, Felipe Balbi wrote:
> John Youn  writes:
 Basically assign all the resources in advance.
>>>
>>> I thought about that, but wouldn't this, essentially, enable all
>>> endpoints unconditionally ? This could, potentially, increase power
>>> consumption on some systems, right ? This could also cause "spurious"
>>> interrupts if a bogus host tries to move data on an endpoint which
>>> hasn't been enabled yet.
>>
>> No, I mean to just assign resources withouth configuring or enabling
>> the endpoint. I have tested this approach and it works. But I still
> 
> oh ok. 
> 
>> need to verify that it won't conflict with anything, such as streams.
> 
> yeah, we would probably have an issue with streams. IIRC, we allocate
> one transfer resource per stream, right ?

Ends up that is not a concern. Streams always use a single resource
per endpoint, not stream.

John
--
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] usb: dwc3: Fix assignment of EP transfer resources

[John Youn]

On 2/10/2016 2:11 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
> 
> 
>>> If it wasn't for that flag, we would always allocate transfer resource 3
>>> for every newly enabled endpoint. Can you check with your RTL engineers
>>> if it's valid to *always* issue DEPSTARTCFG with a proper parameter
>>> depending on endpoint number ? Basically, something like below:
>>>
>>> @@ -306,20 +306,14 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc, 
>>> struct dwc3_ep *dep)
>>>  
>>> memset(, 0x00, sizeof(params));
>>>  
>>> -   if (dep->number != 1) {
>>> -   cmd = DWC3_DEPCMD_DEPSTARTCFG;
>>> -   /* XferRscIdx == 0 for ep0 and 2 for the remaining */
>>> -   if (dep->number > 1) {
>>> -   if (dwc->start_config_issued)
>>> -   return 0;
>>> -   dwc->start_config_issued = true;
>>> -   cmd |= DWC3_DEPCMD_PARAM(2);
>>> -   }
>>> -
>>> -   return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, );
>>> +   cmd = DWC3_DEPCMD_DEPSTARTCFG;
>>> +   /* XferRscIdx == 0 for ep0 and 2 for the remaining */
>>> +   if (dep->number > 1) {
>>> +   dwc->start_config_issued = true;
>>> +   cmd |= DWC3_DEPCMD_PARAM(dwc->current_resource);
>>> }
>>>  
>>> -   return 0;
>>> +   return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, );
>>>  }
>>>  
>>>  static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep,
>>> @@ -388,13 +382,20 @@ static int dwc3_gadget_set_ep_config(struct dwc3 
>>> *dwc, struct dwc3_ep *dep,
>>>  static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep 
>>> *dep)
>>>  {
>>> struct dwc3_gadget_ep_cmd_params params;
>>> +   int ret;
>>>  
>>> memset(, 0x00, sizeof(params));
>>>  
>>> params.param0 = DWC3_DEPXFERCFG_NUM_XFER_RES(1);
>>>  
>>> -   return dwc3_send_gadget_ep_cmd(dwc, dep->number,
>>> +   ret = dwc3_send_gadget_ep_cmd(dwc, dep->number,
>>> DWC3_DEPCMD_SETTRANSFRESOURCE, );
>>> +   if (ret)
>>> +   return ret;
>>> +
>>> +   dwc->current_resource++;
>>> +
>>> +   return 0;
>>>  }
>>>  
>>>  /**
>>>
>>> With this we will *always* use DEPSTARTCFG any time we're enabling an
>>> endpoint which hasn't been enabled, but we will always keep transfer
>>> resources around. (Note that this won't really work as is, I haven't
>>> defined current_resource nor have I made sure to decrement
>>> current_resource whenever we disable an endpoint. Also, it might be that
>>> using a 32-bit flag instead of a counter might be better, dunno)
>>>
>>
>> Something like this might work too.
>>
>> I actually have a patch now which *greatly* simplifies all of this
>> code and eliminates the start_config_issued flag. But I need the RTL
>> engineers to confirm. It should be ok as it relies on the same
>> behavior that this current patch does.
>>
>> Basically assign all the resources in advance.
> 
> I thought about that, but wouldn't this, essentially, enable all
> endpoints unconditionally ? This could, potentially, increase power
> consumption on some systems, right ? This could also cause "spurious"
> interrupts if a bogus host tries to move data on an endpoint which
> hasn't been enabled yet.

No, I mean to just assign resources withouth configuring or enabling
the endpoint. I have tested this approach and it works. But I still
need to verify that it won't conflict with anything, such as streams.

> 
> Not sure this is a good approach here.
> 

In any case, I will also resend the cleaned up version of this patch
as well.

Regards,
John
--
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] usb: dwc3: Fix assignment of EP transfer resources

[Felipe Balbi]

John Youn  writes:
>>> Basically assign all the resources in advance.
>> 
>> I thought about that, but wouldn't this, essentially, enable all
>> endpoints unconditionally ? This could, potentially, increase power
>> consumption on some systems, right ? This could also cause "spurious"
>> interrupts if a bogus host tries to move data on an endpoint which
>> hasn't been enabled yet.
>
> No, I mean to just assign resources withouth configuring or enabling
> the endpoint. I have tested this approach and it works. But I still

oh ok. 

> need to verify that it won't conflict with anything, such as streams.

yeah, we would probably have an issue with streams. IIRC, we allocate
one transfer resource per stream, right ?

>> Not sure this is a good approach here.
>
> In any case, I will also resend the cleaned up version of this patch
> as well.

cool

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[John Youn]

On 2/10/2016 12:44 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>>> John Youn  writes:
 The assignement of EP transfer resources was not handled properly in the
 dwc3 driver. Commit aebda6187181 ("usb: dwc3: Reset the transfer
 resource index on SET_INTERFACE") previously fixed one aspect of this
 where resources may be exhausted with multiple calls to SET_INTERFACE.
 However, it introduced an issue where composite devices with multiple
 interfaces can be assigned the same transfer resources for different
 endpoints.

 This patch solves both issues.

 The assigning of transfer resource should go as follows:

 Each hardware endpoint requires a transfer resource before it can
 perform any USB transfer. The transfer resources are allocated using two
 commands: DEPSTARTCFG and DEPXFERCFG.

 In the controller, there is a transfer resource index that is set by the
 DEPSTARTCFG command. The DEPXFERCFG command assigns the resource to an
 endpoint and increments the transfer resource index.

 Upon startup of the driver, the transfer resource index should be set to
 0 by issuing DEPSTARTCFG(0). EP0-out and EP0-in are then assigned a
 resource by DEPXFERCFG. They are assigned resource indexes 0 and 1.

 Every time a SET_CONFIGURATION usb request occurs the DEPSTARTCFG(2)
 command should be issued to reset the index to 2. Transfer resources 0
 and 1 remain assigned to EP0-out and EP0-in.

 Then for every endpoint in the configuration (endpoints that will be
 enabled by the upper layer) call DEPXFERCFG to assign the next
 resource. On SET_INTERFACE, the same or different endpoints may be
 enabled. If the endpoint already has an assigned transfer resource,
 don't assign a new one.
>>>
>>> very nice and thorough commit log, thanks.
>>>
 Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on 
 SET_INTERFACE")
>>>
>>> You need to Cc stable here:
>>>
>>> Cc:  # v3.2+
>>>
>>> The reason for v3.2+, instead of v4.3+ is that usb: dwc3: Reset the
>>> transfer resource index on SET_INTERFACE has been backported to v3.2+,
>>> so we want to fix all those kernels too.
>>>
 Reported-by: Ravi Babu 
 Signed-off-by: John Youn 
 ---
 Hi Ravi,

 Here is a patch that should solve your issue. Can you review and test
 it out?

 I have tested with multiple SET_INTERFACE for a single interface.

 Thanks,
 John



  drivers/usb/dwc3/core.h   |  3 +++
  drivers/usb/dwc3/ep0.c|  4 
  drivers/usb/dwc3/gadget.c | 36 +---
  3 files changed, 36 insertions(+), 7 deletions(-)

 diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
 index 2913068..7d5d3a2 100644
 --- a/drivers/usb/dwc3/core.h
 +++ b/drivers/usb/dwc3/core.h
 @@ -453,6 +453,8 @@ struct dwc3_event_buffer {
   * @flags: endpoint flags (wedged, stalled, ...)
   * @number: endpoint number (1 - 15)
   * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
 + * @resource_assigned: indicates that a transfer resource is assigned
 + *to this endpoint
   * @resource_index: Resource transfer index
   * @interval: the interval on which the ISOC transfer is started
   * @name: a human readable name e.g. ep1out-bulk
 @@ -485,6 +487,7 @@ struct dwc3_ep {
  
u8  number;
u8  type;
 +  unsignedresource_assigned:1;
>>>
>>> I would prefer to use up another bit from our 'flags' bitfield. Looks
>>> like that would be:
>>>
>>> #define DWC3_EP_RESOURCE_ASSIGNED (1 << 7)
>>>
 diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
 index 3a9354a..878b40e 100644
 --- a/drivers/usb/dwc3/ep0.c
 +++ b/drivers/usb/dwc3/ep0.c
 @@ -737,10 +737,6 @@ static int dwc3_ep0_std_request(struct dwc3 *dwc, 
 struct usb_ctrlrequest *ctrl)
dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_ISOCH_DELAY");
ret = dwc3_ep0_set_isoch_delay(dwc, ctrl);
break;
 -  case USB_REQ_SET_INTERFACE:
 -  dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_INTERFACE");
 -  dwc->start_config_issued = false;
 -  /* Fall through */
default:
dwc3_trace(trace_dwc3_ep0, "Forwarding to gadget driver");
ret = dwc3_ep0_delegate_req(dwc, ctrl);
 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 7d1dd82..1aeea8f 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -385,6 +385,30 @@ static void dwc3_free_trb_pool(struct dwc3_ep *dep)
dep->trb_pool_dma = 0;
  }
  
 +static void 

Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[Felipe Balbi]

Hi,

John Youn  writes:


>> If it wasn't for that flag, we would always allocate transfer resource 3
>> for every newly enabled endpoint. Can you check with your RTL engineers
>> if it's valid to *always* issue DEPSTARTCFG with a proper parameter
>> depending on endpoint number ? Basically, something like below:
>> 
>> @@ -306,20 +306,14 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc, 
>> struct dwc3_ep *dep)
>>  
>>  memset(, 0x00, sizeof(params));
>>  
>> -if (dep->number != 1) {
>> -cmd = DWC3_DEPCMD_DEPSTARTCFG;
>> -/* XferRscIdx == 0 for ep0 and 2 for the remaining */
>> -if (dep->number > 1) {
>> -if (dwc->start_config_issued)
>> -return 0;
>> -dwc->start_config_issued = true;
>> -cmd |= DWC3_DEPCMD_PARAM(2);
>> -}
>> -
>> -return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, );
>> +cmd = DWC3_DEPCMD_DEPSTARTCFG;
>> +/* XferRscIdx == 0 for ep0 and 2 for the remaining */
>> +if (dep->number > 1) {
>> +dwc->start_config_issued = true;
>> +cmd |= DWC3_DEPCMD_PARAM(dwc->current_resource);
>>  }
>>  
>> -return 0;
>> +return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, );
>>  }
>>  
>>  static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep,
>> @@ -388,13 +382,20 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, 
>> struct dwc3_ep *dep,
>>  static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep 
>> *dep)
>>  {
>>  struct dwc3_gadget_ep_cmd_params params;
>> +int ret;
>>  
>>  memset(, 0x00, sizeof(params));
>>  
>>  params.param0 = DWC3_DEPXFERCFG_NUM_XFER_RES(1);
>>  
>> -return dwc3_send_gadget_ep_cmd(dwc, dep->number,
>> +ret = dwc3_send_gadget_ep_cmd(dwc, dep->number,
>>  DWC3_DEPCMD_SETTRANSFRESOURCE, );
>> +if (ret)
>> +return ret;
>> +
>> +dwc->current_resource++;
>> +
>> +return 0;
>>  }
>>  
>>  /**
>> 
>> With this we will *always* use DEPSTARTCFG any time we're enabling an
>> endpoint which hasn't been enabled, but we will always keep transfer
>> resources around. (Note that this won't really work as is, I haven't
>> defined current_resource nor have I made sure to decrement
>> current_resource whenever we disable an endpoint. Also, it might be that
>> using a 32-bit flag instead of a counter might be better, dunno)
>> 
>
> Something like this might work too.
>
> I actually have a patch now which *greatly* simplifies all of this
> code and eliminates the start_config_issued flag. But I need the RTL
> engineers to confirm. It should be ok as it relies on the same
> behavior that this current patch does.
>
> Basically assign all the resources in advance.

I thought about that, but wouldn't this, essentially, enable all
endpoints unconditionally ? This could, potentially, increase power
consumption on some systems, right ? This could also cause "spurious"
interrupts if a bogus host tries to move data on an endpoint which
hasn't been enabled yet.

Not sure this is a good approach here.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[Felipe Balbi]

Hi,

John Youn  writes:
>> John Youn  writes:
>>> The assignement of EP transfer resources was not handled properly in the
>>> dwc3 driver. Commit aebda6187181 ("usb: dwc3: Reset the transfer
>>> resource index on SET_INTERFACE") previously fixed one aspect of this
>>> where resources may be exhausted with multiple calls to SET_INTERFACE.
>>> However, it introduced an issue where composite devices with multiple
>>> interfaces can be assigned the same transfer resources for different
>>> endpoints.
>>>
>>> This patch solves both issues.
>>>
>>> The assigning of transfer resource should go as follows:
>>>
>>> Each hardware endpoint requires a transfer resource before it can
>>> perform any USB transfer. The transfer resources are allocated using two
>>> commands: DEPSTARTCFG and DEPXFERCFG.
>>>
>>> In the controller, there is a transfer resource index that is set by the
>>> DEPSTARTCFG command. The DEPXFERCFG command assigns the resource to an
>>> endpoint and increments the transfer resource index.
>>>
>>> Upon startup of the driver, the transfer resource index should be set to
>>> 0 by issuing DEPSTARTCFG(0). EP0-out and EP0-in are then assigned a
>>> resource by DEPXFERCFG. They are assigned resource indexes 0 and 1.
>>>
>>> Every time a SET_CONFIGURATION usb request occurs the DEPSTARTCFG(2)
>>> command should be issued to reset the index to 2. Transfer resources 0
>>> and 1 remain assigned to EP0-out and EP0-in.
>>>
>>> Then for every endpoint in the configuration (endpoints that will be
>>> enabled by the upper layer) call DEPXFERCFG to assign the next
>>> resource. On SET_INTERFACE, the same or different endpoints may be
>>> enabled. If the endpoint already has an assigned transfer resource,
>>> don't assign a new one.
>> 
>> very nice and thorough commit log, thanks.
>> 
>>> Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on 
>>> SET_INTERFACE")
>> 
>> You need to Cc stable here:
>> 
>> Cc:  # v3.2+
>> 
>> The reason for v3.2+, instead of v4.3+ is that usb: dwc3: Reset the
>> transfer resource index on SET_INTERFACE has been backported to v3.2+,
>> so we want to fix all those kernels too.
>> 
>>> Reported-by: Ravi Babu 
>>> Signed-off-by: John Youn 
>>> ---
>>> Hi Ravi,
>>>
>>> Here is a patch that should solve your issue. Can you review and test
>>> it out?
>>>
>>> I have tested with multiple SET_INTERFACE for a single interface.
>>>
>>> Thanks,
>>> John
>>>
>>>
>>>
>>>  drivers/usb/dwc3/core.h   |  3 +++
>>>  drivers/usb/dwc3/ep0.c|  4 
>>>  drivers/usb/dwc3/gadget.c | 36 +---
>>>  3 files changed, 36 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 2913068..7d5d3a2 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -453,6 +453,8 @@ struct dwc3_event_buffer {
>>>   * @flags: endpoint flags (wedged, stalled, ...)
>>>   * @number: endpoint number (1 - 15)
>>>   * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
>>> + * @resource_assigned: indicates that a transfer resource is assigned
>>> + * to this endpoint
>>>   * @resource_index: Resource transfer index
>>>   * @interval: the interval on which the ISOC transfer is started
>>>   * @name: a human readable name e.g. ep1out-bulk
>>> @@ -485,6 +487,7 @@ struct dwc3_ep {
>>>  
>>> u8  number;
>>> u8  type;
>>> +   unsignedresource_assigned:1;
>> 
>> I would prefer to use up another bit from our 'flags' bitfield. Looks
>> like that would be:
>> 
>> #define DWC3_EP_RESOURCE_ASSIGNED (1 << 7)
>> 
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 3a9354a..878b40e 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -737,10 +737,6 @@ static int dwc3_ep0_std_request(struct dwc3 *dwc, 
>>> struct usb_ctrlrequest *ctrl)
>>> dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_ISOCH_DELAY");
>>> ret = dwc3_ep0_set_isoch_delay(dwc, ctrl);
>>> break;
>>> -   case USB_REQ_SET_INTERFACE:
>>> -   dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_INTERFACE");
>>> -   dwc->start_config_issued = false;
>>> -   /* Fall through */
>>> default:
>>> dwc3_trace(trace_dwc3_ep0, "Forwarding to gadget driver");
>>> ret = dwc3_ep0_delegate_req(dwc, ctrl);
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 7d1dd82..1aeea8f 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -385,6 +385,30 @@ static void dwc3_free_trb_pool(struct dwc3_ep *dep)
>>> dep->trb_pool_dma = 0;
>>>  }
>>>  
>>> +static void dwc3_gadget_reset_xfer_resource_for_ep(struct dwc3 *dwc,
>> 
>> it would be nicer if this would receive struct dwc3_ep *dep as argument.
>> 
>>> +  

Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[Felipe Balbi]

Hi John,

John Youn  writes:
> The assignement of EP transfer resources was not handled properly in the
> dwc3 driver. Commit aebda6187181 ("usb: dwc3: Reset the transfer
> resource index on SET_INTERFACE") previously fixed one aspect of this
> where resources may be exhausted with multiple calls to SET_INTERFACE.
> However, it introduced an issue where composite devices with multiple
> interfaces can be assigned the same transfer resources for different
> endpoints.
>
> This patch solves both issues.
>
> The assigning of transfer resource should go as follows:
>
> Each hardware endpoint requires a transfer resource before it can
> perform any USB transfer. The transfer resources are allocated using two
> commands: DEPSTARTCFG and DEPXFERCFG.
>
> In the controller, there is a transfer resource index that is set by the
> DEPSTARTCFG command. The DEPXFERCFG command assigns the resource to an
> endpoint and increments the transfer resource index.
>
> Upon startup of the driver, the transfer resource index should be set to
> 0 by issuing DEPSTARTCFG(0). EP0-out and EP0-in are then assigned a
> resource by DEPXFERCFG. They are assigned resource indexes 0 and 1.
>
> Every time a SET_CONFIGURATION usb request occurs the DEPSTARTCFG(2)
> command should be issued to reset the index to 2. Transfer resources 0
> and 1 remain assigned to EP0-out and EP0-in.
>
> Then for every endpoint in the configuration (endpoints that will be
> enabled by the upper layer) call DEPXFERCFG to assign the next
> resource. On SET_INTERFACE, the same or different endpoints may be
> enabled. If the endpoint already has an assigned transfer resource,
> don't assign a new one.

very nice and thorough commit log, thanks.

> Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on 
> SET_INTERFACE")

You need to Cc stable here:

Cc:  # v3.2+

The reason for v3.2+, instead of v4.3+ is that usb: dwc3: Reset the
transfer resource index on SET_INTERFACE has been backported to v3.2+,
so we want to fix all those kernels too.

> Reported-by: Ravi Babu 
> Signed-off-by: John Youn 
> ---
> Hi Ravi,
>
> Here is a patch that should solve your issue. Can you review and test
> it out?
>
> I have tested with multiple SET_INTERFACE for a single interface.
>
> Thanks,
> John
>
>
>
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/ep0.c|  4 
>  drivers/usb/dwc3/gadget.c | 36 +---
>  3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2913068..7d5d3a2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -453,6 +453,8 @@ struct dwc3_event_buffer {
>   * @flags: endpoint flags (wedged, stalled, ...)
>   * @number: endpoint number (1 - 15)
>   * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
> + * @resource_assigned: indicates that a transfer resource is assigned
> + *   to this endpoint
>   * @resource_index: Resource transfer index
>   * @interval: the interval on which the ISOC transfer is started
>   * @name: a human readable name e.g. ep1out-bulk
> @@ -485,6 +487,7 @@ struct dwc3_ep {
>  
>   u8  number;
>   u8  type;
> + unsignedresource_assigned:1;

I would prefer to use up another bit from our 'flags' bitfield. Looks
like that would be:

#define DWC3_EP_RESOURCE_ASSIGNED (1 << 7)

> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 3a9354a..878b40e 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -737,10 +737,6 @@ static int dwc3_ep0_std_request(struct dwc3 *dwc, struct 
> usb_ctrlrequest *ctrl)
>   dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_ISOCH_DELAY");
>   ret = dwc3_ep0_set_isoch_delay(dwc, ctrl);
>   break;
> - case USB_REQ_SET_INTERFACE:
> - dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_INTERFACE");
> - dwc->start_config_issued = false;
> - /* Fall through */
>   default:
>   dwc3_trace(trace_dwc3_ep0, "Forwarding to gadget driver");
>   ret = dwc3_ep0_delegate_req(dwc, ctrl);
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 7d1dd82..1aeea8f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -385,6 +385,30 @@ static void dwc3_free_trb_pool(struct dwc3_ep *dep)
>   dep->trb_pool_dma = 0;
>  }
>  
> +static void dwc3_gadget_reset_xfer_resource_for_ep(struct dwc3 *dwc,

it would be nicer if this would receive struct dwc3_ep *dep as argument.

> +int num,
> +int direction)

this is an odd indentation, care to keep up with what's already being
used ? (hint: just add two tabs after breaking the line, no spaces at all)

> +{
> + struct dwc3_ep 

Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[John Youn]

On 2/9/2016 12:06 PM, Felipe Balbi wrote:
> 
> Hi John,
> 
> John Youn  writes:
>> The assignement of EP transfer resources was not handled properly in the
>> dwc3 driver. Commit aebda6187181 ("usb: dwc3: Reset the transfer
>> resource index on SET_INTERFACE") previously fixed one aspect of this
>> where resources may be exhausted with multiple calls to SET_INTERFACE.
>> However, it introduced an issue where composite devices with multiple
>> interfaces can be assigned the same transfer resources for different
>> endpoints.
>>
>> This patch solves both issues.
>>
>> The assigning of transfer resource should go as follows:
>>
>> Each hardware endpoint requires a transfer resource before it can
>> perform any USB transfer. The transfer resources are allocated using two
>> commands: DEPSTARTCFG and DEPXFERCFG.
>>
>> In the controller, there is a transfer resource index that is set by the
>> DEPSTARTCFG command. The DEPXFERCFG command assigns the resource to an
>> endpoint and increments the transfer resource index.
>>
>> Upon startup of the driver, the transfer resource index should be set to
>> 0 by issuing DEPSTARTCFG(0). EP0-out and EP0-in are then assigned a
>> resource by DEPXFERCFG. They are assigned resource indexes 0 and 1.
>>
>> Every time a SET_CONFIGURATION usb request occurs the DEPSTARTCFG(2)
>> command should be issued to reset the index to 2. Transfer resources 0
>> and 1 remain assigned to EP0-out and EP0-in.
>>
>> Then for every endpoint in the configuration (endpoints that will be
>> enabled by the upper layer) call DEPXFERCFG to assign the next
>> resource. On SET_INTERFACE, the same or different endpoints may be
>> enabled. If the endpoint already has an assigned transfer resource,
>> don't assign a new one.
> 
> very nice and thorough commit log, thanks.
> 
>> Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on 
>> SET_INTERFACE")
> 
> You need to Cc stable here:
> 
> Cc:  # v3.2+
> 
> The reason for v3.2+, instead of v4.3+ is that usb: dwc3: Reset the
> transfer resource index on SET_INTERFACE has been backported to v3.2+,
> so we want to fix all those kernels too.
> 
>> Reported-by: Ravi Babu 
>> Signed-off-by: John Youn 
>> ---
>> Hi Ravi,
>>
>> Here is a patch that should solve your issue. Can you review and test
>> it out?
>>
>> I have tested with multiple SET_INTERFACE for a single interface.
>>
>> Thanks,
>> John
>>
>>
>>
>>  drivers/usb/dwc3/core.h   |  3 +++
>>  drivers/usb/dwc3/ep0.c|  4 
>>  drivers/usb/dwc3/gadget.c | 36 +---
>>  3 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 2913068..7d5d3a2 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -453,6 +453,8 @@ struct dwc3_event_buffer {
>>   * @flags: endpoint flags (wedged, stalled, ...)
>>   * @number: endpoint number (1 - 15)
>>   * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
>> + * @resource_assigned: indicates that a transfer resource is assigned
>> + *  to this endpoint
>>   * @resource_index: Resource transfer index
>>   * @interval: the interval on which the ISOC transfer is started
>>   * @name: a human readable name e.g. ep1out-bulk
>> @@ -485,6 +487,7 @@ struct dwc3_ep {
>>  
>>  u8  number;
>>  u8  type;
>> +unsignedresource_assigned:1;
> 
> I would prefer to use up another bit from our 'flags' bitfield. Looks
> like that would be:
> 
> #define DWC3_EP_RESOURCE_ASSIGNED (1 << 7)
> 
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 3a9354a..878b40e 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -737,10 +737,6 @@ static int dwc3_ep0_std_request(struct dwc3 *dwc, 
>> struct usb_ctrlrequest *ctrl)
>>  dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_ISOCH_DELAY");
>>  ret = dwc3_ep0_set_isoch_delay(dwc, ctrl);
>>  break;
>> -case USB_REQ_SET_INTERFACE:
>> -dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_INTERFACE");
>> -dwc->start_config_issued = false;
>> -/* Fall through */
>>  default:
>>  dwc3_trace(trace_dwc3_ep0, "Forwarding to gadget driver");
>>  ret = dwc3_ep0_delegate_req(dwc, ctrl);
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 7d1dd82..1aeea8f 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -385,6 +385,30 @@ static void dwc3_free_trb_pool(struct dwc3_ep *dep)
>>  dep->trb_pool_dma = 0;
>>  }
>>  
>> +static void dwc3_gadget_reset_xfer_resource_for_ep(struct dwc3 *dwc,
> 
> it would be nicer if this would receive struct dwc3_ep *dep as argument.
> 
>> +   int num,
>> +   int direction)

RE: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[B, Ravi]

Hi John

>-Original Message-
>From: John Youn [mailto:john.y...@synopsys.com] 
>Sent: Wednesday, February 10, 2016 1:25 AM
>To: Felipe Balbi; B, Ravi
>Cc: john.y...@synopsys.com; linux-usb@vger.kernel.org
>Subject: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

>The assignement of EP transfer resources was not handled properly in the
>dwc3 driver. Commit aebda6187181 ("usb: dwc3: Reset the transfer
>resource index on SET_INTERFACE") previously fixed one aspect of this
>where resources may be exhausted with multiple calls to SET_INTERFACE.
>However, it introduced an issue where composite devices with multiple
>interfaces can be assigned the same transfer resources for different
>endpoints.

>This patch solves both issues.

>The assigning of transfer resource should go as follows:

>Each hardware endpoint requires a transfer resource before it can
>perform any USB transfer. The transfer resources are allocated using two
>commands: DEPSTARTCFG and DEPXFERCFG.

>In the controller, there is a transfer resource index that is set by the
>DEPSTARTCFG command. The DEPXFERCFG command assigns the resource to an
>endpoint and increments the transfer resource index.

>Upon startup of the driver, the transfer resource index should be set to
>0 by issuing DEPSTARTCFG(0). EP0-out and EP0-in are then assigned a
>resource by DEPXFERCFG. They are assigned resource indexes 0 and 1.

>Every time a SET_CONFIGURATION usb request occurs the DEPSTARTCFG(2)
>command should be issued to reset the index to 2. Transfer resources 0
>and 1 remain assigned to EP0-out and EP0-in.

>Then for every endpoint in the configuration (endpoints that will be
>enabled by the upper layer) call DEPXFERCFG to assign the next
>resource. On SET_INTERFACE, the same or different endpoints may be
>enabled. If the endpoint already has an assigned transfer resource,
>don't assign a new one.

>Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on 
>SET_INTERFACE")
>Reported-by: Ravi Babu 
>Signed-off-by: John Youn 
>---
>Hi Ravi,

>Here is a patch that should solve your issue. Can you review and test
>it out?

>I have tested with multiple SET_INTERFACE for a single interface.

I have run the same test case with composite gadget with two 
interface(NCM+ACM), the resource conflict is not seen.
It resolve the issue. Thanks for the patch.

Regards
Ravi 

 >drivers/usb/dwc3/core.h   |  3 +++
 >drivers/usb/dwc3/ep0.c|  4 
 >drivers/usb/dwc3/gadget.c | 36 +---
 >3 files changed, 36 insertions(+), 7 deletions(-)


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