[PATCH net-next v2 0/2] mctp: Add MCTP-over-USB hardware transport binding

[PATCH net-next v2 0/2] mctp: Add MCTP-over-USB hardware transport binding

[Jeremy Kerr]

Add an implementation of the DMTF standard DSP0283, providing an MCTP
channel over high-speed USB.

This is a fairly trivial first implementation, in that we only submit
one tx and one rx URB at a time. We do accept multi-packet transfers,
but do not yet generate them on transmit.

Of course, questions and comments are most welcome, particularly on the
USB interfaces.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
Changes in v2:
- greg k-h claims that it is 2025; update copyright year
- Add spec references
- Clean up dbg/warn output
- Changes from Oliver Neukum: drop usbdev ref, avoid a GFP_ATOMIC alloc
- Changes from Simon Horman: do rx stats before netif_rx
- Add module metadata
- specify phys binding type
- Link to v1: https://lore.kernel.org/r/20250206-dev-mctp-usb-v1-0-81453fe26a61@codeconstruct.com.au

---
Jeremy Kerr (2):
      usb: Add base USB MCTP definitions
      net: mctp: Add MCTP USB transport driver

 MAINTAINERS                  |   1 +
 drivers/net/mctp/Kconfig     |  10 ++
 drivers/net/mctp/Makefile    |   1 +
 drivers/net/mctp/mctp-usb.c  | 368 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/mctp-usb.h |  30 ++++
 include/uapi/linux/usb/ch9.h |   1 +
 6 files changed, 411 insertions(+)
---
base-commit: be1d2a1b151deb195cd9749988163aa26ad6f616
change-id: 20250206-dev-mctp-usb-c59dea025395

Best regards,
-- 
Jeremy Kerr <jk@codeconstruct.com.au>

[PATCH net-next v2 1/2] usb: Add base USB MCTP definitions

[Jeremy Kerr]

Upcoming changes will add a USB host (and later gadget) driver for the
MCTP-over-USB protocol. Add a header that provides common definitions
for protocol support: the packet header format and a few framing
definitions. Add a define for the MCTP class code, as per
https://usb.org/defined-class-codes.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

---
v2:
 - add reference & URL to DSP0283
 - update copyright year
---
 MAINTAINERS                  |  1 +
 include/linux/usb/mctp-usb.h | 30 ++++++++++++++++++++++++++++++
 include/uapi/linux/usb/ch9.h |  1 +
 3 files changed, 32 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 67665d9dd536873e94afffc00393c2fe2e8c2797..e7b326dba9a9e6f50c3beeb172d93641841f6242 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13903,6 +13903,7 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/networking/mctp.rst
 F:	drivers/net/mctp/
+F:	include/linux/usb/mctp-usb.h
 F:	include/net/mctp.h
 F:	include/net/mctpdevice.h
 F:	include/net/netns/mctp.h
