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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 3 Jul 2023 16:43:47 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Matt Johnston <matt@...econstruct.com.au>
Cc: linux-i3c@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jeremy Kerr <jk@...econstruct.com.au>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH 3/3] mctp i3c: MCTP I3C driver

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

> +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.

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

> +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/


> +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()?

    Andrew