[RFC PATCH 0/2] I3C MCTP net driver

[RFC PATCH 0/2] I3C MCTP net driver

[Matt Johnston]

This series adds an I3C transport for the kernel's MCTP network
protocol. MCTP is a communication protocol between system components
(BMCs, drives, NICs etc), with higher level protocols such as NVMe-MI or
PLDM built on top of it (in userspace). It runs over various transports
such as I2C, PCIe, or I3C.

The mctp-i3c driver follows a similar approach to the kernel's existing
mctp-i2c driver, creating a "mctpi3cX" network interface for each
numbered I3C bus. Busses opt in to support by adding a "mctp-controller"
property to the devicetree:

&i3c0 {
        mctp-controller;
}

The driver will bind to MCTP class devices (DCR 0xCC) that are on a
supported I3C bus. Each bus is represented by a `struct mctp_i3c_bus`
that keeps state for the network device. An individual I3C device
(struct mctp_i3c_device) performs operations using the "parent"
mctp_i3c_bus object. The I3C notify/enumeration patch is needed so that
the mctp-i3c driver can handle creating/removing mctp_i3c_bus objects as
required.

The mctp-i3c driver is using the Provisioned ID as an identifier for
target I3C devices (the neighbour address), as that will be more stable
than the I3C dynamic address. The driver internally translates that to a
dynamic address for bus operations.

The driver has been tested using an AST2600 platform, with Jeremy's
In-Band Interrupt support patches. A remote endpoint has been tested
against Qemu, as well as using the target mode support in Aspeed's
vendor tree.

I'm sending this as an RFC for linux-i3c, then will submit the mctp-i3c
driver itself to the netdev list after it has had review here.

Cheers,
Matt

Jeremy Kerr (1):
  i3c: Add support for bus enumeration & notification

Matt Johnston (1):
  mctp i3c: MCTP I3C driver

 drivers/i3c/master.c        |  35 ++
 drivers/net/mctp/Kconfig    |   9 +
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-i3c.c | 778 ++++++++++++++++++++++++++++++++++++
 include/linux/i3c/master.h  |  11 +
 5 files changed, 834 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-i3c.c

-- 
2.37.2


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC PATCH 1/2] i3c: Add support for bus enumeration & notification

[Matt Johnston]

From: Jeremy Kerr <jk@codeconstruct.com.au>

This allows other drivers to be notified when new i3c busses are
attached, referring to a whole i3c bus as opposed to individual
devices.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/i3c/master.c       | 35 +++++++++++++++++++++++++++++++++++
 include/linux/i3c/master.h | 11 +++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 54e4c34b4a22..a2071e6f0ec9 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -21,6 +21,7 @@
 
 static DEFINE_IDR(i3c_bus_idr);
 static DEFINE_MUTEX(i3c_core_lock);
+static BLOCKING_NOTIFIER_HEAD(i3c_bus_notifier);
 
 /**
  * i3c_bus_maintenance_lock - Lock the bus for a maintenance operation
@@ -441,6 +442,36 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
 	return 0;
 }
 
+void i3c_for_each_bus_locked(int (*fn)(struct i3c_bus *bus, void *data),
+			     void *data)
+{
+	struct i3c_bus *bus;
+	int id;
+
+	mutex_lock(&i3c_core_lock);
+	idr_for_each_entry(&i3c_bus_idr, bus, id)
+		fn(bus, data);
+	mutex_unlock(&i3c_core_lock);
+}
+EXPORT_SYMBOL_GPL(i3c_for_each_bus_locked);
+
+int i3c_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&i3c_bus_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(i3c_register_notifier);
+
+int i3c_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&i3c_bus_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(i3c_unregister_notifier);
+
+static void i3c_bus_notify(struct i3c_bus *bus, unsigned int action)
+{
+	blocking_notifier_call_chain(&i3c_bus_notifier, action, bus);
+}
+
 static const char * const i3c_bus_mode_strings[] = {
 	[I3C_BUS_MODE_PURE] = "pure",
 	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
@@ -2666,6 +2697,8 @@ int i3c_master_register(struct i3c_master_controller *master,
 	if (ret)
 		goto err_del_dev;
 
+	i3c_bus_notify(i3cbus, I3C_NOTIFY_BUS_ADD);
+
 	/*
 	 * We're done initializing the bus and the controller, we can now
 	 * register I3C devices discovered during the initial DAA.
@@ -2700,6 +2733,8 @@ EXPORT_SYMBOL_GPL(i3c_master_register);
  */
 int i3c_master_unregister(struct i3c_master_controller *master)
 {
+	i3c_bus_notify(&master->bus, I3C_NOTIFY_BUS_REMOVE);
+
 	i3c_master_i2c_adapter_cleanup(master);
 	i3c_master_unregister_i3c_devs(master);
 	i3c_master_bus_cleanup(master);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 604a126b78c8..8153c2bd99e1 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -22,6 +22,12 @@
 #define I3C_BROADCAST_ADDR		0x7e
 #define I3C_MAX_ADDR			GENMASK(6, 0)
 
+/* notifier actions. notifier call data is the struct i3c_bus */
+enum {
+	I3C_NOTIFY_BUS_ADD,
+	I3C_NOTIFY_BUS_REMOVE,
+};
+
 struct i3c_master_controller;
 struct i3c_bus;
 struct i2c_device;
@@ -651,4 +657,9 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
 
 struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
 
+void i3c_for_each_bus_locked(int (*fn)(struct i3c_bus *bus, void *data),
+			     void *data);
+int i3c_register_notifier(struct notifier_block *nb);
+int i3c_unregister_notifier(struct notifier_block *nb);
+
 #endif /* I3C_MASTER_H */
-- 
2.37.2


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC PATCH 2/2] mctp i3c: MCTP I3C driver

[Matt Johnston]

Provides MCTP network transport over an I3C bus, as specified in
DMTF DSP0233.

Each I3C bus (with "mctp-controller" devicetree property) gets an
"mctpi3cX" net device created. I3C devices are reachable as remote
endpoints through that net device. Link layer addressing uses the
I3C PID as a fixed hardware address for neighbour table entries.

The driver matches I3C devices that have the MIPI assigned DCR 0xCC for
MCTP.

Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 drivers/net/mctp/Kconfig    |   9 +
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-i3c.c | 778 ++++++++++++++++++++++++++++++++++++
 3 files changed, 788 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-i3c.c

diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index dc71657d9184..ce9d2d2ccf3b 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -33,6 +33,15 @@ config MCTP_TRANSPORT_I2C
 	  from DMTF specification DSP0237. A MCTP protocol network device is
 	  created for each I2C bus that has been assigned a mctp-i2c device.
 