diff --git a/include/linux/usb/mctp-usb.h b/include/linux/usb/mctp-usb.h
new file mode 100644
index 0000000000000000000000000000000000000000..b19392aa29310eda504f65bd098c849bd02dc0a1
--- /dev/null
+++ b/include/linux/usb/mctp-usb.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * mctp-usb.h - MCTP USB transport binding: common definitions,
+ * based on DMTF0283 specification:
+ * https://www.dmtf.org/sites/default/files/standards/documents/DSP0283_1.0.1.pdf
+ *
+ * These are protocol-level definitions, that may be shared between host
+ * and gadget drivers.
+ *
+ * Copyright (C) 2024-2025 Code Construct Pty Ltd
+ */
+
+#ifndef __LINUX_USB_MCTP_USB_H
+#define __LINUX_USB_MCTP_USB_H
+
+#include <linux/types.h>
+
+struct mctp_usb_hdr {
+	__be16	id;
+	__u8	rsvd;
+	__u8	len;
+} __packed;
+
+#define MCTP_USB_XFER_SIZE	512
+#define MCTP_USB_BTU		68
+#define MCTP_USB_MTU_MIN	MCTP_USB_BTU
+#define MCTP_USB_MTU_MAX	(U8_MAX - sizeof(struct mctp_usb_hdr))
+#define MCTP_USB_DMTF_ID	0x1ab4
+
+#endif /*  __LINUX_USB_MCTP_USB_H */
diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
index 91f0f7e214a5a57c8bee3f44c4dbf7b175843d8c..052290652046591fba46f1f0cb5cf77fd965f555 100644
--- a/include/uapi/linux/usb/ch9.h
+++ b/include/uapi/linux/usb/ch9.h
@@ -330,6 +330,7 @@ struct usb_device_descriptor {
 #define USB_CLASS_AUDIO_VIDEO		0x10
 #define USB_CLASS_BILLBOARD		0x11
 #define USB_CLASS_USB_TYPE_C_BRIDGE	0x12
+#define USB_CLASS_MCTP			0x14
 #define USB_CLASS_MISC			0xef
 #define USB_CLASS_APP_SPEC		0xfe
 #define USB_SUBCLASS_DFU			0x01

-- 
2.39.5

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

[Jeremy Kerr]

Add an implementation for DMTF DSP0283, which defines a MCTP-over-USB
transport. As per that spec, we're restricted to full speed mode,
requiring 512-byte transfers.

Each MCTP-over-USB interface is a peer-to-peer link to a single MCTP
endpoint, so no physical addressing is required (of course, that MCTP
endpoint may then bridge to further MCTP endpoints). Consequently,
interfaces will report with no lladdr data:

    # mctp link
    dev lo index 1 address 00:00:00:00:00:00 net 1 mtu 65536 up
    dev mctpusb0 index 6 address none net 1 mtu 68 up

This is a simple initial implementation, with single rx & tx urbs, and
no multi-packet tx transfers - although we do accept multi-packet rx
from the device.

Includes suggested fixes from Santosh Puranik <spuranik@nvidia.com>.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Cc: Santosh Puranik <spuranik@nvidia.com>

---
v2:
 - update copyright year to 2025
 - improve dbg messages: use netdev, demote from _err, remove __func__
 - pass gfp_t to mctp_usb_rx_queue, allocate first as GFP_KERNEL
 - release ->usbdev on disconnect
 - include MODULE_{DESCRIPTION,AUTHOR,DEVICE_TABLE}
 - more sensible driver->name
 - do rx stats update before netif_rx()
 - specify phys binding type via mctp_register_netdev()
---
 drivers/net/mctp/Kconfig    |  10 ++
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-usb.c | 368 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 379 insertions(+)

diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index 15860d6ac39fef62847d7186f1f0d81c1d3cd619..cf325ab0b1ef555e21983ace1b838e10c7f34570 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -47,6 +47,16 @@ config MCTP_TRANSPORT_I3C
 	  A MCTP protocol network device is created for each I3C bus
 	  having a "mctp-controller" devicetree property.
 
+config MCTP_TRANSPORT_USB
+	tristate "MCTP USB transport"
+	depends on USB
+	help
+	  Provides a driver to access MCTP devices over USB transport,
+	  defined by DMTF specification DSP0283.
+
+	  MCTP-over-USB interfaces are peer-to-peer, so each interface
+	  represents a physical connection to one remote MCTP endpoint.
+
 endmenu
 
 endif
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index e1cb99ced54ac136db0347a9ee0435a5ed938955..c36006849a1e7d04f2cafafb8931329fc0992b63 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
 obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
 obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
+obj-$(CONFIG_MCTP_TRANSPORT_USB) += mctp-usb.o
diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c
new file mode 100644
index 0000000000000000000000000000000000000000..9ffca70811cffbd609aeb2ac7f6df3ba69b8008a
--- /dev/null
+++ b/drivers/net/mctp/mctp-usb.c
@@ -0,0 +1,368 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * mctp-usb.c - MCTP-over-USB (DMTF DSP0283) transport binding driver.
+ *
+ * DSP0283 is available at:
+ * https://www.dmtf.org/sites/default/files/standards/documents/DSP0283_1.0.1.pdf
+ *
+ * Copyright (C) 2024-2025 Code Construct Pty Ltd
+ */
+
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/usb.h>
+#include <linux/usb/mctp-usb.h>
+
+#include <net/mctp.h>
+#include <net/mctpdevice.h>
+#include <net/pkt_sched.h>
+
+#include <uapi/linux/if_arp.h>
+
+struct mctp_usb {
+	struct usb_device *usbdev;
+	struct usb_interface *intf;
+
+	struct net_device *netdev;
+
+	__u8 ep_in;
+	__u8 ep_out;
+
+	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);
+}
+
+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);
+}
+
+static void mctp_usb_out_complete(struct urb *urb)
+{
+	struct sk_buff *skb = urb->context;
+	struct net_device *netdev = skb->dev;
+	int status;
+
+	status = urb->status;
+
+	switch (status) {
+	case -ENOENT:
+	case -ECONNRESET:
+	case -ESHUTDOWN:
+	case -EPROTO:
+		mctp_usb_stat_tx_dropped(netdev);
+		break;
+	case 0:
+		mctp_usb_stat_tx_done(netdev, skb->len);
+		netif_wake_queue(netdev);
+		consume_skb(skb);
+		return;
+	default:
+		netdev_dbg(netdev, "unexpected tx urb status: %d\n", status);
+		mctp_usb_stat_tx_dropped(netdev);
+	}
+
+	kfree_skb(skb);
+}
+
+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));
+	if (!hdr)
+		goto err_drop;
+
+	hdr->id = cpu_to_be16(MCTP_USB_DMTF_ID);
+	hdr->rsvd = 0;
+	hdr->len = plen + sizeof(*hdr);
+
+	urb = mctp_usb->tx_urb;
+
+	usb_fill_bulk_urb(urb, mctp_usb->usbdev,
+			  usb_sndbulkpipe(mctp_usb->usbdev, mctp_usb->ep_out),
+			  skb->data, skb->len,
+			  mctp_usb_out_complete, skb);
+
+	rc = usb_submit_urb(urb, GFP_ATOMIC);
+	if (rc)
+		goto err_drop;
+	else
+		netif_stop_queue(dev);
+
+	return NETDEV_TX_OK;
+
+err_drop:
+	mctp_usb_stat_tx_dropped(dev);
+	kfree_skb(skb);
+	return NETDEV_TX_OK;
+}
+
+static void mctp_usb_in_complete(struct urb *urb);
+
+static int mctp_usb_rx_queue(struct mctp_usb *mctp_usb, gfp_t gfp)
+{
+	struct sk_buff *skb;
+	int rc;
+
+	skb = __netdev_alloc_skb(mctp_usb->netdev, MCTP_USB_XFER_SIZE, gfp);
+	if (!skb)
+		return -ENOMEM;
+
+	usb_fill_bulk_urb(mctp_usb->rx_urb, mctp_usb->usbdev,
+			  usb_rcvbulkpipe(mctp_usb->usbdev, mctp_usb->ep_in),
+			  skb->data, MCTP_USB_XFER_SIZE,
+			  mctp_usb_in_complete, skb);
+
+	rc = usb_submit_urb(mctp_usb->rx_urb, gfp);
+	if (rc) {
+		netdev_err(mctp_usb->netdev, "rx urb submit failure: %d\n", rc);
+		kfree_skb(skb);
+	}
+
+	return rc;
+}
+
+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;
+
+	status = urb->status;
+
+	switch (status) {
+	case -ENOENT:
+	case -ECONNRESET:
+	case -ESHUTDOWN:
+	case -EPROTO:
+		kfree_skb(skb);
+		return;
+	case 0:
+		break;
+	default:
+		netdev_dbg(netdev, "unexpected rx urb status: %d\n", status);
+		kfree_skb(skb);
+		return;
+	}
+
+	len = urb->actual_length;
+	__skb_put(skb, len);
+
+	while (skb) {
+		struct sk_buff *skb2 = NULL;
+		struct mctp_usb_hdr *hdr;
+		u8 pkt_len; /* length of MCTP packet, no USB header */
+
+		hdr = skb_pull_data(skb, sizeof(*hdr));
+		if (!hdr)
+			break;
+
+		if (be16_to_cpu(hdr->id) != MCTP_USB_DMTF_ID) {
+			netdev_dbg(netdev, "rx: invalid id %04x\n",
+				   be16_to_cpu(hdr->id));
+			break;
+		}
+
+		if (hdr->len <
+		    sizeof(struct mctp_hdr) + sizeof(struct mctp_usb_hdr)) {
+			netdev_dbg(netdev, "rx: short packet (hdr) %d\n",
+				   hdr->len);
+			break;
+		}
+
+		/* we know we have at least sizeof(struct mctp_usb_hdr) here */
+		pkt_len = hdr->len - sizeof(struct mctp_usb_hdr);
+		if (pkt_len > skb->len) {
+			netdev_dbg(netdev,
+				   "rx: short packet (xfer) %d, actual %d\n",
+				   hdr->len, skb->len);
+			break;
+		}
+
+		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);
+		}
+
+		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);
+
+		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);
+}
+
+static int mctp_usb_open(struct net_device *dev)
+{
+	struct mctp_usb *mctp_usb = netdev_priv(dev);
+
+	return mctp_usb_rx_queue(mctp_usb, GFP_KERNEL);
+}
+
+static int mctp_usb_stop(struct net_device *dev)
+{
+	struct mctp_usb *mctp_usb = netdev_priv(dev);
+
+	netif_stop_queue(dev);
+	usb_kill_urb(mctp_usb->rx_urb);
+	usb_kill_urb(mctp_usb->tx_urb);
+
+	return 0;
+}
+
+static const struct net_device_ops mctp_usb_netdev_ops = {
+	.ndo_start_xmit = mctp_usb_start_xmit,
+	.ndo_open = mctp_usb_open,
+	.ndo_stop = mctp_usb_stop,
+};
+
+static void mctp_usb_netdev_setup(struct net_device *dev)
+{
+	dev->type = ARPHRD_MCTP;
+
+	dev->mtu = MCTP_USB_MTU_MIN;
+	dev->min_mtu = MCTP_USB_MTU_MIN;
+	dev->max_mtu = MCTP_USB_MTU_MAX;
+
+	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;
+	dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
+}
+
+static int mctp_usb_probe(struct usb_interface *intf,
+			  const struct usb_device_id *id)
+{
+	struct usb_endpoint_descriptor *ep_in, *ep_out;
+	struct usb_host_interface *iface_desc;
+	struct net_device *netdev;
+	struct mctp_usb *dev;
+	int rc;
+
+	/* only one alternate */
+	iface_desc = intf->cur_altsetting;
+
+	rc = usb_find_common_endpoints(iface_desc, &ep_in, &ep_out, NULL, NULL);
+	if (rc) {
+		dev_err(&intf->dev, "invalid endpoints on device?\n");
+		return rc;
+	}
+
+	netdev = alloc_netdev(sizeof(*dev), "mctpusb%d", NET_NAME_ENUM,
+			      mctp_usb_netdev_setup);
+	if (!netdev)
+		return -ENOMEM;
+
+	SET_NETDEV_DEV(netdev, &intf->dev);
+	dev = netdev_priv(netdev);
+	dev->netdev = netdev;
+	dev->usbdev = usb_get_dev(interface_to_usbdev(intf));
+	dev->intf = intf;
+	usb_set_intfdata(intf, dev);
+
+	dev->ep_in = ep_in->bEndpointAddress;
+	dev->ep_out = ep_out->bEndpointAddress;
+
+	dev->tx_urb = usb_alloc_urb(0, GFP_KERNEL);
+	dev->rx_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!dev->tx_urb || !dev->rx_urb) {
+		rc = -ENOMEM;
+		goto err_free_urbs;
+	}
+
+	rc = mctp_register_netdev(netdev, NULL, MCTP_PHYS_BINDING_USB);
+	if (rc)
+		goto err_free_urbs;
+
+	return 0;
+
+err_free_urbs:
+	usb_free_urb(dev->tx_urb);
+	usb_free_urb(dev->rx_urb);
+	free_netdev(netdev);
+	return rc;
+}
+
+static void mctp_usb_disconnect(struct usb_interface *intf)
+{
+	struct mctp_usb *dev = usb_get_intfdata(intf);
+
+	mctp_unregister_netdev(dev->netdev);
+	usb_free_urb(dev->tx_urb);
+	usb_free_urb(dev->rx_urb);
+	usb_put_dev(dev->usbdev);
+	free_netdev(dev->netdev);
+}
+
+static const struct usb_device_id mctp_usb_devices[] = {
+	{ USB_INTERFACE_INFO(USB_CLASS_MCTP, 0x0, 0x1) },
+	{ 0 },
+};
+
+MODULE_DEVICE_TABLE(usb, mctp_usb_devices);
+
+static struct usb_driver mctp_usb_driver = {
+	.name		= "mctp-usb",
+	.id_table	= mctp_usb_devices,
+	.probe		= mctp_usb_probe,
+	.disconnect	= mctp_usb_disconnect,
+};
+
+module_usb_driver(mctp_usb_driver)
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jeremy Kerr <jk@codeconstruct.com.au>");
+MODULE_DESCRIPTION("MCTP USB transport");

