linux-kernel - Re: [PATCH v3] usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240917224026.qsewm3epwaubcgs6@synopsys.com>
Date: Tue, 17 Sep 2024 22:40:31 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Akash Kumar <quic_akakum@...cinc.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jing Leng <jleng@...arella.com>, Felipe Balbi <balbi@...nel.org>,
        Jack Pham <quic_jackp@...cinc.com>,
        "kernel@...cinc.com" <kernel@...cinc.com>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Daniel Scally <dan.scally@...asonboard.com>,
        Vijayavardhan Vennapusa <quic_vvreddy@...cinc.com>,
        Krishna Kurapati <quic_kriskura@...cinc.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] usb: dwc3: gadget: Refine the logic for resizing Tx
 FIFOs

On Tue, Sep 17, 2024, Akash Kumar wrote:
> The current logic is rigid, setting num_fifos to fixed values:
> 
> 3 for any maxburst greater than 1.
> tx_fifo_resize_max_num for maxburst greater than 6.
> Additionally, it did not differentiate much between bulk and
> isochronous transfers, applying similar logic to both.
> 
> The new logic is more dynamic and tailored to the specific needs of
> bulk and isochronous transfers:
> 
> Bulk Transfers: Ensures that num_fifos is optimized by considering
> both the maxburst value and the maximum allowed number of FIFOs
> based on the DT property tx_fifo_resize_max_num and the maximum
> packet multiplier for HS.
> 
> Isochronous Transfers: Ensures that num_fifos is sufficient by
> considering the maximum packet multiplier for HS and maxburst for SS,
> along with a constraint with the DT property tx_fifo_resize_max_num.
> 
> This change aims to optimize the allocation of Tx FIFOs for both bulk
> and isochronous endpoints, potentially improving data transfer
> efficiency and overall performance. It also enhances support for all
> use cases, which can be tweaked with DT parameters and the

You should clarify that this is only verified on your specific platform
and specific application. It may not be applicable to all cases.
However, we try our best to make it so. Please reword as such. The
commit message makes it seems that this will work for all cases.

> endpoint’s maxburst and maxpacket
> 
> Signed-off-by: Akash Kumar <quic_akakum@...cinc.com>
> ---
>  Changes for v3:
>  Redefine logic for resizing tx fifos,added check based on
>  operating speed and used maxp for HS and maxburst for SS
>  and defined max allocation based on dt property.
> 
>  Changes for v2:
>  Redefine logic for resizing tx fifos, handled fifo based on
>  minimum of maxp and maxburts.
> 
>  Changes for v1:
>  Added additional condition to allocate tx fifo for hs isoc
>  eps, keeping the other resize logic same.
> ---
>  drivers/usb/dwc3/gadget.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 89fc690fdf34..7557bd0053a7 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -778,15 +778,19 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)
>  
>  	ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7);
>  
> -	if ((dep->endpoint.maxburst > 1 &&
> -	     usb_endpoint_xfer_bulk(dep->endpoint.desc)) ||
> -	    usb_endpoint_xfer_isoc(dep->endpoint.desc))
> -		num_fifos = 3;
> +	if (dwc->gadget->speed <= USB_SPEED_HIGH &&
> +	    (usb_endpoint_xfer_bulk(dep->endpoint.desc) ||
> +	     usb_endpoint_xfer_isoc(dep->endpoint.desc)))
> +		num_fifos = min_t(unsigned int,
> +				  usb_endpoint_maxp_mult(dep->endpoint.desc) + 1,

This logic looks wrong. This implies maxp_mult is applicable to bulk and
that it's also applicable to speed below highspeed, which it isn't.

> +				  dwc->tx_fifo_resize_max_num);
>  
> -	if (dep->endpoint.maxburst > 6 &&
> +	if (dwc->gadget->speed > USB_SPEED_HIGH &&
>  	    (usb_endpoint_xfer_bulk(dep->endpoint.desc) ||
> -	     usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31))
> -		num_fifos = dwc->tx_fifo_resize_max_num;
> +	     usb_endpoint_xfer_isoc(dep->endpoint.desc)))
> +		num_fifos = min_t(unsigned int,
> +				  dep->endpoint.maxburst,

maxburst can be 0 right?

> +				  dwc->tx_fifo_resize_max_num);
>  
>  	/* FIFO size for a single buffer */
>  	fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1);
> -- 
> 2.17.1
> 

Thanks,
Thinh