Hi Jakub,
Thanks for the review. Comments inline.
> > + __u8 ep_in;
> > + __u8 ep_out;
>
> same nit about u8 as on the header
Ack, have changed, as well as on the header.
> Letter for letter dev_dstats_tx_dropped() ?
[...]
> And this dev_dstats_tx_add() ?
[...]
> dev_dstats_rx_add()
Neat, thanks!
> > +static netdev_tx_t mctp_usb_start_xmit(struct sk_buff *skb,
> > + struct net_device *dev)
> > +{
> > + struct mctp_usb *mctp_usb = netdev_priv(dev);
> > + struct mctp_usb_hdr *hdr;
> > + unsigned int plen;
> > + struct urb *urb;
> > + int rc;
> > +
> > + plen = skb->len;
> > +
> > + if (plen + sizeof(*hdr) > MCTP_USB_XFER_SIZE)
> > + goto err_drop;
> > +
> > + hdr = skb_push(skb, sizeof(*hdr));
>
> Hm, I guess MCTP may have its own rules but technically you should
> call skb_cow_head() before you start writing to the header buffer.
We currently have ensured that we have headroom when the skb had been
allocated. However, things will get a bit more complex when we introduce
proper forwarding, so I will add the skb_cow_head() for v3 (and
introduce it on the other drivers as we go...)
> > + if (skb)
> > + kfree_skb(skb);
> > +
> > + mctp_usb_rx_queue(mctp_usb, GFP_ATOMIC);
>
> What if we fail to allocate an skb ?
> Admittedly the buffers are relatively small but if the allocation
> fails we'd get stuck, no more packets will ever be received, right?
> May be safer to allocate the skb first, and if it fails reuse the
> skb that just completed (effectively discarding the incoming packets
> until a replacement buffer can be allocated).
I think we can do a little better, and defer retries to a non-atomic
context instead. This means we have a chance of flow control over the
IN transfers from the device too, rather than dropping everything.
I'll implement that for v3.
> > + dev->hard_header_len = sizeof(struct mctp_usb_hdr);
> > + dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> > + dev->addr_len = 0;
> > + dev->flags = IFF_NOARP;
> > + dev->netdev_ops = &mctp_usb_netdev_ops;
> > + dev->needs_free_netdev = false;
>
> Is there a reason to set this to false?
> dev memory is guaranteed to be zero'ed out.
.. only because I had previously had it as true before the usb
disconnect was implemented. With that change, I had decided to not
remove it with the justification that it's a little more clear that we
need to do our own free after unregister.
Happy to make this more conventional though, so will remove (as well
as the addr_len assignment) in v3.
Cheers,
Jeremy
