netdev - Re: [PATCH] net: usb: ipheth: add CDC NCM support

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: <00275913-6b7c-9b91-4f5c-d8a425bd3e46@pen.gy>
Date: Wed, 17 May 2023 22:30:18 +0000
From: Forst <forst@....gy>
To: Simon Horman <simon.horman@...igine.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, linux-usb@...r.kernel.org, netdev@...r.kernel.org, Georgi Valkov <gvalkov@...il.com>
Subject: Re: [PATCH] net: usb: ipheth: add CDC NCM support

Hello Simon,

Thank you for the speedy review.

> nit: The target tree for this patch is probably net-next.
>      As such it should be included in the Subject:

Ack, will label the next patch revision accordingly. The netdev workflow
doc has been very useful.

> nit: Looking at Git history, probably the patch prefix should be
>      'usbnet: ipheth: '

I looked at the commit log for drivers/net/usb, and saw "net: usb:"
prefix a lot more often than "usbnet:", that's why I chose the former.
As for ipheth, you're right, "usbnet:" is more frequent. Will change to
"usbnet:" in v2, unless there's a different opinion on this. Naming
things is difficult. :)

> I see that this patch applies  to current net-next, which is where> development occurs. Has the patch been tested there?

I will make sure to test it on net-next before submitting v2, and make
an explicit note about that in the comments.

> I wonder if there are any issues with increasing the RX size
> from 1514 to 65536. Not that I have anything specific in mind.

The RX buffer increase was to accommodate for multiple Ethernet frames
encapsulated in a single URB. Per the USB NCM spec [1], an NTB-16 is
"shorter than 65,536 bytes", may include "up to forty 1514-byte
Ethernet frames". Frames themselves are still up to 1514 bytes each.

Somebody else may have to comment whether this has any other
implications from the kernel side of things.

> For networking code, please arrange local variables in reverse xmas tree
> order - longest line to shortest.

Ack

>> +	ncmh = (struct usb_cdc_ncm_nth16 *)(urb->transfer_buffer);
> nit: There is no need to cast a void pointer.
> 
>> +	ncm0 = (struct usb_cdc_ncm_ndp16 *)(urb->transfer_buffer + le16_to_cpu(ncmh->wNdpIndex));
> Ditto.

Ack

> nit: Lines less than 80 columns wide a re a bit nicer IMHO

Ack

>> +	if (retval == 0)
> nit: if (!retval)

Ack

>> @@ -510,8 +628,8 @@ static int ipheth_probe(struct usb_interface *intf,
>>  	ipheth_free_urbs(dev);
>>  err_alloc_urbs:
>>  err_get_macaddr:
>> -err_alloc_ctrl_buf:
>>  	kfree(dev->ctrl_buf);
>> +err_alloc_ctrl_buf:
>>  err_endpoints:
>>  	free_netdev(netdev);
>>  	return retval;
> 
> nit: this hunk seems unrelated to the rest of the patch

You're right, this should live in a separate patch. Would you suggest
making this a second patch in the same series, or submitting it
separately?

[1]: https://www.usb.org/document-library/network-control-model-devices-specification-v10-and-errata-and-adopters-agreement