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

[PATCH net-next 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>
---
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  | 367 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/mctp-usb.h |  28 ++++
 include/uapi/linux/usb/ch9.h |   1 +
 6 files changed, 408 insertions(+)
---
base-commit: 7ea2745766d776866cfbc981b21ed3cfdf50124e
change-id: 20250206-dev-mctp-usb-c59dea025395

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

[PATCH net-next 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>
---
 MAINTAINERS                  |  1 +
 include/linux/usb/mctp-usb.h | 28 ++++++++++++++++++++++++++++
 include/uapi/linux/usb/ch9.h |  1 +
 3 files changed, 30 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 79756f2100e001177191b129c48cf49e90173a68..f4e093674cf07260ca1cbb5a8873bdff782c614d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13775,6 +13775,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..ad58a7edff8d5228717f9add22615c3fad7d4cde
--- /dev/null
+++ b/include/linux/usb/mctp-usb.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * mctp-usb.h - MCTP USB transport binding: common definitions.
+ *
+ * These are protocol-level definitions, that may be shared between host
+ * and gadget drivers.
+ *
+ * Copyright (C) 2024 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

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

[Greg Kroah-Hartman]

On Thu, Feb 06, 2025 at 02:48:23PM +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>
> ---
>  MAINTAINERS                  |  1 +
>  include/linux/usb/mctp-usb.h | 28 ++++++++++++++++++++++++++++
>  include/uapi/linux/usb/ch9.h |  1 +
>  3 files changed, 30 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 79756f2100e001177191b129c48cf49e90173a68..f4e093674cf07260ca1cbb5a8873bdff782c614d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13775,6 +13775,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..ad58a7edff8d5228717f9add22615c3fad7d4cde
> --- /dev/null
> +++ b/include/linux/usb/mctp-usb.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * mctp-usb.h - MCTP USB transport binding: common definitions.
> + *
> + * These are protocol-level definitions, that may be shared between host
> + * and gadget drivers.

Perhaps add a link to the spec?

> + *
> + * Copyright (C) 2024 Code Construct Pty Ltd

It's 2025 now :)

thanks,

greg k-h

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

[Jeremy Kerr]

Hi Greg,

Thanks for the review.

> > --- /dev/null
> > +++ b/include/linux/usb/mctp-usb.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * mctp-usb.h - MCTP USB transport binding: common definitions.
> > + *
> > + * These are protocol-level definitions, that may be shared between host
> > + * and gadget drivers.
> 
> Perhaps add a link to the spec?

Can do. I have one in the actual driver, but can replicate that here if
it's helpful.

> > + *
> > + * Copyright (C) 2024 Code Construct Pty Ltd
> 
> It's 2025 now :)

WHAT?!

:D

(this was started in 2024, and I have some preliminary versions up since
then, but I assume the last date is preferrable)

Unless I hear otherwise, I'll v2 this after a netdev-appropriate
backoff, with the added link and 2025 for both patches.

Cheers,


Jeremy

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

[Greg Kroah-Hartman]

On Thu, Feb 06, 2025 at 03:11:25PM +0800, Jeremy Kerr wrote:
> Hi Greg,
> 
> Thanks for the review.
> 
> > > --- /dev/null
> > > +++ b/include/linux/usb/mctp-usb.h
> > > @@ -0,0 +1,28 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * mctp-usb.h - MCTP USB transport binding: common definitions.
> > > + *
> > > + * These are protocol-level definitions, that may be shared between host
> > > + * and gadget drivers.
> > 
> > Perhaps add a link to the spec?
> 
> Can do. I have one in the actual driver, but can replicate that here if
> it's helpful.

Isn't this a usb.org spec and not a vendor-specific one?

> > > + * Copyright (C) 2024 Code Construct Pty Ltd
> > 
> > It's 2025 now :)
> 
> WHAT?!
> 
> :D
> 
> (this was started in 2024, and I have some preliminary versions up since
> then, but I assume the last date is preferrable)

As per copyright norms, list the real dates, so that would be:
	Copyright (C) 2024-2025 ...

thanks,

greg k-h

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

[Jeremy Kerr]

> 
Hi Greg,

> > Can do. I have one in the actual driver, but can replicate that
> > here if it's helpful.
> 
> Isn't this a usb.org spec and not a vendor-specific one?

Nope, all defined by the DMTF - so not really a vendor, but external to
the USB-IF at least. The only mention of this under USB-IF is the class
code allocation, along with the note:

   [0x14] This base class is defined for devices that conform to the
   “MCTP over USB” found at the DMTF website as DSP0283. This
   specification defines the usable set of SubClass and Protocol
   values. Values outside of this defined spec are reserved. These
   class codes can only be used in Interface Descriptors.

> As per copyright norms, list the real dates, so that would be:
>         Copyright (C) 2024-2025 ...

ack, will do.

Cheers,


Jeremy

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

[Greg Kroah-Hartman]

On Thu, Feb 06, 2025 at 03:36:05PM +0800, Jeremy Kerr wrote:
> > 
> Hi Greg,
> 
> > > Can do. I have one in the actual driver, but can replicate that
> > > here if it's helpful.
> > 
> > Isn't this a usb.org spec and not a vendor-specific one?
> 
> Nope, all defined by the DMTF - so not really a vendor, but external to
> the USB-IF at least. The only mention of this under USB-IF is the class
> code allocation, along with the note:
> 
>    [0x14] This base class is defined for devices that conform to the
>    “MCTP over USB” found at the DMTF website as DSP0283. This
>    specification defines the usable set of SubClass and Protocol
>    values. Values outside of this defined spec are reserved. These
>    class codes can only be used in Interface Descriptors.

Ah, ok, then a link to the DSP0283 spec here in the .h file would be
good.

thanks,

greg k-h

[PATCH net-next 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>
---
 drivers/net/mctp/Kconfig    |  10 ++
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-usb.c | 367 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 378 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..f44e3d418d9544b45cc0369c3c3fa4d6ca11cc29
--- /dev/null
+++ b/drivers/net/mctp/mctp-usb.c
@@ -0,0 +1,367 @@
+// 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 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/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;
+	struct mctp_usb *mctp_usb = netdev_priv(netdev);
+	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:
+		dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
+			__func__, 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)
+{
+	struct sk_buff *skb;
+	int rc;
+
+	skb = netdev_alloc_skb(mctp_usb->netdev, MCTP_USB_XFER_SIZE);
+	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_ATOMIC);
+	if (rc) {
+		dev_err(&mctp_usb->usbdev->dev, "%s: usb_submit_urb: %d\n",
+			__func__, 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:
+		dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
+			__func__, 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) {
+			dev_dbg(&mctp_usb->usbdev->dev, "%s: invalid id %04x\n",
+				__func__, be16_to_cpu(hdr->id));
+			break;
+		}
+
+		if (hdr->len <
+		    sizeof(struct mctp_hdr) + sizeof(struct mctp_usb_hdr)) {
+			dev_dbg(&mctp_usb->usbdev->dev,
+				"%s: short packet (hdr) %d\n",
+				__func__, 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) {
+			dev_dbg(&mctp_usb->usbdev->dev,
+				"%s: short packet (xfer) %d, actual %d\n",
+				__func__, 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);
+		}
+
+		skb->protocol = htons(ETH_P_MCTP);
+		skb_reset_network_header(skb);
+		cb = __mctp_cb(skb);
+		cb->halen = 0;
+		netif_rx(skb);
+
+		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 = skb2;
+	}
+
+	if (skb)
+		kfree_skb(skb);
+
+	mctp_usb_rx_queue(mctp_usb);
+}
+
+static int mctp_usb_open(struct net_device *dev)
+{
+	struct mctp_usb *mctp_usb = netdev_priv(dev);
+
+	return mctp_usb_rx_queue(mctp_usb);
+}
+
+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 = register_netdev(netdev);
+	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);
+
+	unregister_netdev(dev->netdev);
+	usb_free_urb(dev->tx_urb);
+	usb_free_urb(dev->rx_urb);
+	free_netdev(dev->netdev);
+}
+
+static const struct usb_device_id mctp_usb_devices[] = {
+	{ USB_INTERFACE_INFO(USB_CLASS_MCTP, 0x0, 0x1) },
+	{ 0 },
+};
+
+static struct usb_driver mctp_usb_driver = {
+	.name		= "mctp",
+	.id_table	= mctp_usb_devices,
+	.probe		= mctp_usb_probe,
+	.disconnect	= mctp_usb_disconnect,
+};
+
+module_usb_driver(mctp_usb_driver)
+
+MODULE_LICENSE("GPL");