+config MCTP_TRANSPORT_I3C
+	tristate "MCTP I3C transport"
+	depends on I3C
+	help
+	  Provides a driver to access MCTP devices over I3C transport,
+	  from DMTF specification DSP0233.
+	  A MCTP protocol network device is created for each I3C bus
+	  having a "mctp-controller" devicetree property.
+
 endmenu
 
 endif
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index 1ca3e6028f77..e1cb99ced54a 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
 obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
+obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
diff --git a/drivers/net/mctp/mctp-i3c.c b/drivers/net/mctp/mctp-i3c.c
new file mode 100644
index 000000000000..29157be57aa5
--- /dev/null
+++ b/drivers/net/mctp/mctp-i3c.c
@@ -0,0 +1,778 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Implements DMTF specification
+ * "DSP0232 Management Component Transport Protocol (MCTP) I3C Transport
+ * Binding"
+ * https://www.dmtf.org/sites/default/files/standards/documents/DSP0233_1.0.0.pdf
+ *
+ * Copyright (c) 2023 Code Construct
+ */
+
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/if_arp.h>
+#include <net/mctp.h>
+#include <net/mctpdevice.h>
+
+#define MCTP_I3C_MAXBUF 65536
+/* 48 bit Provisioned Id */
+#define PID_SIZE 6
+
+/* 64 byte payload, 4 byte MCTP header */
+static const int MCTP_I3C_MINMTU = 64 + 4;
+/* One byte less to allow for the PEC */
+static const int MCTP_I3C_MAXMTU = MCTP_I3C_MAXBUF - 1;
+/* 4 byte MCTP header, no data, 1 byte PEC */
+static const int MCTP_I3C_MINLEN = 4 + 1;
+
+/* Sufficient for 64kB at min mtu */
+static const int MCTP_I3C_TX_QUEUE_LEN = 1100;
+
+/* Somewhat arbitrary */
+static const int MCTP_I3C_IBI_SLOTS = 8;
+
+/* Mandatory Data Byte in an IBI, from DSP0233 */
+static const u8 I3C_MDB_MCTP = 0xAE;
+/* From MIPI Device Characteristics Register (DCR) Assignments */
+static const u8 I3C_DCR_MCTP = 0xCC;
+
+static const char *MCTP_I3C_OF_PROP = "mctp-controller";
+
+/* List of mctp_i3c_busdev */
+static LIST_HEAD(busdevs);
+/* Protects busdevs, as well as mctp_i3c_bus.devs lists */
+static DEFINE_MUTEX(busdevs_lock);
+
+struct mctp_i3c_bus {
+	struct net_device *ndev;
+
+	struct task_struct *tx_thread;
+	wait_queue_head_t tx_wq;
+	/* tx_lock protects tx_skb and devs */
+	spinlock_t tx_lock;
+	/* Next skb to transmit */
+	struct sk_buff *tx_skb;
+	/* Scratch buffer for xmit */
+	u8 tx_scratch[MCTP_I3C_MAXBUF];
+
+	/* Element of busdevs */
+	struct list_head list;
+
+	/* Provisioned ID of our controller */
+	u64 pid;
+
+	struct i3c_bus *bus;
+	/* Head of mctp_i3c_device.list. Protected by busdevs_lock */
+	struct list_head devs;
+};
+
+struct mctp_i3c_device {
+	struct i3c_device *i3c;
+	struct mctp_i3c_bus *mbus;
+	struct list_head list; /* Element of mctp_i3c_bus.devs */
+
+	/* Held while tx_thread is using this device */
+	struct mutex lock;
+
+	/* Whether BCR indicates MDB is present in IBI */
+	bool have_mdb;
+	/* I3C dynamic address */
+	u8 addr;
+	/* Maximum read length */
+	u16 mrl;
+	/* Maximum write length */
+	u16 mwl;
+	/* Provisioned ID */
+	u64 pid;
+};
+
+/* We synthesise a mac header using the Provisioned ID.
+ * Used to pass dest to mctp_i3c_start_xmit.
+ */
+struct mctp_i3c_internal_hdr {
+	u8 dest[PID_SIZE];
+	u8 source[PID_SIZE];
+} __packed;
+
+/* Returns the 48 bit Provisioned Id from an i3c_device_info.pid */
+static void pid_to_addr(u64 pid, u8 addr[PID_SIZE])
+{
+	pid = cpu_to_be64(pid);
+	memcpy(addr, ((u8 *)&pid) + 2, PID_SIZE);
+}
+
+static u64 addr_to_pid(u8 addr[PID_SIZE])
+{
+	u64 pid = 0;
+
+	memcpy(((u8 *)&pid) + 2, addr, PID_SIZE);
+	return be64_to_cpu(pid);
+}
+
+static int mctp_i3c_read(struct mctp_i3c_device *mi)
+{
+	struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
+	struct net_device_stats *stats = &mi->mbus->ndev->stats;
+	struct mctp_i3c_internal_hdr *ihdr = NULL;
+	struct sk_buff *skb = NULL;
+	struct mctp_skb_cb *cb;
+	int net_status, rc;
+	u8 pec, addr;
+
+	skb = netdev_alloc_skb(mi->mbus->ndev,
+			       mi->mrl + sizeof(struct mctp_i3c_internal_hdr));
+	if (!skb) {
+		stats->rx_dropped++;
+		rc = -ENOMEM;
+		goto err;
+	}
+
+	skb->protocol = htons(ETH_P_MCTP);
+	/* Create a header for internal use */
+	skb_reset_mac_header(skb);
+	ihdr = skb_put(skb, sizeof(struct mctp_i3c_internal_hdr));
+	pid_to_addr(mi->pid, ihdr->source);
+	pid_to_addr(mi->mbus->pid, ihdr->dest);
+	skb_pull(skb, sizeof(struct mctp_i3c_internal_hdr));
+
+	xfer.data.in = skb_put(skb, mi->mrl);
+
+	rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
+	if (rc < 0)
+		goto err;
+
+	if (WARN_ON_ONCE(xfer.len > mi->mrl)) {
+		/* Bad i3c bus driver */
+		rc = -EIO;
+		goto err;
+	}
+	if (xfer.len < MCTP_I3C_MINLEN) {
+		stats->rx_length_errors++;
+		rc = -EIO;
+		goto err;
+	}
+
+	/* check PEC, including address byte */
+	addr = mi->addr << 1 | 1;
+	pec = i2c_smbus_pec(0, &addr, 1);
+	pec = i2c_smbus_pec(pec, xfer.data.in, xfer.len - 1);
+	if (pec != ((u8 *)xfer.data.in)[xfer.len - 1]) {
+		stats->rx_crc_errors++;
+		rc = -EINVAL;
+		goto err;
+	}
+
+	/* Remove PEC */
+	skb_trim(skb, xfer.len - 1);
+
+	cb = __mctp_cb(skb);
+	cb->halen = PID_SIZE;
+	pid_to_addr(mi->pid, cb->haddr);
+
+	net_status = netif_rx(skb);
+
+	if (net_status == NET_RX_SUCCESS) {
+		stats->rx_packets++;
+		stats->rx_bytes += xfer.len - 1;
+	} else {
+		stats->rx_dropped++;
+	}
+
+	return 0;
+err:
+	kfree_skb(skb);
+	return rc;
+}
+
+static void mctp_i3c_ibi_handler(struct i3c_device *i3c,
+				 const struct i3c_ibi_payload *payload)
+{
+	struct mctp_i3c_device *mi = i3cdev_get_drvdata(i3c);
+
+	if (WARN_ON_ONCE(!mi))
+		return;
+
+	if (mi->have_mdb) {
+		if (payload->len > 0) {
+			if (((u8 *)payload->data)[0] != I3C_MDB_MCTP) {
+				/* Not a mctp-i3c interrupt, ignore it */
+				return;
+			}
+		} else {
+			/* The BCR advertised a Mandatory Data Byte but the
+			 * device didn't send one.
+			 */
+			dev_warn_once(i3cdev_to_dev(i3c), "IBI with missing MDB");
+		}
+	}
+
+	mctp_i3c_read(mi);
+}
+
+static int mctp_i3c_setup(struct mctp_i3c_device *mi)
+{
+	const struct i3c_ibi_setup ibi = {
+		.max_payload_len = 1,
+		.num_slots = MCTP_I3C_IBI_SLOTS,
+		.handler = mctp_i3c_ibi_handler,
+	};
+	bool ibi_set = false, ibi_enabled = false;
+	struct i3c_device_info info;
+	int rc;
+
+	i3c_device_get_info(mi->i3c, &info);
+	mi->have_mdb = info.bcr & BIT(2);
+	mi->addr = info.dyn_addr;
+	mi->mwl = info.max_write_len;
+	mi->mrl = info.max_read_len;
+	mi->pid = info.pid;
+
+	rc = i3c_device_request_ibi(mi->i3c, &ibi);
+	if (rc == 0) {
+		ibi_set = true;
+	} else if (rc == -ENOTSUPP) {
+		/* This driver only supports In-Band Interrupt mode. Support for Polling Mode
+		 * could be added if required.
+		 */
+		dev_warn(i3cdev_to_dev(mi->i3c), "Failed, bus driver doesn't support In-Band Interrupts");
+		goto err;
+	} else {
+		dev_err(i3cdev_to_dev(mi->i3c), "Failed requesting IBI (%d)\n", rc);
+		goto err;
+	}
+
+	if (ibi_set) {
+		/* Device setup must be complete when IBI is enabled */
+		rc = i3c_device_enable_ibi(mi->i3c);
+		if (rc < 0) {
+			/* Assume a driver supporting request_ibi also supports enable_ibi */
+			dev_err(i3cdev_to_dev(mi->i3c), "Failed enabling IBI (%d)\n", rc);
+			goto err;
+		}
+		ibi_enabled = true;
+	}
+
+	return 0;
+err:
+	if (ibi_enabled)
+		i3c_device_disable_ibi(mi->i3c);
+	if (ibi_set)
+		i3c_device_free_ibi(mi->i3c);
+	return rc;
+}
+
+/* Adds a new MCTP i3c_device to a bus */
+static int mctp_i3c_add_device(struct mctp_i3c_bus *mbus, struct i3c_device *i3c)
+__must_hold(&busdevs_lock)
+{
+	struct mctp_i3c_device *mi = NULL;
+	int rc;
+
+	mi = kzalloc(sizeof(*mi), GFP_KERNEL);
+	if (!mi) {
+		rc = -ENOMEM;
+		goto err;
+	}
+	mi->mbus = mbus;
+	mi->i3c = i3c;
+	mutex_init(&mi->lock);
+	list_add(&mi->list, &mbus->devs);
+
+	i3cdev_set_drvdata(i3c, mi);
+	rc = mctp_i3c_setup(mi);
+	if (rc < 0)
+		goto err;
+
+	return 0;
+err:
+	dev_warn(i3cdev_to_dev(i3c), "Error adding mctp-i3c device, %d\n", rc);
+	if (mi) {
+		list_del(&mi->list);
+		kfree(mi);
+	}
+	return rc;
+}
+
+static int mctp_i3c_probe(struct i3c_device *i3c)
+{
+	struct mctp_i3c_bus *b = NULL, *mbus = NULL;
+	int rc;
+
+	/* Look for a known bus */
+	mutex_lock(&busdevs_lock);
+	list_for_each_entry(b, &busdevs, list)
+		if (b->bus == i3c->bus) {
+			mbus = b;
+			break;
+		}
+	mutex_unlock(&busdevs_lock);
+
+	if (!mbus) {
+		/* probably no "mctp-controller" property on the i3c bus */
+		return -ENODEV;
+	}
+
+	rc = mctp_i3c_add_device(mbus, i3c);
+	if (!rc)
+		goto err;
+
+	return 0;
+
+err:
+	return rc;
+}
+
+static void mctp_i3c_remove_device(struct mctp_i3c_device *mi)
+__must_hold(&busdevs_lock)
+{
+	/* Ensure the tx thread isn't using the device */
+	mutex_lock(&mi->lock);
+
+	/* Counterpart of mctp_i3c_setup */
+	i3c_device_disable_ibi(mi->i3c);
+	i3c_device_free_ibi(mi->i3c);
+
+	/* Counterpart of mctp_i3c_add_device */
+	i3cdev_set_drvdata(mi->i3c, NULL);
+	list_del(&mi->list);
+
+	/* Safe to unlock after removing from the list */
+	mutex_unlock(&mi->lock);
+	kfree(mi);
+}
+
+static void mctp_i3c_remove(struct i3c_device *i3c)
+{
+	struct mctp_i3c_device *mi = i3cdev_get_drvdata(i3c);
+
+	/* We my have received a Bus Remove notify prior to device remove,
+	 * so mi will already be removed.
+	 */
+	if (!mi)
+		return;
+
+	mutex_lock(&busdevs_lock);
+	mctp_i3c_remove_device(mi);
+	mutex_unlock(&busdevs_lock);
+}
+
+/* Returns the device for an address, with mi->lock held */
+static struct mctp_i3c_device *
+mctp_i3c_lookup(struct mctp_i3c_bus *mbus, u64 pid)
+{
+	struct mctp_i3c_device *mi = NULL, *ret = NULL;
+
+	mutex_lock(&busdevs_lock);
+	list_for_each_entry(mi, &mbus->devs, list)
+		if (mi->pid == pid) {
+			ret = mi;
+			mutex_lock(&mi->lock);
+			break;
+		}
+	mutex_unlock(&busdevs_lock);
+	return ret;
+}
+
+static void mctp_i3c_xmit(struct mctp_i3c_bus *mbus, struct sk_buff *skb)
+{
+	struct net_device_stats *stats = &mbus->ndev->stats;
+	struct i3c_priv_xfer xfer = { .rnw = false };
+	struct mctp_i3c_internal_hdr *ihdr = NULL;
+	struct mctp_i3c_device *mi = NULL;
+	u8 *data = NULL;
+	unsigned int data_len;
+	u8 addr, pec;
+	int rc = 0;
+	u64 pid;
+
+	skb_pull(skb, sizeof(struct mctp_i3c_internal_hdr));
+	data_len = skb->len;
+
+	ihdr = (void *)skb_mac_header(skb);
+
+	pid = addr_to_pid(ihdr->dest);
+	mi = mctp_i3c_lookup(mbus, pid);
+	if (!mi) {
+		/* I3C endpoint went away after the packet was enqueued? */
+		stats->tx_dropped++;
+		goto out;
+	}
+
+	if (WARN_ON_ONCE(data_len + 1 > MCTP_I3C_MAXBUF))
+		goto out;
+
+	if (data_len + 1 > (unsigned int)mi->mwl) {
+		/* Route MTU was larger than supported by the endpoint */
+		stats->tx_dropped++;
+		goto out;
+	}
+
+	/* Need a linear buffer with space for the PEC */
+	xfer.len = data_len + 1;
+	if (skb_tailroom(skb) >= 1) {
+		skb_put(skb, 1);
+		data = skb->data;
+	} else {
+		// TODO: test this
+		/* Otherwise need to copy the buffer */
+		skb_copy_bits(skb, 0, mbus->tx_scratch, skb->len);
+		data = mbus->tx_scratch;
+	}
+
+	/* PEC calculation */
+	addr = mi->addr << 1;
+	pec = i2c_smbus_pec(0, &addr, 1);
+	pec = i2c_smbus_pec(pec, data, data_len);
+	data[data_len] = pec;
+
+	xfer.data.out = data;
+	rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
+	if (rc == 0) {
+		stats->tx_bytes += data_len;
+		stats->tx_packets++;
+	} else {
+		stats->tx_errors++;
+	}
+
+out:
+	if (mi)
+		mutex_unlock(&mi->lock);
+}
+
+static int mctp_i3c_tx_thread(void *data)
+{
+	struct mctp_i3c_bus *mbus = data;
+	struct sk_buff *skb;
+	unsigned long flags;
+
+	for (;;) {
+		if (kthread_should_stop())
+			break;
+
+		spin_lock_irqsave(&mbus->tx_lock, flags);
+		skb = mbus->tx_skb;
+		mbus->tx_skb = NULL;
+		spin_unlock_irqrestore(&mbus->tx_lock, flags);
+
+		if (netif_queue_stopped(mbus->ndev))
+			netif_wake_queue(mbus->ndev);
+
+		if (skb) {
+			mctp_i3c_xmit(mbus, skb);
+			kfree_skb(skb);
+		} else {
+			wait_event_idle(mbus->tx_wq,
+					mbus->tx_skb || kthread_should_stop());
+		}
+	}
+
+	return 0;
+}
+
+static netdev_tx_t mctp_i3c_start_xmit(struct sk_buff *skb,
+				       struct net_device *ndev)
+{
+	struct mctp_i3c_bus *mbus = netdev_priv(ndev);
+	unsigned long flags;
+	netdev_tx_t ret;
+
+	spin_lock_irqsave(&mbus->tx_lock, flags);
+	netif_stop_queue(ndev);
+	if (mbus->tx_skb) {
+		dev_warn_ratelimited(&ndev->dev, "TX with queue stopped");
+		ret = NETDEV_TX_BUSY;
+	} else {
+		mbus->tx_skb = skb;
+		ret = NETDEV_TX_OK;
+	}
+	spin_unlock_irqrestore(&mbus->tx_lock, flags);
+
+	if (ret == NETDEV_TX_OK)
+		wake_up(&mbus->tx_wq);
+
+	return ret;
+}
+
+static void mctp_i3c_bus_free(struct mctp_i3c_bus *mbus)
+__must_hold(&busdevs_lock)
+{
+	struct mctp_i3c_device *mi = NULL, *tmp = NULL;
+
+	if (mbus->tx_thread) {
+		kthread_stop(mbus->tx_thread);
+		mbus->tx_thread = NULL;
+	}
+
+	/* Remove any child devices */
+	list_for_each_entry_safe(mi, tmp, &mbus->devs, list) {
+		mctp_i3c_remove_device(mi);
+	}
+
+	kfree_skb(mbus->tx_skb);
+	list_del(&mbus->list);
+}
+
+static void mctp_i3c_ndo_uninit(struct net_device *ndev)
+{
+	struct mctp_i3c_bus *mbus = netdev_priv(ndev);
+
+	/* Perform cleanup here to ensure there are no remaining references */
+	mctp_i3c_bus_free(mbus);
+}
+
+static int mctp_i3c_header_create(struct sk_buff *skb, struct net_device *dev,
+				  unsigned short type, const void *daddr,
+	   const void *saddr, unsigned int len)
+{
+	struct mctp_i3c_internal_hdr *ihdr;
+
+	skb_push(skb, sizeof(struct mctp_i3c_internal_hdr));
+	skb_reset_mac_header(skb);
+	ihdr = (void *)skb_mac_header(skb);
+	memcpy(ihdr->dest, daddr, PID_SIZE);
+	memcpy(ihdr->source, saddr, PID_SIZE);
+	return 0;
+}
+
+static const struct net_device_ops mctp_i3c_ops = {
+	.ndo_start_xmit = mctp_i3c_start_xmit,
+	.ndo_uninit = mctp_i3c_ndo_uninit,
+};
+
+static const struct header_ops mctp_i3c_headops = {
+	.create = mctp_i3c_header_create,
+};
+
+static void mctp_i3c_net_setup(struct net_device *dev)
+{
+	dev->type = ARPHRD_MCTP;
+
+	dev->mtu = MCTP_I3C_MAXMTU;
+	dev->min_mtu = MCTP_I3C_MINMTU;
+	dev->max_mtu = MCTP_I3C_MAXMTU;
+	dev->tx_queue_len = MCTP_I3C_TX_QUEUE_LEN;
+
+	dev->hard_header_len = sizeof(struct mctp_i3c_internal_hdr);
+	dev->addr_len = PID_SIZE;
+
+	dev->netdev_ops	= &mctp_i3c_ops;
+	dev->header_ops	= &mctp_i3c_headops;
+}
+
+static bool mctp_i3c_is_mctp_controller(struct i3c_bus *bus)
+{
+	struct i3c_dev_desc *master = bus->cur_master;
+
+	if (!master)
+		return false;
+
+	return of_property_read_bool(master->common.master->dev.of_node, MCTP_I3C_OF_PROP);
+}
+
+/* Returns the Provisioned Id of a local bus master */
+static int mctp_i3c_bus_local_pid(struct i3c_bus *bus, u64 *ret_pid)
+{
+	struct i3c_dev_desc *master;
+
+	master = bus->cur_master;
+	if (WARN_ON_ONCE(!master))
+		return -ENOENT;
+	*ret_pid = master->info.pid;
+
+	return 0;
+}
+
+/* Returns an ERR_PTR on failure */
+static struct mctp_i3c_bus *mctp_i3c_bus_add(struct i3c_bus *bus)
+__must_hold(&busdevs_lock)
+{
+	struct mctp_i3c_bus *mbus = NULL;
+	struct net_device *ndev = NULL;
+	u8 addr[PID_SIZE];
+	char namebuf[IFNAMSIZ];
+	int rc;
+
+	if (!mctp_i3c_is_mctp_controller(bus))
+		return ERR_PTR(-ENOENT);
+
+	snprintf(namebuf, sizeof(namebuf), "mctpi3c%d", bus->id);
+	ndev = alloc_netdev(sizeof(*mbus), namebuf, NET_NAME_ENUM, mctp_i3c_net_setup);
+	if (!ndev) {
+		pr_warn("No memory for %s\n", namebuf);
+		rc = -ENOMEM;
+		goto err;
+	}
+	dev_net_set(ndev, current->nsproxy->net_ns);
+
+	mbus = netdev_priv(ndev);
+	mbus->ndev = ndev;
+	mbus->bus = bus;
+	INIT_LIST_HEAD(&mbus->devs);
+	list_add(&mbus->list, &busdevs);
+
+	rc = mctp_i3c_bus_local_pid(bus, &mbus->pid);
+	if (rc < 0) {
+		pr_err("No I3C PID available for %s\n", namebuf);
+		goto err;
+	}
+	pid_to_addr(mbus->pid, addr);
+	dev_addr_set(ndev, addr);
+
+	init_waitqueue_head(&mbus->tx_wq);
+	spin_lock_init(&mbus->tx_lock);
+	mbus->tx_thread = kthread_create(mctp_i3c_tx_thread, mbus,
+					 "%s/tx", ndev->name);
+	if (IS_ERR(mbus->tx_thread)) {
+		pr_warn("Error creating thread for %s: %pe\n",
+			namebuf, mbus->tx_thread);
+		rc = PTR_ERR(mbus->tx_thread);
+		mbus->tx_thread = NULL;
+		goto err;
+	}
+	wake_up_process(mbus->tx_thread);
+
+	rc = mctp_register_netdev(ndev, NULL);
+	if (rc < 0) {
+		pr_warn("netdev register failed for %s: %d\n", namebuf, rc);
+		goto err;
+	}
+	return mbus;
+err:
+	/* uninit will not get called if a netdev has not been registered,
+	 * so we perform the same mbus cleanup manually.
+	 */
+	if (mbus)
+		mctp_i3c_bus_free(mbus);
+	if (ndev)
+		free_netdev(ndev);
+	return ERR_PTR(rc);
+}
+
+static void mctp_i3c_bus_remove(struct mctp_i3c_bus *mbus)
+__must_hold(&busdevs_lock)
+{
+	/* Unregister calls through to ndo_uninit -> mctp_i3c_bus_free() */
+	mctp_unregister_netdev(mbus->ndev);
+
+	free_netdev(mbus->ndev);
+	/* mbus is deallocated */
+}
+
+/* Removes all mctp-i3c busses */
+static void mctp_i3c_bus_remove_all(void)
+{
+	struct mctp_i3c_bus *mbus = NULL, *tmp = NULL;
+
+	mutex_lock(&busdevs_lock);
+	list_for_each_entry_safe(mbus, tmp, &busdevs, list) {
+		mctp_i3c_bus_remove(mbus);
+	}
+	mutex_unlock(&busdevs_lock);
+}
+
+/* Adds a i3c_bus if it isn't already in the busdevs list.
+ * Suitable as an i3c_for_each_bus_locked callback.
+ */
+static int mctp_i3c_bus_add_new(struct i3c_bus *bus, void *data)
+{
+	struct mctp_i3c_bus *mbus = NULL, *tmp = NULL;
+	bool exists = false;
+
+	mutex_lock(&busdevs_lock);
+	list_for_each_entry_safe(mbus, tmp, &busdevs, list)
+		if (mbus->bus == bus)
+			exists = true;
+
+	/* It is OK for a bus to already exist. That can occur due to
+	 * the race in mod_init between notifier and for_each_bus
+	 */
+	if (!exists)
+		mctp_i3c_bus_add(bus);
+	mutex_unlock(&busdevs_lock);
+	return 0;
+}
+
+static void mctp_i3c_notify_bus_remove(struct i3c_bus *bus)
+{
+	struct mctp_i3c_bus *mbus = NULL, *tmp;
+
+	mutex_lock(&busdevs_lock);
+	list_for_each_entry_safe(mbus, tmp, &busdevs, list)
+		if (mbus->bus == bus)
+			mctp_i3c_bus_remove(mbus);
+	mutex_unlock(&busdevs_lock);
+}
+
+static int mctp_i3c_notifier_call(struct notifier_block *nb,
+				  unsigned long action, void *data)
+{
+	switch (action) {
+	case I3C_NOTIFY_BUS_ADD:
+		mctp_i3c_bus_add_new((struct i3c_bus *)data, NULL);
+		break;
+	case I3C_NOTIFY_BUS_REMOVE:
+		mctp_i3c_notify_bus_remove((struct i3c_bus *)data);
+		break;
+	}
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block mctp_i3c_notifier = {
+	.notifier_call = mctp_i3c_notifier_call,
+};
+
+static const struct i3c_device_id mctp_i3c_ids[] = {
+	I3C_CLASS(I3C_DCR_MCTP, NULL),
+	{ 0 },
+};
+
+static struct i3c_driver mctp_i3c_driver = {
+	.driver = {
+		.name = "mctp-i3c",
+	},
+	.probe = mctp_i3c_probe,
+	.remove = mctp_i3c_remove,
+	.id_table = mctp_i3c_ids,
+};
+
+static __init int mctp_i3c_mod_init(void)
+{
+	int rc;
+
+	rc = i3c_register_notifier(&mctp_i3c_notifier);
+	if (rc < 0) {
+		i3c_driver_unregister(&mctp_i3c_driver);
+		return rc;
+	}
+
+	i3c_for_each_bus_locked(mctp_i3c_bus_add_new, NULL);
+
+	rc = i3c_driver_register(&mctp_i3c_driver);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static __exit void mctp_i3c_mod_exit(void)
+{
+	int rc;
+
+	i3c_driver_unregister(&mctp_i3c_driver);
+
+	rc = i3c_unregister_notifier(&mctp_i3c_notifier);
+	if (rc < 0)
+		pr_warn("MCTP I3C could not unregister notifier, %d\n", rc);
+
+	mctp_i3c_bus_remove_all();
+}
+
+module_init(mctp_i3c_mod_init);
+module_exit(mctp_i3c_mod_exit);
+
+MODULE_DEVICE_TABLE(i3c, mctp_i3c_ids);
+MODULE_DESCRIPTION("MCTP I3C device");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matt Johnston <matt@codeconstruct.com.au>");
-- 
2.37.2


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC PATCH 0/2] I3C MCTP net driver

[Winiarska, Iwona]

On Thu, 2023-04-13 at 16:11 +0800, Matt Johnston wrote:
> This series adds an I3C transport for the kernel's MCTP network
> protocol. MCTP is a communication protocol between system components
> (BMCs, drives, NICs etc), with higher level protocols such as NVMe-MI or
> PLDM built on top of it (in userspace). It runs over various transports
> such as I2C, PCIe, or I3C.
> 
> The mctp-i3c driver follows a similar approach to the kernel's existing
> mctp-i2c driver, creating a "mctpi3cX" network interface for each
> numbered I3C bus. Busses opt in to support by adding a "mctp-controller"
> property to the devicetree:
> 
> &i3c0 {
>         mctp-controller;
> }

Hi,

Wouldn't creating "mctpi3cX" network interface for each I3C device rather than
I3C bus be a better fit to model MCTP over I3C?
Why are we following the mctp-i2c approach rather than mctp-serial?

MCTP over I3C is (like serial) point-to-point - connection between I3C
controller and each I3C device can be treated as separate "logical" bus (since
the only way that those devices can potentially communicate with each other is
using MCTP bridging) and, potentially, a separate MCTP network.

This would also simplify the series, as we would no longer need "mctp-
controller" property, and the driver would just follow the I3C class driver
model without the need for notifiers.

Thanks
-Iwona

> 
> The driver will bind to MCTP class devices (DCR 0xCC) that are on a
> supported I3C bus. Each bus is represented by a `struct mctp_i3c_bus`
> that keeps state for the network device. An individual I3C device
> (struct mctp_i3c_device) performs operations using the "parent"
> mctp_i3c_bus object. The I3C notify/enumeration patch is needed so that
> the mctp-i3c driver can handle creating/removing mctp_i3c_bus objects as
> required.
> 
> The mctp-i3c driver is using the Provisioned ID as an identifier for
> target I3C devices (the neighbour address), as that will be more stable
> than the I3C dynamic address. The driver internally translates that to a
> dynamic address for bus operations.
> 
> The driver has been tested using an AST2600 platform, with Jeremy's
> In-Band Interrupt support patches. A remote endpoint has been tested
> against Qemu, as well as using the target mode support in Aspeed's
> vendor tree.
> 
> I'm sending this as an RFC for linux-i3c, then will submit the mctp-i3c
> driver itself to the netdev list after it has had review here.
> 
> Cheers,
> Matt
> 
> Jeremy Kerr (1):
>   i3c: Add support for bus enumeration & notification
> 
> Matt Johnston (1):
>   mctp i3c: MCTP I3C driver
> 
>  drivers/i3c/master.c        |  35 ++
>  drivers/net/mctp/Kconfig    |   9 +
>  drivers/net/mctp/Makefile   |   1 +
>  drivers/net/mctp/mctp-i3c.c | 778 ++++++++++++++++++++++++++++++++++++
>  include/linux/i3c/master.h  |  11 +
>  5 files changed, 834 insertions(+)
>  create mode 100644 drivers/net/mctp/mctp-i3c.c
> 
> -- 
> 2.37.2
> 
> 

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC PATCH 0/2] I3C MCTP net driver

