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

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 12 Feb 2025 10:46:51 +0800 Jeremy Kerr wrote:
> +	__u8 ep_in;
> +	__u8 ep_out;

same nit about u8 as on the header

> +	struct urb *tx_urb;
> +	struct urb *rx_urb;
> +};
> +
> +static void mctp_usb_stat_tx_dropped(struct net_device *dev)
> +{
> +	struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
> +
> +	u64_stats_update_begin(&dstats->syncp);
> +	u64_stats_inc(&dstats->tx_drops);
> +	u64_stats_update_end(&dstats->syncp);
> +}

Letter for letter dev_dstats_tx_dropped() ?

> +static void mctp_usb_stat_tx_done(struct net_device *dev, unsigned int len)
> +{
> +	struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
> +
> +	u64_stats_update_begin(&dstats->syncp);
> +	u64_stats_inc(&dstats->tx_packets);
> +	u64_stats_add(&dstats->tx_bytes, len);
> +	u64_stats_update_end(&dstats->syncp);
> +}

And this dev_dstats_tx_add() ?

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

> +	if (!hdr)
> +		goto err_drop;
> +
> +	hdr->id = cpu_to_be16(MCTP_USB_DMTF_ID);
> +	hdr->rsvd = 0;
> +	hdr->len = plen + sizeof(*hdr);

> +static void mctp_usb_in_complete(struct urb *urb)
> +{
> +	struct sk_buff *skb = urb->context;
> +	struct net_device *netdev = skb->dev;
> +	struct pcpu_dstats *dstats = this_cpu_ptr(netdev->dstats);
> +	struct mctp_usb *mctp_usb = netdev_priv(netdev);
> +	struct mctp_skb_cb *cb;
> +	unsigned int len;
> +	int status;
> +

> +		u64_stats_update_begin(&dstats->syncp);
> +		u64_stats_inc(&dstats->rx_packets);
> +		u64_stats_add(&dstats->rx_bytes, skb->len);
> +		u64_stats_update_end(&dstats->syncp);

dev_dstats_rx_add()

> +		skb->protocol = htons(ETH_P_MCTP);
> +		skb_reset_network_header(skb);
> +		cb = __mctp_cb(skb);
> +		cb->halen = 0;
> +		netif_rx(skb);
> +
> +		skb = skb2;
> +	}
> +
> +	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).

> +}

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

> +	dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
> +}
-- 
pw-bot: cr