lkml.org 
[lkml]   [2023]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs
From


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,

\
 
 \ /
  Last update: 2023-10-09 17:33    [W:0.143 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site