On 2/10/2016 12:44 AM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>> 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.
>
> good point :-) (no databook around to re-check how things should work
> :-s Another week and I should be back to work)
>
>> 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.
>
> okay, cool.
>
>>> 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.
>
> It's odd, IMO, that we always reset transfer resources whenever we
> enable an endpoint. The only thing that prevents us from falling into
> situations described by $SUBJECT is our start_config_issued flag, right?
>
> 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(¶ms, 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, ¶ms);
> + 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, ¶ms);
> }
>
> 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(¶ms, 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, ¶ms);
> + 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.
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
