Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Mon, 9 Oct 2023 17:08:31 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Krishna Kurapati <quic_kriskura@quicinc.com>
Cc:     Maciej =?utf-8?Q?=C5=BBenczykowski?= <maze@google.com>,
        onathan Corbet <corbet@lwn.net>,
        Linyu Yuan <quic_linyyuan@quicinc.com>,
        linux-usb@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
        quic_wcheng@quicinc.com, quic_jackp@quicinc.com
Subject: Re: [PATCH 2/2] usb: gadget: ncm: Add support to update
 wMaxSegmentSize via configfs
Message-ID: <2023100931-reward-justice-ed1c@gregkh>
References: <20231009142005.21338-1-quic_kriskura@quicinc.com>
 <20231009142005.21338-2-quic_kriskura@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231009142005.21338-2-quic_kriskura@quicinc.com>
Precedence: bulk

On Mon, Oct 09, 2023 at 07:50:05PM +0530, Krishna Kurapati wrote:
> Currently the NCM driver restricts wMaxSegmentSize that indicates
> the datagram size coming from network layer to 1514.

I don't see that restriction in the existing driver, where does that
happen?

> However the spec doesn't have any limitation.

What spec?

> For P2P connections over NCM, increasing MTU helps increasing
> throughput.

While increasing latency, right?

> Add support to configure this value before configfs symlink is
> created. Also since the NTB Out/In buffer sizes are fixed at 16384
> bytes, limit the segment size to an upper cap of 15014. Set the
> default MTU size for the ncm interface during function bind before
> network interface is registered allowing MTU to be set in parity
> with wMaxSegmentSize.

Where does 15014 come from?

> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/gadget/function/f_ncm.c | 51 +++++++++++++++++++++++++++++
>  drivers/usb/gadget/function/u_ncm.h |  2 ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index feccf4c8cc4f..eab297b22200 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -103,6 +103,8 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
>  /* Delay for the transmit to wait before sending an unfilled NTB frame. */
>  #define TX_TIMEOUT_NSECS	300000
>  
> +#define MAX_DATAGRAM_SIZE	15014

Where does this magic value come from?  Please document it really really
well.

> +
>  #define FORMATS_SUPPORTED	(USB_CDC_NCM_NTB16_SUPPORTED |	\
>  				 USB_CDC_NCM_NTB32_SUPPORTED)
>  
> @@ -1408,6 +1410,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>  	ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst);
>  
>  	if (cdev->use_os_string) {
> +		ncm_opts->net->mtu = (ncm_opts->max_segment_size - ETH_HLEN);
>  		f->os_desc_table = kzalloc(sizeof(*f->os_desc_table),
>  					   GFP_KERNEL);
>  		if (!f->os_desc_table)
> @@ -1469,6 +1472,8 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	status = -ENODEV;
>  
> +	ecm_desc.wMaxSegmentSize = ncm_opts->max_segment_size;
> +
>  	/* allocate instance-specific endpoints */
>  	ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_in_desc);
>  	if (!ep)
> @@ -1569,11 +1574,56 @@ USB_ETHERNET_CONFIGFS_ITEM_ATTR_QMULT(ncm);
>  /* f_ncm_opts_ifname */
>  USB_ETHERNET_CONFIGFS_ITEM_ATTR_IFNAME(ncm);
>  
> +static ssize_t ncm_opts_max_segment_size_show(struct config_item *item,
> +					      char *page)
> +{
> +	struct f_ncm_opts *opts = to_f_ncm_opts(item);
> +	u32 segment_size;
> +
> +	mutex_lock(&opts->lock);
> +	segment_size = opts->max_segment_size;
> +	mutex_unlock(&opts->lock);
> +
> +	return sprintf(page, "%u\n", segment_size);

sysfs_emit()?

thanks,

greg k-h