[Jeremy Kerr]

Hi Iwona,

> Wouldn't creating "mctpi3cX" network interface for each I3C device rather than
> I3C bus be a better fit to model MCTP over I3C?

Great question! A few reasons for this structure:

 - it is a closer match to existing usage of network interfaces and
   remote endpoints (say, IP, CAN, etc): you have an interface for
   representing the local hardware and stack state, which may provide a
   path for any devices reachable through that hardware.

 - a simpler addressing structure: you only need one *local* MCTP
   address (EID) for the whole bus, rather that one per remote device.

 - the presence/absence of interfaces is not dependent on what's
   connected to the bus. If we used a netdev per remote device, users
   would not be able to set up the local stack until a remote endpoint
   is bound (ie, userspace would need to configure every interface on
   i3c hot join). With this model, that stack state can persist over
   whatever may be happening to remote devices (unplug, reset,
   re-addressing, etc).

 - if there is ever a proposal for broadcast messages in the i3c
   transport binding, we would have to re-write it in this structure
   anyway.

> Why are we following the mctp-i2c approach rather than mctp-serial?

mctp-serial follows the same model: the local device is always present,
the difference here being that no physical addressing is required.
Routes to remote endpoints are added depending on remote endpoint state,
but the local state is preserved regardless of what is (or is not) on
the other end of the link.

