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

[Felipe Balbi]

Hi,

John Youn <john.y...@synopsys.com> 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 assignment of transfer resources cannot perfectly follow the data
> book due to the fact that the controller driver does not have all
> knowledge of the configuration in advance. It is given this information
> piecemeal by the composite gadget framework after every
> SET_CONFIGURATION and SET_INTERFACE. Trying to follow the databook
> programming model in this scenario can cause errors. For two reasons:
>
> 1) The databook says to do DEPSTARTCFG for every SET_CONFIGURATION and
> SET_INTERFACE (8.1.5). This is incorrect in the scenario of multiple
> interfaces.
>
> 2) The databook does not mention doing more DEPXFERCFG for new endpoint
> on alt setting (8.1.6).
>
> The following simplified method is used instead:
>
> All hardware endpoints can be assigned a transfer resource and this
> setting will stay persistent until either a core reset or hibernation.
> So whenever we do a DEPSTARTCFG(0) we can go ahead and do DEPXFERCFG for
> every hardware endpoint as well. We are guaranteed that there are as
> many transfer resources as endpoints.
>
> This patch triggers off of the calling dwc3_gadget_start_config() for
> EP0-out, which always happens first, and which should only happen in one
> of the above conditions.

very good commit log. Thank you :-)

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

you missed:

Cc: <sta...@vger.kernel.org> # v....

> Reported-by: Ravi Babu <ravib...@ti.com>
> Signed-off-by: John Youn <johny...@synopsys.com>
> ---
>
> Hi Ravi,
>
> This is a simplified version of the previous patch. Could you verify
> that it works with your test scenario?
>
> Thanks,
> John
>
> v2:
> * Simplified assignment of resources by doing all endpoints at once.
>
>
>  drivers/usb/dwc3/core.h   |  1 -
>  drivers/usb/dwc3/ep0.c    |  5 -----
>  drivers/usb/dwc3/gadget.c | 38 ++++++++++++++++++++------------------
>  3 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2913068..e4f8b90 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -856,7 +856,6 @@ struct dwc3 {
>       unsigned                pullups_connected:1;
>       unsigned                resize_fifos:1;
>       unsigned                setup_packet_pending:1;
> -     unsigned                start_config_issued:1;
>       unsigned                three_stage_setup:1;
>       unsigned                usb3_lpm_capable:1;
>  
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 3a9354a..8d6b75c 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -555,7 +555,6 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct 
> usb_ctrlrequest *ctrl)
>       int ret;
>       u32 reg;
>  
> -     dwc->start_config_issued = false;
>       cfg = le16_to_cpu(ctrl->wValue);
>  
>       switch (state) {
> @@ -737,10 +736,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..ae2e546 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -385,24 +385,34 @@ static void dwc3_free_trb_pool(struct dwc3_ep *dep)
>       dep->trb_pool_dma = 0;
>  }
>  
> +static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep 
> *dep);
> +
>  static int dwc3_gadget_start_config(struct dwc3 *dwc, struct dwc3_ep *dep)
>  {
>       struct dwc3_gadget_ep_cmd_params params;
>       u32                     cmd;
> +     int                     i, ret;

I'd prefer if these two variables would be split into their own lines:

        int                     ret;
        int                     i;

> +     if (dep->number)
> +             return 0;
>  
>       memset(&params, 0x00, sizeof(params));
> +     cmd = DWC3_DEPCMD_DEPSTARTCFG;
>  
> -     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);
> -             }
> +     ret = dwc3_send_gadget_ep_cmd(dwc, 0, cmd, &params);
> +     if (ret)
> +             return ret;
> +
> +     for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
> +             int epnum, dir;

ditto. Also, (nit-picking, sorry) I'd prefer if the structure
declaration would come first:

                struct dwc3_ep *dep = dwc->eps[i];
                int epnum;
                int dir;

also, do you mind adding a comment somewhere here explaining that we're
not following the databook for a reason... It could be an almost
carbon-copy of your commit log, actually.

-- 
balbi

signature.asc
Description: PGP signature