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 ;-)
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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