-- 
2.39.5

Re: [PATCH net-next v2 1/2] usb: Add base USB MCTP definitions

[Greg Kroah-Hartman]

On Wed, Feb 12, 2025 at 10:46:50AM +0800, Jeremy Kerr wrote:
> Upcoming changes will add a USB host (and later gadget) driver for the
> MCTP-over-USB protocol. Add a header that provides common definitions
> for protocol support: the packet header format and a few framing
> definitions. Add a define for the MCTP class code, as per
> https://usb.org/defined-class-codes.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> ---
> v2:
>  - add reference & URL to DSP0283
>  - update copyright year
> ---
>  MAINTAINERS                  |  1 +
>  include/linux/usb/mctp-usb.h | 30 ++++++++++++++++++++++++++++++
>  include/uapi/linux/usb/ch9.h |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 67665d9dd536873e94afffc00393c2fe2e8c2797..e7b326dba9a9e6f50c3beeb172d93641841f6242 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13903,6 +13903,7 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/networking/mctp.rst
>  F:	drivers/net/mctp/
> +F:	include/linux/usb/mctp-usb.h
>  F:	include/net/mctp.h
>  F:	include/net/mctpdevice.h
>  F:	include/net/netns/mctp.h
> diff --git a/include/linux/usb/mctp-usb.h b/include/linux/usb/mctp-usb.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..b19392aa29310eda504f65bd098c849bd02dc0a1
> --- /dev/null
> +++ b/include/linux/usb/mctp-usb.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */

I missed this the last time, sorry, but I have to ask, do you really
mean v2 or later?  If so, that's fine, just want to make sure.

Whichever you pick is fine with me, so:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH net-next v2 1/2] usb: Add base USB MCTP definitions

[Jakub Kicinski]

On Wed, 12 Feb 2025 10:46:50 +0800 Jeremy Kerr wrote:
> +	__u8	rsvd;
> +	__u8	len;

Since this is not a uAPI header u8 is preferred to __u8

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

[Jakub Kicinski]

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

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

[Jeremy Kerr]

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

Re: [PATCH net-next v2 1/2] usb: Add base USB MCTP definitions

[Jeremy Kerr]

Hi Greg,

> > --- /dev/null
> > +++ b/include/linux/usb/mctp-usb.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> 
> I missed this the last time, sorry, but I have to ask, do you really
> mean v2 or later?  If so, that's fine, just want to make sure.

I'm fine with 2.0+, but I figure the preference is consistency here. So,
since I'm doing a v3, I will send that out with GPL-2.0.

> Whichever you pick is fine with me, so:
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks! v3 will have the __u8s changed to u8, as Jakub has requested.
Would you like me to keep the Ack on that?

Cheers,


Jeremy

Re: [PATCH net-next v2 1/2] usb: Add base USB MCTP definitions

[Greg Kroah-Hartman]

On Mon, Feb 17, 2025 at 04:55:59PM +0800, Jeremy Kerr wrote:
> Hi Greg,
> 
> > > --- /dev/null
> > > +++ b/include/linux/usb/mctp-usb.h
> > > @@ -0,0 +1,30 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > 
> > I missed this the last time, sorry, but I have to ask, do you really
> > mean v2 or later?  If so, that's fine, just want to make sure.
> 
> I'm fine with 2.0+, but I figure the preference is consistency here. So,
> since I'm doing a v3, I will send that out with GPL-2.0.
> 
> > Whichever you pick is fine with me, so:
> > 
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Thanks! v3 will have the __u8s changed to u8, as Jakub has requested.
> Would you like me to keep the Ack on that?

Yes, that's fine.  But to be fair, "__u8" is correct here as that's what
the endpoint variable is as well (it is coming directly from hardware),
but I'm not going to complain about that as it really doesn't matter :)

thanks,

greg k-h