> This would also simplify the series, as we would no longer need "mctp-
> controller" property, and the driver would just follow the I3C class driver
> model without the need for notifiers.

It would simplify the series, but we would be punting a lot of those
complexities over to userspace.

Cheers,


Jeremy

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC PATCH 0/2] I3C MCTP net driver

[Winiarska, Iwona]

On Tue, 2023-05-09 at 07:42 +0800, Jeremy Kerr wrote:
> Hi Iwona,
> 
> > Wouldn't creating "mctpi3cX" network interface for each I3C device rather
> > than
> > I3C bus be a better fit to model MCTP over I3C?
> 
> Great question! A few reasons for this structure:
> 
>  - it is a closer match to existing usage of network interfaces and
>    remote endpoints (say, IP, CAN, etc): you have an interface for
>    representing the local hardware and stack state, which may provide a
>    path for any devices reachable through that hardware.
> 
>  - a simpler addressing structure: you only need one *local* MCTP
>    address (EID) for the whole bus, rather that one per remote device.
> 
>  - the presence/absence of interfaces is not dependent on what's
>    connected to the bus. If we used a netdev per remote device, users
>    would not be able to set up the local stack until a remote endpoint
>    is bound (ie, userspace would need to configure every interface on
>    i3c hot join). With this model, that stack state can persist over
>    whatever may be happening to remote devices (unplug, reset,
>    re-addressing, etc).
> 
>  - if there is ever a proposal for broadcast messages in the i3c
>    transport binding, we would have to re-write it in this structure
>    anyway.
> 
> > Why are we following the mctp-i2c approach rather than mctp-serial?
> 
> mctp-serial follows the same model: the local device is always present,
> the difference here being that no physical addressing is required.
> Routes to remote endpoints are added depending on remote endpoint state,
> but the local state is preserved regardless of what is (or is not) on
> the other end of the link.
> 
> > This would also simplify the series, as we would no longer need "mctp-
> > controller" property, and the driver would just follow the I3C class driver
> > model without the need for notifiers.
> 
> It would simplify the series, but we would be punting a lot of those
> complexities over to userspace.

