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
