Re: [PATCH 3/3] mctp i3c: MCTP I3C driver

Hi Andrew,

On Mon, 2023-07-03 at 16:43 +0200, Andrew Lunn wrote:

quoted

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

Why static const and not #define? It would also be normal for
variables to be lower case, to make it clear they are in fact
variables, not #defines.

My personal preference is for static const since it's less error prone,
though had to use #define for the ones used in array sizing. Happy to change
to #define if that's the style though.

quoted

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

Since you have commented about most of the members of these
structures, you could use kerneldoc.

These aren't intended to be exposed as an API anywhere, just commented
for future code readers (including me). Is kerneldoc still appropriate?

quoted

+/* 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);
+}

I don't know anything about MCTP. But Ethernet MAC addresses are also
48 bits. Could you make use of u64_to_ether_addr() and ether_addr_to_u64()?

The 48 bit identifier is an I3C Provisioned ID. It has a similar purpose to
an ethernet MAC address but for a different protocol. I think it might cause
confusion to code readers if it were passed to ethernet functions.

quoted

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

In networking, we try to avoid ENOTSUPP and use EOPNOTSUPP:

https://lore.kernel.org/netdev/20200511165319.2251678-1-kuba@kernel.org/ (local)

checkpatch noticed this one too, but the existing I3C functions return
ENOTSUPP so it needs to match against that.

quoted

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

ether_addr_copy() ?

quoted

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

pr_ functions are not liked too much. Is there a struct device you can
use with dev_warn()?

I'll change the ones with a device available, that one in particular can be
removed anyway.

Thanks,
Matt