-- 
2.39.5

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

[Greg Kroah-Hartman]

On Thu, Feb 06, 2025 at 02:48:24PM +0800, Jeremy Kerr wrote:
> 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>
> ---
>  drivers/net/mctp/Kconfig    |  10 ++
>  drivers/net/mctp/Makefile   |   1 +
>  drivers/net/mctp/mctp-usb.c | 367 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 378 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..f44e3d418d9544b45cc0369c3c3fa4d6ca11cc29
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-usb.c
> @@ -0,0 +1,367 @@
> +// 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 Code Construct Pty Ltd

It's 2025 :)

> +static void mctp_usb_out_complete(struct urb *urb)
> +{
> +	struct sk_buff *skb = urb->context;
> +	struct net_device *netdev = skb->dev;
> +	struct mctp_usb *mctp_usb = netdev_priv(netdev);
> +	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:
> +		dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> +			__func__, status);

This could flood the logs, are you sure you need it at dev_err() level?

And __func__ is redundant, it's present in dev_*() calls already.

> +static int mctp_usb_rx_queue(struct mctp_usb *mctp_usb)
> +{
> +	struct sk_buff *skb;
> +	int rc;
> +
> +	skb = netdev_alloc_skb(mctp_usb->netdev, MCTP_USB_XFER_SIZE);
> +	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_ATOMIC);
> +	if (rc) {
> +		dev_err(&mctp_usb->usbdev->dev, "%s: usb_submit_urb: %d\n",
> +			__func__, rc);

Again, __func__ is redundant.  Same everywhere else in this file.

thanks,

greg k-h

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

[Jeremy Kerr]

Hi Greg,

Just a check here:

> > +               dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> > +                       __func__, status);
> 
> This could flood the logs, are you sure you need it at dev_err()
> level?
> 
> And __func__ is redundant, it's present in dev_*() calls already.

