Hi Oliver,

Thanks for taking a look. Some responses too.

quoted

+               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.

quoted

+               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?

quoted

+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.

quoted

+       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