Thanks for the explanation - and it makes perfect sense, I'm just worried that
it might be difficult to model certain use cases with this structure.

Specifically: 

1) This is a simplified example - the EID collision can happen further down the
network (with both I3C devices acting as bridges and having multiple MCTP
endpoints behind them), bottom line is - we're dealing two separate networks.
Since the network is a link property - do we have a way forward if we go with a
single network interface for I3C controller if we need to work in an environment
like this?

                             Network 1
                         +------------------+
                         |                  |
   Network ?             | device eid 0x12  |
+------------+     +-----+                  |
|  eid 0x11  +-----+     +------------------+
| controller |
|  eid 0x10  +-----+     +------------------+
+------------+     +-----+                  |
                         | device eid 0x12  |
                         |                  |
                         +------------------+
                             Network 2

2) This is the "normal" case. 0x11 sends MCTP message to 0x12 (by issuing IBI to
the controller). How will the routing table need to look like in order for the
message to be retransmited on the same network interface? What if we do not want
to forward? What would be the difference?

                             Network 1
                         +------------------+
                         |                  |
   Network 1             | device eid 0x11  |
+------------+     +-----+                  |
|            +-----+     +------------------+
| controller |
|  eid 0x10  +-----+     +------------------+
+------------+     +-----+                  |
                         | device eid 0x12  |
                         |                  |
                         +------------------+
                             Network 1

