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)
> I'll resend with your feedback shortly.
great, thanks.
--
balbi
Attachment:
signature.asc
Description: PGP signature
