| Message ID | 20230703053048.275709-1-matt@codeconstruct.com.au |
|---|---|
| Headers | show |
| Series | I3C MCTP net driver | expand |
On Mon, 2023-07-03 at 16:16 +0200, Krzysztof Kozlowski wrote: > On 03/07/2023 10:14, Matt Johnston wrote: > > On Mon, 2023-07-03 at 09:15 +0200, Krzysztof Kozlowski wrote: > > > On Mon, 3 Jul 2023 at 07:31, Matt Johnston <matt@codeconstruct.com.au> wrote: > > > > > > > > + mctp-controller: > > > > + description: | > > > > + Indicates that this bus hosts MCTP-over-I3C target devices. > > > > > > I have doubts you actually tested it - there is no type/ref. Also, > > > your description is a bit different than existing from dtschema. Why? > > > Aren't these the same things? > > > > Ah, I'll add > > $ref: /schemas/types.yaml#/definitions/flag > > Although does not matter, but use the same as in dtschema. > type: boolean OK, thanks. > > For the description, do you mean it differs to the other properties in > > i3c.yaml, or something else? > > It differs than existing mctp-controller property. If this was on > purpose, please share a bit more why. If not, maybe use the same > description? The mctp-controller property has the same meaning as for I2C, so I'll use the existing I2C text for I3C as well. That will also be more suitable if in future Linux works as an I3C target device (currently it's controller only). "indicates that the system is accessible via this bus as an endpoint for MCTP over I3C transport." Thanks, Matt
Hi Andrew, On Mon, 2023-07-03 at 16:43 +0200, Andrew Lunn wrote: > > +#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. > > +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? > > > +/* 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. > > > +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/ checkpatch noticed this one too, but the existing I3C functions return ENOTSUPP so it needs to match against that. > > +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() ? > > > +/* 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
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. A remote endpoint has been tested against Qemu, as well as using the target mode support in Aspeed's vendor tree. --- I'll leave it to maintainers whether this should be merged through the i3c or net tree. Since my previous RFC email to the i3c list, this adds dt-bindings and fixes a comment typo. Jeremy Kerr (1): i3c: Add support for bus enumeration & notification Matt Johnston (2): dt-bindings: i3c: Add mctp-controller property mctp i3c: MCTP I3C driver .../devicetree/bindings/i3c/i3c.yaml | 4 + 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 + 6 files changed, 838 insertions(+) create mode 100644 drivers/net/mctp/mctp-i3c.c