Thanks
-Iwona


> 
> Cheers,
> 
> 
> Jeremy

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC PATCH 0/2] I3C MCTP net driver

[Jeremy Kerr]

Hi Iwona,

> Since the network is a link property - do we have a way forward if we go with a
> single network interface for I3C controller if we need to work in an environment
> like this?
> 
>                              Network 1
>                          +------------------+
>                          |                  |
>    Network ?             | device eid 0x12  |
> +------------+     +-----+                  |
> >  eid 0x11  +-----+     +------------------+
> > controller |
> >  eid 0x10  +-----+     +------------------+
> +------------+     +-----+                  |
>                          | device eid 0x12  |
>                          |                  |
>                          +------------------+
>                              Network 2

So I went digging around to see how this configuration fits with the
MCTP standards. A bit of background, which I'm sure you're already
familiar with, but just so we have a shared reference:

The MCTP Base spec (DSP0236) has a definition for a "MCTP bus":

    3.2.7
    bus
    a physical addressing domain shared between one or more platform
    components that share a common physical layer address space

Which would mean that your two devices there are on the same MCTP bus,
given we have shared physical address space. Our approach for the kernel
MCTP stack is for a netdev (ie, the interface) to represent the system's
connection to a bus.

While there's nothing explicitly defining whether a MCTP bus is required
to only host one MCTP network, all of the example topologies in DSP0236
seem to assume so.

