[PATCH net-next] mctp: Add MCTP-over-serial transport binding

[PATCH net-next] mctp: Add MCTP-over-serial transport binding

[Jeremy Kerr]

This change adds a MCTP Serial transport binding, as per DMTF DSP0253.
This is implemented as a new serial line discipline, and can be attached
to arbitrary serial devices.

The 'mctp' utility provides the ldisc magic to set up the serial link:

  # mctp link serial /dev/ttyS0 &
  # mctp link
  dev lo index 1 address 0x00:00:00:00:00:00 net 1 mtu 65536 up
  dev mctpserial0 index 5 address 0x(no-addr) net 1 mtu 68 down

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/net/mctp/Kconfig       |  11 +
 drivers/net/mctp/Makefile      |   1 +
 drivers/net/mctp/mctp-serial.c | 494 +++++++++++++++++++++++++++++++++
 include/uapi/linux/tty.h       |   1 +
 4 files changed, 507 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-serial.c

diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index d8f966cedc89..02f3c2d600fd 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -3,6 +3,17 @@ if MCTP
 
 menu "MCTP Device Drivers"
 
+config MCTP_SERIAL
+	tristate "MCTP serial transport"
+	depends on TTY
+	select CRC_CCITT
+	help
+	  This driver provides an MCTP-over-serial interface, through a
+	  serial line-discipline. By attaching the ldisc to a serial device,
+	  we get a new net device to transport MCTP packets.
+
+	  Say y here if you need to connect to MCTP devices over serial.
+
 endmenu
 
 endif
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index e69de29bb2d1..d32622613ce4 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
new file mode 100644
index 000000000000..30950f1ea6f4
--- /dev/null
+++ b/drivers/net/mctp/mctp-serial.c
@@ -0,0 +1,494 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Management Component Transport Protocol (MCTP) - serial transport
+ * binding.
+ *
+ * Copyright (c) 2021 Code Construct
+ */
+
+#include <linux/idr.h>
+#include <linux/if_arp.h>
+#include <linux/module.h>
+#include <linux/skbuff.h>
+#include <linux/tty.h>
+#include <linux/workqueue.h>
+#include <linux/crc-ccitt.h>
+
+#include <linux/mctp.h>
+#include <net/mctp.h>
+#include <net/pkt_sched.h>
+
+#define MCTP_SERIAL_MTU		68 /* base mtu (64) + mctp header */
+#define MCTP_SERIAL_FRAME_MTU	(MCTP_SERIAL_MTU + 6) /* + serial framing */
+
+#define MCTP_SERIAL_VERSION	0x1
+
+#define BUFSIZE			MCTP_SERIAL_FRAME_MTU
+
+#define BYTE_FRAME		0x7e
+#define BYTE_ESC		0x7d
+
+static DEFINE_IDA(mctp_serial_ida);
+
+enum mctp_serial_state {
+	STATE_IDLE,
+	STATE_START,
+	STATE_HEADER,
+	STATE_DATA,
+	STATE_ESCAPE,
+	STATE_TRAILER,
+	STATE_DONE,
+	STATE_ERR,
+};
+
+struct mctp_serial {
+	struct net_device	*netdev;
+	struct tty_struct	*tty;
+
+	int			idx;
+
+	/* protects our rx & tx state machines; held during both paths */
+	spinlock_t		lock;
+
+	struct work_struct	tx_work;
+	enum mctp_serial_state	txstate, rxstate;
+	u16			txfcs, rxfcs, rxfcs_rcvd;
+	unsigned int		txlen, rxlen;
+	unsigned int		txpos, rxpos;
+	unsigned char		txbuf[BUFSIZE],
+				rxbuf[BUFSIZE];
+};
+
+static bool needs_escape(unsigned char c)
+{
+	return c == BYTE_ESC || c == BYTE_FRAME;
+}
+
+static int next_chunk_len(struct mctp_serial *dev)
+{
+	int i;
+
+	/* either we have no bytes to send ... */
+	if (dev->txpos == dev->txlen)
+		return 0;
+
+	/* ... or the next byte to send is an escaped byte; requiring a
+	 * single-byte chunk...
+	 */
+	if (needs_escape(dev->txbuf[dev->txpos]))
+		return 1;
+
+	/* ... or we have one or more bytes up to the next escape - this chunk
+	 * will be those non-escaped bytes, and does not include the escaped
+	 * byte.
+	 */
+	for (i = 1; i + dev->txpos + 1 < dev->txlen; i++) {
+		if (needs_escape(dev->txbuf[dev->txpos + i + 1]))
+			break;
+	}
+
+	return i;
+}
+
+static int write_chunk(struct mctp_serial *dev, unsigned char *buf, int len)
+{
+	return dev->tty->ops->write(dev->tty, buf, len);
+}
+
+static void mctp_serial_tx_work(struct work_struct *work)
+{
+	struct mctp_serial *dev = container_of(work, struct mctp_serial,
+					       tx_work);
+	unsigned char c, buf[3];
+	unsigned long flags;
+	int len, txlen;
+
+	spin_lock_irqsave(&dev->lock, flags);
+
+	/* txstate represents the next thing to send */
+	switch (dev->txstate) {
+	case STATE_START:
+		dev->txpos = 0;
+		fallthrough;
+	case STATE_HEADER:
+		buf[0] = BYTE_FRAME;
+		buf[1] = MCTP_SERIAL_VERSION;
+		buf[2] = dev->txlen;
+
+		if (!dev->txpos)
+			dev->txfcs = crc_ccitt(0, buf + 1, 2);
+
+		txlen = write_chunk(dev, buf + dev->txpos, 3 - dev->txpos);
+		if (txlen <= 0) {
+			dev->txstate = STATE_ERR;
+		} else {
+			dev->txpos += txlen;
+			if (dev->txpos == 3) {
+				dev->txstate = STATE_DATA;
+				dev->txpos = 0;
+			}
+		}
+		break;
+
+	case STATE_ESCAPE:
+		buf[0] = dev->txbuf[dev->txpos] & ~0x20;
+		txlen = write_chunk(dev, buf, 1);
+		dev->txpos += txlen;
+		if (dev->txpos == dev->txlen) {
+			dev->txstate = STATE_TRAILER;
+			dev->txpos = 0;
+		}
+
+		break;
+
+	case STATE_DATA:
+		len = next_chunk_len(dev);
+		if (len) {
+			c = dev->txbuf[dev->txpos];
+			if (len == 1 && needs_escape(c)) {
+				buf[0] = BYTE_ESC;
+				buf[1] = c & ~0x20;
+				dev->txfcs = crc_ccitt_byte(dev->txfcs, c);
+				txlen = write_chunk(dev, buf, 2);
+				if (txlen == 2)
+					dev->txpos++;
+				else if (txlen == 1)
+					dev->txstate = STATE_ESCAPE;
+			} else {
+				txlen = write_chunk(dev,
+						    dev->txbuf + dev->txpos,
+						    len);
+				dev->txfcs = crc_ccitt(dev->txfcs,
+						       dev->txbuf + dev->txpos,
+						       txlen);
+				dev->txpos += txlen;
+			}
+			if (dev->txstate == STATE_DATA &&
+			    dev->txpos == dev->txlen) {
+				dev->txstate = STATE_TRAILER;
+				dev->txpos = 0;
+			}
+			break;
+		}
+		dev->txstate = STATE_TRAILER;
+		fallthrough;
+
+	case STATE_TRAILER:
+		buf[0] = dev->txfcs >> 8;
+		buf[1] = dev->txfcs & 0xff;
+		buf[2] = BYTE_FRAME;
+		txlen = write_chunk(dev, buf + dev->txpos, 3 - dev->txpos);
+		if (txlen <= 0) {
+			dev->txstate = STATE_ERR;
+		} else {
+			dev->txpos += txlen;
+			if (dev->txpos == 3) {
+				dev->txstate = STATE_DONE;
+				dev->txpos = 0;
+			}
+		}
+		break;
+	default:
+		netdev_err_once(dev->netdev, "invalid tx state %d\n",
+				dev->txstate);
+	}
+
+	if (dev->txstate == STATE_DONE) {
+		dev->netdev->stats.tx_packets++;
+		dev->netdev->stats.tx_bytes += dev->txlen;
+		dev->txlen = 0;
+		dev->txpos = 0;
+		clear_bit(TTY_DO_WRITE_WAKEUP, &dev->tty->flags);
+		dev->txstate = STATE_IDLE;
+		spin_unlock_irqrestore(&dev->lock, flags);
+
+		netif_wake_queue(dev->netdev);
+	} else {
+		spin_unlock_irqrestore(&dev->lock, flags);
+	}
+}
+
+static netdev_tx_t mctp_serial_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mctp_serial *dev = netdev_priv(ndev);
+	unsigned long flags;
+
+	WARN_ON(dev->txstate != STATE_IDLE);
+
+	if (skb->len > MCTP_SERIAL_MTU) {
+		dev->netdev->stats.tx_dropped++;
+		goto out;
+	}
+
+	spin_lock_irqsave(&dev->lock, flags);
+	netif_stop_queue(dev->netdev);
+	skb_copy_bits(skb, 0, dev->txbuf, skb->len);
+	dev->txpos = 0;
+	dev->txlen = skb->len;
+	dev->txstate = STATE_START;
+	spin_unlock_irqrestore(&dev->lock, flags);
+
+	set_bit(TTY_DO_WRITE_WAKEUP, &dev->tty->flags);
+	schedule_work(&dev->tx_work);
+
+out:
+	kfree_skb(skb);
+	return NETDEV_TX_OK;
+}
+
+static void mctp_serial_tty_write_wakeup(struct tty_struct *tty)
+{
+	struct mctp_serial *dev;
+
+	rcu_read_lock();
+	dev = rcu_dereference(tty->disc_data);
+	schedule_work(&dev->tx_work);
+	rcu_read_unlock();
+}
+
+static void mctp_serial_rx(struct mctp_serial *dev)
+{
+	struct mctp_skb_cb *cb;
+	struct sk_buff *skb;
+
+	if (dev->rxfcs != dev->rxfcs_rcvd) {
+		dev->netdev->stats.rx_dropped++;
+		dev->netdev->stats.rx_crc_errors++;
+		return;
+	}
+
+	skb = netdev_alloc_skb(dev->netdev, dev->rxlen);
+	if (!skb) {
+		dev->netdev->stats.rx_dropped++;
+		return;
+	}
+
+	skb->protocol = htons(ETH_P_MCTP);
+	skb_put_data(skb, dev->rxbuf, dev->rxlen);
+	skb_reset_network_header(skb);
+
+	cb = __mctp_cb(skb);
+	cb->halen = 0;
+
+	netif_rx_ni(skb);
+	dev->netdev->stats.rx_packets++;
+	dev->netdev->stats.rx_bytes += dev->rxlen;
+}
+
+static void mctp_serial_push_header(struct mctp_serial *dev, unsigned char c)
+{
+	switch (dev->rxpos) {
+	case 0:
+		if (c == BYTE_FRAME)
+			dev->rxpos++;
+		else
+			dev->rxstate = STATE_ERR;
+		break;
+	case 1:
+		if (c == MCTP_SERIAL_VERSION) {
+			dev->rxpos++;
+			dev->rxfcs = crc_ccitt_byte(0, c);
+		} else {
+			dev->rxstate = STATE_ERR;
+		}
+		break;
+	case 2:
+		if (c > MCTP_SERIAL_FRAME_MTU) {
+			dev->rxstate = STATE_ERR;
+		} else {
+			dev->rxlen = c;
+			dev->rxpos = 0;
+			dev->rxstate = STATE_DATA;
+			dev->rxfcs = crc_ccitt_byte(dev->rxfcs, c);
+		}
+		break;
+	}
+}
+
+static void mctp_serial_push_trailer(struct mctp_serial *dev, unsigned char c)
+{
+	switch (dev->rxpos) {
+	case 0:
+		dev->rxfcs_rcvd = c << 8;
+		dev->rxpos++;
+		break;
+	case 1:
+		dev->rxfcs_rcvd |= c;
+		dev->rxpos++;
+		break;
+	case 2:
+		if (c != BYTE_FRAME) {
+			dev->rxstate = STATE_ERR;
+		} else {
+			mctp_serial_rx(dev);
+			dev->rxlen = 0;
+			dev->rxpos = 0;
+			dev->rxstate = STATE_IDLE;
+		}
+		break;
+	}
+}
+
+static void mctp_serial_push(struct mctp_serial *dev, unsigned char c)
+{
+	switch (dev->rxstate) {
+	case STATE_IDLE:
+		dev->rxstate = STATE_HEADER;
+		fallthrough;
+	case STATE_HEADER:
+		mctp_serial_push_header(dev, c);
+		break;
+
+	case STATE_ESCAPE:
+		c |= 0x20;
+		fallthrough;
+	case STATE_DATA:
+		if (dev->rxstate != STATE_ESCAPE && c == BYTE_ESC) {
+			dev->rxstate = STATE_ESCAPE;
+		} else {
+			dev->rxfcs = crc_ccitt_byte(dev->rxfcs, c);
+			dev->rxbuf[dev->rxpos] = c;
+			dev->rxpos++;
+			dev->rxstate = STATE_DATA;
+			if (dev->rxpos == dev->rxlen) {
+				dev->rxpos = 0;
+				dev->rxstate = STATE_TRAILER;
+			}
+		}
+		break;
+
+	case STATE_TRAILER:
+		mctp_serial_push_trailer(dev, c);
+		break;
+
+	case STATE_ERR:
+		if (c == BYTE_FRAME)
+			dev->rxstate = STATE_IDLE;
+		break;
+
+	default:
+		netdev_err_once(dev->netdev, "invalid rx state %d\n",
+				dev->rxstate);
+	}
+}
+
+static void mctp_serial_tty_receive_buf(struct tty_struct *tty,
+					const unsigned char *c,
+					const char *f, int len)
+{
+	struct mctp_serial *dev = tty->disc_data;
+	int i;
+
+	if (!netif_running(dev->netdev))
+		return;
+
+	/* we don't (currently) use the flag bytes, just data. */
+	for (i = 0; i < len; i++)
+		mctp_serial_push(dev, c[i]);
+}
+
+static const struct net_device_ops mctp_serial_netdev_ops = {
+	.ndo_start_xmit = mctp_serial_tx,
+};
+
+static void mctp_serial_setup(struct net_device *ndev)
+{
+	ndev->type = ARPHRD_MCTP;
+	ndev->mtu = MCTP_SERIAL_MTU;
+	ndev->hard_header_len = 0;
+	ndev->addr_len = 0;
+	ndev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
+	ndev->flags = IFF_NOARP;
+	ndev->netdev_ops = &mctp_serial_netdev_ops;
+	ndev->needs_free_netdev = true;
+}
+
+static int mctp_serial_open(struct tty_struct *tty)
+{
+	struct mctp_serial *dev;
+	struct net_device *ndev;
+	char name[32];
+	int idx, rc;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (!tty->ops->write)
+		return -EOPNOTSUPP;
+
+	if (tty->disc_data)
+		return -EEXIST;
+
+	idx = ida_alloc(&mctp_serial_ida, GFP_KERNEL);
+	if (idx < 0)
+		return idx;
+
+	snprintf(name, sizeof(name), "mctpserial%d", idx);
+	ndev = alloc_netdev(sizeof(*dev), name, NET_NAME_ENUM,
+			    mctp_serial_setup);
+	if (!ndev) {
+		rc = -ENOMEM;
+		goto free_ida;
+	}
+
+	dev = netdev_priv(ndev);
+	dev->idx = idx;
+	dev->tty = tty;
+	dev->netdev = ndev;
+	dev->txstate = STATE_IDLE;
+	dev->rxstate = STATE_IDLE;
+	spin_lock_init(&dev->lock);
+	INIT_WORK(&dev->tx_work, mctp_serial_tx_work);
+
+	rc = register_netdev(ndev);
+	if (rc)
+		goto free_netdev;
+
+	tty->receive_room = 64 * 1024;
+	tty->disc_data = dev;
+
+	return 0;
+
+free_netdev:
+	free_netdev(ndev);
+
+free_ida:
+	ida_free(&mctp_serial_ida, idx);
+	return rc;
+}
+
+static void mctp_serial_close(struct tty_struct *tty)
+{
+	struct mctp_serial *dev = tty->disc_data;
+	int idx = dev->idx;
+
+	unregister_netdev(dev->netdev);
+	ida_free(&mctp_serial_ida, idx);
+}
+
+static struct tty_ldisc_ops mctp_ldisc = {
+	.owner		= THIS_MODULE,
+	.num		= N_MCTP,
+	.name		= "mctp",
+	.open		= mctp_serial_open,
+	.close		= mctp_serial_close,
+	.receive_buf	= mctp_serial_tty_receive_buf,
+	.write_wakeup	= mctp_serial_tty_write_wakeup,
+};
+
+static int __init mctp_serial_init(void)
+{
+	return tty_register_ldisc(&mctp_ldisc);
+}
+
+static void __exit mctp_serial_exit(void)
+{
+	tty_unregister_ldisc(&mctp_ldisc);
+}
+
+module_init(mctp_serial_init);
+module_exit(mctp_serial_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Jeremy Kerr <jk@codeconstruct.com.au>");
+MODULE_DESCRIPTION("MCTP Serial transport");
diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
index 376cccf397be..a58deb3061eb 100644
--- a/include/uapi/linux/tty.h
+++ b/include/uapi/linux/tty.h
@@ -38,5 +38,6 @@
 #define N_NCI		25	/* NFC NCI UART */
 #define N_SPEAKUP	26	/* Speakup communication with synths */
 #define N_NULL		27	/* Null ldisc used for error handling */
+#define N_MCTP		28	/* MCTP-over-serial */
 
 #endif /* _UAPI_LINUX_TTY_H */
-- 
2.30.2

Re: [PATCH net-next] mctp: Add MCTP-over-serial transport binding

[Greg Kroah-Hartman]

On Mon, Nov 22, 2021 at 12:28:17PM +0800, Jeremy Kerr wrote:
> This change adds a MCTP Serial transport binding, as per DMTF DSP0253.

What is "DMTF DSP0253"?  Can you provide a link to this or more
information that explains why this has to be a serial thing?

> This is implemented as a new serial line discipline, and can be attached
> to arbitrary serial devices.

Why?  Who is going to use this?

> The 'mctp' utility provides the ldisc magic to set up the serial link:
> 
>   # mctp link serial /dev/ttyS0 &
>   # mctp link
>   dev lo index 1 address 0x00:00:00:00:00:00 net 1 mtu 65536 up
>   dev mctpserial0 index 5 address 0x(no-addr) net 1 mtu 68 down

Where is this magic mctp application?  I can't find it in my distro
packages anywhere.


> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
>  drivers/net/mctp/Kconfig       |  11 +
>  drivers/net/mctp/Makefile      |   1 +
>  drivers/net/mctp/mctp-serial.c | 494 +++++++++++++++++++++++++++++++++
>  include/uapi/linux/tty.h       |   1 +
>  4 files changed, 507 insertions(+)
>  create mode 100644 drivers/net/mctp/mctp-serial.c
> 
> diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
> index d8f966cedc89..02f3c2d600fd 100644
> --- a/drivers/net/mctp/Kconfig
> +++ b/drivers/net/mctp/Kconfig
> @@ -3,6 +3,17 @@ if MCTP
>  
>  menu "MCTP Device Drivers"
>  
> +config MCTP_SERIAL
> +	tristate "MCTP serial transport"
> +	depends on TTY
> +	select CRC_CCITT
> +	help
> +	  This driver provides an MCTP-over-serial interface, through a
> +	  serial line-discipline. By attaching the ldisc to a serial device,
> +	  we get a new net device to transport MCTP packets.
> +
> +	  Say y here if you need to connect to MCTP devices over serial.

Module name?

> +
>  endmenu
>  
>  endif
> diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
> index e69de29bb2d1..d32622613ce4 100644
> --- a/drivers/net/mctp/Makefile
> +++ b/drivers/net/mctp/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
> diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
> new file mode 100644
> index 000000000000..30950f1ea6f4
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -0,0 +1,494 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Management Component Transport Protocol (MCTP) - serial transport
> + * binding.
> + *
> + * Copyright (c) 2021 Code Construct
> + */
> +
> +#include <linux/idr.h>
> +#include <linux/if_arp.h>
> +#include <linux/module.h>
> +#include <linux/skbuff.h>
> +#include <linux/tty.h>
> +#include <linux/workqueue.h>
> +#include <linux/crc-ccitt.h>
> +
> +#include <linux/mctp.h>
> +#include <net/mctp.h>
> +#include <net/pkt_sched.h>
> +
> +#define MCTP_SERIAL_MTU		68 /* base mtu (64) + mctp header */
> +#define MCTP_SERIAL_FRAME_MTU	(MCTP_SERIAL_MTU + 6) /* + serial framing */
> +
> +#define MCTP_SERIAL_VERSION	0x1

Where does this number come from?

> +
> +#define BUFSIZE			MCTP_SERIAL_FRAME_MTU
> +
> +#define BYTE_FRAME		0x7e
> +#define BYTE_ESC		0x7d
> +
> +static DEFINE_IDA(mctp_serial_ida);

I think you forgot to clean this up when the module is removed.

thanks,

greg k-h

Re: [PATCH net-next] mctp: Add MCTP-over-serial transport binding

[Jeremy Kerr]

Hi Greg,

Thanks for the review, I'll get a v2 done. Some replies inline.

> > This change adds a MCTP Serial transport binding, as per DMTF
> > DSP0253.
> 
> What is "DMTF DSP0253"?  Can you provide a link to this or more
> information that explains why this has to be a serial thing?

Sure, can do!

[it doesn't *have* to be a serial thing - MCTP supports multiple
physical layers as "transports" - current specs define transports for
serial, i2c and PCIe. The choice of transport will be dictated by
however you've connected your remote MCTP device(s). In this case
though, it's also handy for emulation, where we can transport MCTP
packets between virtual machines by connecting VMs' pty channels]

> > The 'mctp' utility provides the ldisc magic to set up the serial
> > link:
> > 
> >   # mctp link serial /dev/ttyS0 &
> >   # mctp link
> >   dev lo index 1 address 0x00:00:00:00:00:00 net 1 mtu 65536 up
> >   dev mctpserial0 index 5 address 0x(no-addr) net 1 mtu 68 down
> 
> Where is this magic mctp application?  I can't find it in my distro
> packages anywhere.

The MCTP support is pretty new, and possibly a bit eclectic for general
distro inclusion at this stage. I'll include a ref to the tools.

> > +         Say y here if you need to connect to MCTP devices over serial.
> 
> Module name?

Ack.

> > +#define MCTP_SERIAL_VERSION    0x1
> 
> Where does this number come from?

Defined by the current spec; I'll add a comment.

> > +static DEFINE_IDA(mctp_serial_ida);
> 
> I think you forgot to clean this up when the module is removed.

Would it be possible to have the module exit called while we still have
ida bitmaps still allocated? It looks like a ldisc being open will
require a reference on the module; so a module remove will mean we have
no ldiscs in use, and therefore an empty ida, so the ida_destroy() will
always be a no-op.

Is there a path I'm missing here? Or is this more of a completeness
thing?

Cheers,


Jeremy

Re: [PATCH net-next] mctp: Add MCTP-over-serial transport binding

[Greg Kroah-Hartman]

On Mon, Nov 22, 2021 at 03:16:55PM +0800, Jeremy Kerr wrote:
> > > +static DEFINE_IDA(mctp_serial_ida);
> > 
> > I think you forgot to clean this up when the module is removed.
> 
> Would it be possible to have the module exit called while we still have
> ida bitmaps still allocated? It looks like a ldisc being open will
> require a reference on the module; so a module remove will mean we have
> no ldiscs in use, and therefore an empty ida, so the ida_destroy() will
> always be a no-op.

ida_destroy() will not be a no-op if you have allocated some things in
the past.  It should always be called when your module is removed.

Or at least that is how it used to be, if this has changed in the past
year, then I am mistaken here.

thanks,

greg k-h

Re: [PATCH net-next] mctp: Add MCTP-over-serial transport binding

[Jeremy Kerr]

Hi Greg,

> ida_destroy() will not be a no-op if you have allocated some things
> in the past.  It should always be called when your module is removed.
> 
> Or at least that is how it used to be, if this has changed in the
> past year, then I am mistaken here.

I was going by this bit of the comment on ida_destroy:

   * Calling this function frees all IDs and releases all resources used
   * by an IDA.  When this call returns, the IDA is empty and can be reused
   * or freed.  If the IDA is already empty, there is no need to call this
   * function.

[From a documentation improvement in 50d97d50715]

Looking at ida_destroy, it's iterating the xarray and freeing all !value
entries. ida_free will free a (allocated) value entry once all bits are
clear, so the comment looks correct to me - there's nothing left to free
if the ida is empty.

However, I'm definitely no ida/idr/xarray expert! Happy to be corrected
here - and I'll send a patch to clarify that comment too, if so.

Cheers,


Jeremy

Re: [PATCH net-next] mctp: Add MCTP-over-serial transport binding

[Matthew Wilcox]

On Mon, Nov 22, 2021 at 04:23:10PM +0800, Jeremy Kerr wrote:
> Hi Greg,
> 
> > ida_destroy() will not be a no-op if you have allocated some things
> > in the past.  It should always be called when your module is removed.
> > 
> > Or at least that is how it used to be, if this has changed in the
> > past year, then I am mistaken here.

I think Greg is remembering how the IDA behaved before it was converted
to use the radix tree back in 2016 (0a835c4f090a).  About two-thirds
of the users of the IDA and IDR forgot to call ida_destroy/idr_destroy,
so rather than fix those places, I decided to make those data structures
no longer require a destructor.

> I was going by this bit of the comment on ida_destroy:
> 
>    * Calling this function frees all IDs and releases all resources used
>    * by an IDA.  When this call returns, the IDA is empty and can be reused
>    * or freed.  If the IDA is already empty, there is no need to call this
>    * function.
> 
> [From a documentation improvement in 50d97d50715]
> 
> Looking at ida_destroy, it's iterating the xarray and freeing all !value
> entries. ida_free will free a (allocated) value entry once all bits are
> clear, so the comment looks correct to me - there's nothing left to free
> if the ida is empty.
> 
> However, I'm definitely no ida/idr/xarray expert! Happy to be corrected
> here - and I'll send a patch to clarify that comment too, if so.
> 
> Cheers,
> 
> 
> Jeremy
> 

Re: [PATCH net-next] mctp: Add MCTP-over-serial transport binding

[Greg Kroah-Hartman]

On Mon, Nov 22, 2021 at 01:11:52PM +0000, Matthew Wilcox wrote:
> On Mon, Nov 22, 2021 at 04:23:10PM +0800, Jeremy Kerr wrote:
> > Hi Greg,
> > 
> > > ida_destroy() will not be a no-op if you have allocated some things
> > > in the past.  It should always be called when your module is removed.
> > > 
> > > Or at least that is how it used to be, if this has changed in the
> > > past year, then I am mistaken here.
> 
> I think Greg is remembering how the IDA behaved before it was converted
> to use the radix tree back in 2016 (0a835c4f090a).  About two-thirds
> of the users of the IDA and IDR forgot to call ida_destroy/idr_destroy,
> so rather than fix those places, I decided to make those data structures
> no longer require a destructor.

Ah, yes.  Glad to see that's no longer needed, sorry for the noise and
thanks for correcting me.

greg k-h