am I missing something then?

   [  146.130170] usb 2-1: short packet (hdr) 6

emitted from:

    dev_dbg(&mctp_usb->usbdev->dev,
            "short packet (hdr) %d\n",
            hdr->len);

Seems like we get the driver name, but not the function.

I'm happy to remove the __func__ output either way, but I will also
make the logs a little more descriptive for context, if we don't have
func data.

Cheers,


Jeremy

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

[Greg Kroah-Hartman]

On Fri, Feb 07, 2025 at 04:49:05PM +0800, Jeremy Kerr wrote:
> Hi Greg,
> 
> Just a check here:
> 
> > > +               dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> > > +                       __func__, status);
> > 
> > This could flood the logs, are you sure you need it at dev_err()
> > level?
> > 
> > And __func__ is redundant, it's present in dev_*() calls already.
> 
> am I missing something then?
> 
>    [  146.130170] usb 2-1: short packet (hdr) 6
> 
> emitted from:
> 
>     dev_dbg(&mctp_usb->usbdev->dev,
>             "short packet (hdr) %d\n",
>             hdr->len);
> 
> Seems like we get the driver name, but not the function.
> 
> I'm happy to remove the __func__ output either way, but I will also
> make the logs a little more descriptive for context, if we don't have
> func data.

Please read Documentation/admin-guide/dynamic-debug-howto.rst, it shows
how to get the function information from the dev_dbg() lines at runtime.

In short:
	$ alias ddcmd='echo $* > /proc/dynamic_debug/control'
	# add function to all enabled messages
	$ ddcmd '+f'

hope this helps,

greg k-h

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

[Jeremy Kerr]

Hi Greg,

> > > > +               dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> > > > +                       __func__, status);
> > > 
> > > This could flood the logs, are you sure you need it at dev_err()
> > > level?
> > > 
> > > And __func__ is redundant, it's present in dev_*() calls already.
> > 
> > am I missing something then?
> > 
> >    [  146.130170] usb 2-1: short packet (hdr) 6
> > 
> > emitted from:
> > 
> >     dev_dbg(&mctp_usb->usbdev->dev,
> >             "short packet (hdr) %d\n",
> >             hdr->len);
> > 
> > Seems like we get the driver name, but not the function.
> > 
> > I'm happy to remove the __func__ output either way, but I will also
> > make the logs a little more descriptive for context, if we don't have
> > func data.
> 
> Please read Documentation/admin-guide/dynamic-debug-howto.rst, it shows
> how to get the function information from the dev_dbg() lines at runtime.
> 
> In short:
>         $ alias ddcmd='echo $* > /proc/dynamic_debug/control'
>         # add function to all enabled messages
>         $ ddcmd '+f'

Your original comment was on the dev_err() call though (sorry, I've
complicated the discussion by using a dev_dbg() example).

Looks like only dev_dbg (and not _err/_warn/etc) has provision for
__func__, is that right?

I've since removed the __func__ references anyway, and replaced with
better context on the messages, but keen to make sure I have the correct
understanding in general.

Cheers,


Jeremy

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

[Greg Kroah-Hartman]