Then: the MCTP I3C Transport Binding spec (DSP0233) introduces a
"logical bus" term, but with no formal definition. This would cover your
model above, where the two devices are on different logical busses, but
the same MCTP bus. There's an example of that in Figure 3.

However, there's still no clarification on whether these logical buses
can be on separate networks (and so operate within a separate EID
space). In fact, the Figure 3 example *requires* the logical busses to
be on the same network, as the Top Level Bus Owner exists on a separate
logical bus to its owned devices.

[There are also a couple of inconsistencies in this part of DSP0233;
there's a reference to "bridging between networks", which isn't possible
with the definitions of "bridging" and "network", as well as a footnote
that contradicts the definition of a MCTP bus. I'll see if I can chase
up some clarifications to those]

Anyhow: it doesn't seem to be prohibited by the standard, but I'm not
seeing much in the way of allowing it either. I'm happy to go by the
assumption that it's allowed, but it does seem unnecessarily eclectic to
me.

Back to the figure 3 example, and assuming we want to preserve the
existing concept of the netdev being the connection to a bus: this
*requires* us to implement the netdev-per-bus (rather than
netdev-per-remote-endpoint) model as proposed in this series, as there
is the shared logical bus between multiple remote endpoints.

In order to represent your use-case of multiple logical busses, I would
propose that we use a similar model to how logical ethernet interfaces
(ie, VLANs) are already implemented in Linux. Under that model, we could
allow new (logical) interfaces to be created as subordinate to the
existing (actual) interfaces, which would represent the system's
connection to a *logical* bus, and provide the mapping to a MCTP network.

For that, we would need a bit of netlink definition to allow
construction of the logical-bus netdev. I *think* we could then use the
existing neighbour table definitions to provide the partitioning of
devices over logical busses.

However: I would think it would be much simpler to just put the two
devices on the same i3c bus on the same MCTP network, and assign
distinct EIDs. Whether those devices could communicate can be defined by
bridging policy.

On to that, while slightly re-ordering your questions:

> 2) This is the "normal" case.
> 
>                              Network 1
>                          +------------------+
>                          |                  |
>    Network 1             | device eid 0x11  |
> +------------+     +-----+                  |
> >            +-----+     +------------------+
> > controller |
> >  eid 0x10  +-----+     +------------------+
> +------------+     +-----+                  |
>                          | device eid 0x12  |
>                          |                  |
>                          +------------------+
>                              Network 1
> 
> How will the routing table need to look like in order for the message
> to be retransmited on the same network interface? What if we do not
> want to forward? What would be the difference?

There are a couple of options here:

 * we set a "forwarding" boolean flag on the network, which specifies
   whether or not packets would be forwarded by the controller (acting
   as a bridge)

 * individual routes could be configured with bridging policy: in this
   case, the input interface and/or the source EIDs.

The former is definitely simpler, and was what I had originally
intended, but may not cover all use-cases. I'd be interested in your
thoughts there.

Cheers,


