Re: [PATCH net-next 2/2] net: mctp: Add MCTP USB transport driver

[Jeremy Kerr <jk@codeconstruct.com.au>]

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