On Fri, Feb 07, 2025 at 05:45:33PM +0800, Jeremy Kerr wrote:
> Hi Greg,
> 
> > > > > +               dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> > > > > +                       __func__, status);
> > > > 
> > > > This could flood the logs, are you sure you need it at dev_err()
> > > > level?
> > > > 
> > > > And __func__ is redundant, it's present in dev_*() calls already.
> > > 
> > > am I missing something then?
> > > 
> > >    [  146.130170] usb 2-1: short packet (hdr) 6
> > > 
> > > emitted from:
> > > 
> > >     dev_dbg(&mctp_usb->usbdev->dev,
> > >             "short packet (hdr) %d\n",
> > >             hdr->len);
> > > 
> > > Seems like we get the driver name, but not the function.
> > > 
> > > I'm happy to remove the __func__ output either way, but I will also
> > > make the logs a little more descriptive for context, if we don't have
> > > func data.
> > 
> > Please read Documentation/admin-guide/dynamic-debug-howto.rst, it shows
> > how to get the function information from the dev_dbg() lines at runtime.
> > 
> > In short:
> >         $ alias ddcmd='echo $* > /proc/dynamic_debug/control'
> >         # add function to all enabled messages
> >         $ ddcmd '+f'
> 
> Your original comment was on the dev_err() call though (sorry, I've
> complicated the discussion by using a dev_dbg() example).

Sorry, I got confused here too, I saw it on dev_dbg() calls in my
review.

> Looks like only dev_dbg (and not _err/_warn/etc) has provision for
> __func__, is that right?

Yes.

> I've since removed the __func__ references anyway, and replaced with
> better context on the messages, but keen to make sure I have the correct
> understanding in general.

That sounds better, avoiding __func__ wherever possible is usually a
good idea.

thanks,

gre gk-h

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

[Oliver Neukum]

On 06.02.25 07:48, Jeremy Kerr wrote:

Hi,

remarks inline.

	Regards
		Oliver

> +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:
> +		dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> +			__func__, 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) {

It would be more efficient to do the conversion on the constant

> +			dev_dbg(&mctp_usb->usbdev->dev, "%s: invalid id %04x\n",
> +				__func__, be16_to_cpu(hdr->id));
> +			break;
> +		}
> +
> +		if (hdr->len <
> +		    sizeof(struct mctp_hdr) + sizeof(struct mctp_usb_hdr)) {
> +			dev_dbg(&mctp_usb->usbdev->dev,
> +				"%s: short packet (hdr) %d\n",
> +				__func__, 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) {
> +			dev_dbg(&mctp_usb->usbdev->dev,
> +				"%s: short packet (xfer) %d, actual %d\n",
> +				__func__, 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);

This is functional. Though in terms of algorithm you are copying
the same data multiple times.

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

> +}

[..]

> +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));

Taking a reference.
Where is the corresponding put?

> +	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 = register_netdev(netdev);
> +	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;
> +}

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

[Jeremy Kerr]

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

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

[Simon Horman]

On Thu, Feb 06, 2025 at 02:48:24PM +0800, Jeremy Kerr wrote:
> 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>

...

> diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c

...

> +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:
> +		dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n",
> +			__func__, 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) {
> +			dev_dbg(&mctp_usb->usbdev->dev, "%s: invalid id %04x\n",
> +				__func__, be16_to_cpu(hdr->id));
> +			break;
> +		}
> +
> +		if (hdr->len <
> +		    sizeof(struct mctp_hdr) + sizeof(struct mctp_usb_hdr)) {
> +			dev_dbg(&mctp_usb->usbdev->dev,
> +				"%s: short packet (hdr) %d\n",
> +				__func__, 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) {
> +			dev_dbg(&mctp_usb->usbdev->dev,
> +				"%s: short packet (xfer) %d, actual %d\n",
> +				__func__, 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);
> +		}
> +
> +		skb->protocol = htons(ETH_P_MCTP);
> +		skb_reset_network_header(skb);
> +		cb = __mctp_cb(skb);
> +		cb->halen = 0;
> +		netif_rx(skb);

Hi Jeremy,

skb is dereferenced a few lines further down,
but I don't think it is is safe to do so after calling netif_rx().

> +
> +		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 = skb2;
> +	}
> +
> +	if (skb)
> +		kfree_skb(skb);
> +
> +	mctp_usb_rx_queue(mctp_usb);
> +}

...

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

[Jeremy Kerr]

Hi Horms,

> > +               skb->protocol = htons(ETH_P_MCTP);
> > +               skb_reset_network_header(skb);
> > +               cb = __mctp_cb(skb);
> > +               cb->halen = 0;
> > +               netif_rx(skb);
> 
> Hi Jeremy,
> 
> skb is dereferenced a few lines further down,
> but I don't think it is is safe to do so after calling netif_rx().

Yep, neither do I. I have moved the rx accounting prior to the
netif_rx() call for v2.

Thanks for the check!

Cheers,


Jeremy

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

[Jeff Johnson]

On 2/5/25 22:48, Jeremy Kerr wrote:
...
> +module_usb_driver(mctp_usb_driver)
> +
> +MODULE_LICENSE("GPL");
> 

Since commit 1fffe7a34c89 ("script: modpost: emit a warning when the
description is missing"), a module without a MODULE_DESCRIPTION() will
result in a warning with make W=1. Please add a MODULE_DESCRIPTION()
to avoid this warning.