Hi Oliver,
Thanks for taking a look. Some responses too.
> > + hdr = skb_pull_data(skb, sizeof(*hdr));
> > + if (!hdr)
> > + break;
> > +
> > + if (be16_to_cpu(hdr->id) != MCTP_USB_DMTF_ID) {
>
> It would be more efficient to do the conversion on the constant
Compiler should be clever enough for that not to make a difference:
$ diff -u \
<(arm-linux-gnueabihf-objdump -d obj/drivers/net/mctp/mctp-usb.o.orig) \
<(arm-linux-gnueabihf-objdump -d obj/drivers/net/mctp/mctp-usb.o)
--- /dev/fd/63 2025-02-07 15:32:53.813084894 +0800
+++ /dev/fd/62 2025-02-07 15:32:53.809084826 +0800
@@ -1,5 +1,5 @@
-obj/drivers/net/mctp/mctp-usb.o.orig: file format elf32-littlearm
+obj/drivers/net/mctp/mctp-usb.o: file format elf32-littlearm
Disassembly of section .text:
$
And endian-converting the header field (rather than the const) seems
more readable to me.
> > + if (pkt_len < skb->len) {
> > + /* more packets may follow - clone to a new
> > + * skb to use on the next iteration
> > + */
> > + skb2 = skb_clone(skb, GFP_ATOMIC);
> > + if (skb2) {
> > + if (!skb_pull(skb2, pkt_len)) {
> > + kfree_skb(skb2);
> > + skb2 = NULL;
> > + }
> > + }
> > + skb_trim(skb, pkt_len);
>
> This is functional. Though in terms of algorithm you are copying
> the same data multiple times.
There should be no copy here; they're shared clones of the same buffer.
Or am I missing some situation where they would get unshared?
> > +static int mctp_usb_open(struct net_device *dev)
> > +{
> > + struct mctp_usb *mctp_usb = netdev_priv(dev);
> > +
> > + return mctp_usb_rx_queue(mctp_usb);
>
> This will needlessly use GFP_ATOMIC
It's only the one (first) skb and urb submission, but fair enough. I'll
add a gfp_t argument in a v2.
> > + SET_NETDEV_DEV(netdev, &intf->dev);
> > + dev = netdev_priv(netdev);
> > + dev->netdev = netdev;
> > + dev->usbdev = usb_get_dev(interface_to_usbdev(intf));
>
> Taking a reference.
> Where is the corresponding put?
Good catch - we should have one in disconnect(). Coming up in v2 too.
Cheers,
Jeremy