Jeremy
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC PATCH 0/2] I3C MCTP net driver

[Winiarska, Iwona]

On Wed, 2023-05-10 at 11:28 +0800, Jeremy Kerr wrote:
> Hi Iwona,
> 
> > Since the network is a link property - do we have a way forward if we go
> > with a
> > single network interface for I3C controller if we need to work in an
> > environment
> > like this?
> > 
> >                              Network 1
> >                          +------------------+
> >                          |                  |
> >    Network ?             | device eid 0x12  |
> > +------------+     +-----+                  |
> > >  eid 0x11  +-----+     +------------------+
> > > controller |
> > >  eid 0x10  +-----+     +------------------+
> > +------------+     +-----+                  |
> >                          | device eid 0x12  |
> >                          |                  |
> >                          +------------------+
> >                              Network 2
> 
> So I went digging around to see how this configuration fits with the
> MCTP standards. A bit of background, which I'm sure you're already
> familiar with, but just so we have a shared reference:
> 
> The MCTP Base spec (DSP0236) has a definition for a "MCTP bus":
> 
>     3.2.7
>     bus
>     a physical addressing domain shared between one or more platform
>     components that share a common physical layer address space
> 
> Which would mean that your two devices there are on the same MCTP bus,
> given we have shared physical address space. Our approach for the kernel
> MCTP stack is for a netdev (ie, the interface) to represent the system's
> connection to a bus.
> 
> While there's nothing explicitly defining whether a MCTP bus is required
> to only host one MCTP network, all of the example topologies in DSP0236
> seem to assume so.
> 
> Then: the MCTP I3C Transport Binding spec (DSP0233) introduces a
> "logical bus" term, but with no formal definition. This would cover your
> model above, where the two devices are on different logical busses, but
> the same MCTP bus. There's an example of that in Figure 3.
> 
> However, there's still no clarification on whether these logical buses
> can be on separate networks (and so operate within a separate EID
> space). In fact, the Figure 3 example *requires* the logical busses to
> be on the same network, as the Top Level Bus Owner exists on a separate
> logical bus to its owned devices.
> 
> [There are also a couple of inconsistencies in this part of DSP0233;
> there's a reference to "bridging between networks", which isn't possible
> with the definitions of "bridging" and "network", as well as a footnote
> that contradicts the definition of a MCTP bus. I'll see if I can chase
> up some clarifications to those]
> 
> Anyhow: it doesn't seem to be prohibited by the standard, but I'm not
> seeing much in the way of allowing it either. I'm happy to go by the
> assumption that it's allowed, but it does seem unnecessarily eclectic to
> me.
> 
> Back to the figure 3 example, and assuming we want to preserve the
> existing concept of the netdev being the connection to a bus: this
> *requires* us to implement the netdev-per-bus (rather than
> netdev-per-remote-endpoint) model as proposed in this series, as there
> is the shared logical bus between multiple remote endpoints.
> 
> In order to represent your use-case of multiple logical busses, I would
> propose that we use a similar model to how logical ethernet interfaces
> (ie, VLANs) are already implemented in Linux. Under that model, we could
> allow new (logical) interfaces to be created as subordinate to the
> existing (actual) interfaces, which would represent the system's
> connection to a *logical* bus, and provide the mapping to a MCTP network.
> 
> For that, we would need a bit of netlink definition to allow
> construction of the logical-bus netdev. I *think* we could then use the
> existing neighbour table definitions to provide the partitioning of
> devices over logical busses.
> 
> However: I would think it would be much simpler to just put the two
> devices on the same i3c bus on the same MCTP network, and assign
> distinct EIDs. Whether those devices could communicate can be defined by
> bridging policy.

The only reason why I believe it would be possible for someone to come up with
such network layout would be the limited amount of EIDs available in MCTP 1.x,
in which case putting everything in the same MCTP network won't be an option.

I'm bringing this up, since I originally considered device-driver model that
goes with network controller per I3C device.
I think this is the only case in which it can be more complicated to model.
I just want to avoid a situation where we paint ourselves into a corner, and the
VLAN-like approach sounds good to me (if we ever end up needing it).

> 
> On to that, while slightly re-ordering your questions:
> 
> > 2) This is the "normal" case.
> > 
> >                              Network 1
> >                          +------------------+
> >                          |                  |
> >    Network 1             | device eid 0x11  |
> > +------------+     +-----+                  |
> > >            +-----+     +------------------+
> > > controller |
> > >  eid 0x10  +-----+     +------------------+
> > +------------+     +-----+                  |
> >                          | device eid 0x12  |
> >                          |                  |
> >                          +------------------+
> >                              Network 1
> > 
> > How will the routing table need to look like in order for the message
> > to be retransmited on the same network interface? What if we do not
> > want to forward? What would be the difference?
> 
> There are a couple of options here:
> 
>  * we set a "forwarding" boolean flag on the network, which specifies
>    whether or not packets would be forwarded by the controller (acting
>    as a bridge)
> 
>  * individual routes could be configured with bridging policy: in this
>    case, the input interface and/or the source EIDs.
> 
> The former is definitely simpler, and was what I had originally
> intended, but may not cover all use-cases. I'd be interested in your
> thoughts there.

Currently, I don't see any use cases where a "forwarding" boolean flag wouldn't
be enough, but we can go back to this in the future when we add bridging
support. 

Thanks
-Iwona

> 
> Cheers,
> 
> 
> Jeremy

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC PATCH 0/2] I3C MCTP net driver

[Jeremy Kerr]

Hi Iwona,

> > However: I would think it would be much simpler to just put the two
> > devices on the same i3c bus on the same MCTP network, and assign
> > distinct EIDs. Whether those devices could communicate can be defined by
> > bridging policy.
> 
> The only reason why I believe it would be possible for someone to come up with
> such network layout would be the limited amount of EIDs available in MCTP 1.x,
> in which case putting everything in the same MCTP network won't be an option.

Yep, that's the reason why we implemented networks in the initial core
code - it's likely that we'll hit the EID limits at some point.

> I'm bringing this up, since I originally considered device-driver model that
> goes with network controller per I3C device.
> I think this is the only case in which it can be more complicated to model.
> I just want to avoid a situation where we paint ourselves into a corner, and the
> VLAN-like approach sounds good to me (if we ever end up needing it).

OK, neat - I think we have a plan should we need to represent the i3c
"logical bus" in future then.

> > The former is definitely simpler, and was what I had originally
> > intended, but may not cover all use-cases. I'd be interested in your
> > thoughts there.
> 
> Currently, I don't see any use cases where a "forwarding" boolean flag wouldn't
> be enough, but we can go back to this in the future when we add bridging
> support.

Sounds good then, thanks for the input!

Cheers,


Jeremy

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c