From: Matt Johnston <matt@codeconstruct.com.au>
To: Andrew Lunn <andrew@lunn.ch>
Cc: linux-i3c@lists.infradead.org, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Jeremy Kerr <jk@codeconstruct.com.au>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>
Subject: Re: [PATCH 3/3] mctp i3c: MCTP I3C driver
Date: Tue, 04 Jul 2023 14:49:11 +0800 [thread overview]
Message-ID: <a4c5090defc84625cbac5e16ed50dc8316dda755.camel@codeconstruct.com.au> (raw)
In-Reply-To: <8321002c-9b75-44da-9200-23d951148ae9@lunn.ch>
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
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
prev parent reply other threads:[~2023-07-04 6:49 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-03 5:30 [PATCH 0/3] I3C MCTP net driver Matt Johnston
2023-07-03 5:30 ` [PATCH 1/3] dt-bindings: i3c: Add mctp-controller property Matt Johnston
2023-07-03 7:15 ` Krzysztof Kozlowski
2023-07-03 8:03 ` Matt Johnston
2023-07-03 8:14 ` Matt Johnston
2023-07-03 14:16 ` Krzysztof Kozlowski
2023-07-04 6:34 ` Matt Johnston
2023-07-03 5:30 ` [PATCH 2/3] i3c: Add support for bus enumeration & notification Matt Johnston
2023-07-03 15:01 ` Krzysztof Kozlowski
2023-07-03 5:30 ` [PATCH 3/3] mctp i3c: MCTP I3C driver Matt Johnston
2023-07-03 11:48 ` David Miller
2023-07-03 14:43 ` Andrew Lunn
2023-07-04 6:49 ` Matt Johnston [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a4c5090defc84625cbac5e16ed50dc8316dda755.camel@codeconstruct.com.au \
--to=matt@codeconstruct.com.au \
--cc=alexandre.belloni@bootlin.com \
--cc=andrew@lunn.ch \
--cc=conor+dt@kernel.org \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=jk@codeconstruct.com.au \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kuba@kernel.org \
--cc=linux-i3c@lists.infradead.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).