Re: [PATCH 2/2] usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

On 10/9/2023 8:38 PM, Greg Kroah-Hartman wrote:
> 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?

Hi Greg,

  In the ecm_desc, the following line restricts the value:

.wMaxSegmentSize =      cpu_to_le16(ETH_FRAME_LEN),

> 
>> However the spec doesn't have any limitation.
> 
> What spec?

NCM Specification.
I didn't mention "NCM specification" as I thought the patch header would 
imply it is NCM Spec. Will update the wording here.

> 
>> For P2P connections over NCM, increasing MTU helps increasing
>> throughput.
> 
> While increasing latency, right?

I used iperf for capturing the data. I was more focussing on throughput.

Here are some results I found:

For throughput, the increase is as follows (HS link with and an old iperf):

MTU Size	Throughput
1500		145
2500		204
3500		208
4500		223
5500		227
6500		299
7500		324
8050		335

As per the latency, an internal test application I was using didn't show 
much difference in latency as MTU was increasing. Then again, it could 
be application specific.

> 
>> 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.
> 
Sorry for not being clear. The 14 here meant ETH_HLEN which gets 
appended to MTU usually. I wanted to limit mtu to 15000 max (via 
wMaxsegment size) and mentioned it here as 15014.

Will update comments here in v2.

>> +
>>   #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()?
> 
Apologies.

I followed u_ether_configfs.h which used sprintf. Will replace it with 
sysfs_emit in v2.

Thanks for the review.

Regards,
Krishna,