On 2/9/2016 12:06 PM, Felipe Balbi wrote:
>
> Hi John,
>
> John Youn <John.Youn@xxxxxxxxxxxx> 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: <stable@xxxxxxxxxxxxxxx> # 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 <ravibabu@xxxxxx>
>> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
>> ---
>> 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;
>> + unsigned resource_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 *dep;
>> + int idx;
>> +
>> + idx = (num << 1) | direction;
>> + dep = dwc->eps[idx];
>> + dep->resource_assigned = 0;
>> +}
>> +
>> +static void dwc3_gadget_reset_xfer_resources(struct dwc3 *dwc, bool do_ep0)
>> +{
>> + int i;
>> + int first_ep = do_ep0 ? 0 : 1;
>> +
>> + for (i = first_ep; i < dwc->num_out_eps; i++)
>> + dwc3_gadget_reset_xfer_resource_for_ep(dwc, i, 0);
>> +
>> + for (i = first_ep; i < dwc->num_in_eps; i++)
>> + dwc3_gadget_reset_xfer_resource_for_ep(dwc, i, 1);
>
> I don't quite like this. Every time dwc3_gadget_start_config() is
> called, we clear *ALL* endpoints. We need to find a better way for
> this. As it stands it's just pointless overhead.
>
> Consider a gadget which uses up ALL 32 physical endpoints. This loop
> will execute 32 * 32 = 1024 times. Now change configuration and that
> happens again ;-)
No that shouldn't be the case. I think the start_config() function
could be rewritten to make it clearer but it should be called only in
the following two conditions:
1) dep->number == 0
Called when ep0 is first enabled which only happens on startup.
2) dep->number > 1 (ie non ep0) &&
dwc->start_config_issued == false.
Only called after the first SET_CONFIGURATION and the gadget
framework enables the first non-ep0 endpoint. All subsequent
SET_CONFIG/SET_INTERFACES will enable/disable whatever endpoint
they are configured to use. And we will assign a transfer resource
to it if it doesn't have one already.
This is just following the programming model as described in the
databook but possibly some optimizations can be made. I am looking
into some possibilities currently.
>
> Seems like we just need some clarification on the resource allocation
> procedure then find a way to skip the clearing of new resources.
>
> I like your 'resource_assigned' flag (but move it to the bitfield above)
> and I bet that's all we need. Together with a counter to how many
> resources have been allocated thus far so that DWC3_DEPCMD_PARAM(2) can
> be changed to DWC3_DEPCMD_PARAM(dwc->current_resource) or something
> along these lines.
>
This is one potential solution but I think it's unnecessary with the
above programming model since the hardware keeps track of the
available resources and we just keep getting the next one whenever we
come across an endpoint that isn't assigned one.
I'll resend with your feedback shortly.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
