[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <13e122213157b0f8cae1d90de3f558279a6ca068.camel@codeconstruct.com.au>
Date: Fri, 28 Jun 2024 16:41:42 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: admiyo@...amperecomputing.com, Matt Johnston
<matt@...econstruct.com.au>, "David S. Miller" <davem@...emloft.net>, Eric
Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport
Hi Adam,
Looking good, just a couple of minor things inline, and a query on the
new _remove() implementation.
> +#define SPDM_VERSION_OFFSET 1
> +#define SPDM_REQ_RESP_OFFSET 2
These seem unrelated, and are unused?
> +#define MCTP_PAYLOAD_LENGTH 256
> +#define MCTP_CMD_LENGTH 4
> +#define MCTP_PCC_VERSION 0x1 /* DSP0253 defines a single version: 1 */
> +#define MCTP_SIGNATURE "MCTP"
> +#define SIGNATURE_LENGTH 4
> +#define MCTP_HEADER_LENGTH 12
> +#define MCTP_MIN_MTU 68
> +#define PCC_MAGIC 0x50434300
> +#define PCC_HEADER_FLAG_REQ_INT 0x1
> +#define PCC_HEADER_FLAGS PCC_HEADER_FLAG_REQ_INT
> +#define PCC_DWORD_TYPE 0x0c
> +#define PCC_ACK_FLAG_MASK 0x1
> +
> +struct mctp_pcc_hdr {
> + u32 signature;
> + u32 flags;
> + u32 length;
> + char mctp_signature[4];
> +};
I'd recommend the endian-annotated types for signature, flags & length
here, and converting on access.
(yes, maybe this will only ever be intended for little-endian, but
there's almost always cases where code gets moved around, copied into
new drivers, etc)
> +struct mctp_pcc_hw_addr {
> + u32 inbox_index;
> + u32 outbox_index;
> +};
> +
> +/* The netdev structure. One of these per PCC adapter. */
> +struct mctp_pcc_ndev {
> + struct list_head next;
> + /* spinlock to serialize access to pcc buffer and registers*/
> + spinlock_t lock;
> + struct mctp_dev mdev;
> + struct acpi_device *acpi_device;
> + struct pcc_mbox_chan *in_chan;
> + struct pcc_mbox_chan *out_chan;
> + struct mbox_client outbox_client;
> + struct mbox_client inbox_client;
> + void __iomem *pcc_comm_inbox_addr;
> + void __iomem *pcc_comm_outbox_addr;
> + struct mctp_pcc_hw_addr hw_addr;
> +};
> +
> +static struct list_head mctp_pcc_ndevs = LIST_HEAD_INIT(mctp_pcc_ndevs);
Saw your changelog entry about needing this, makes sense if that's what
the acpi core requires.
In which case, you can make this a little more brief with:
static LIST_HEAD(mctp_pcc_ndevs);
(but more on that below)
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev;
> + struct mctp_pcc_hdr mctp_pcc_hdr;
> + struct mctp_skb_cb *cb;
> + struct sk_buff *skb;
> + void *skb_buf;
> + u32 data_len;
> + u32 flags;
> +
> + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> + memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr,
> + sizeof(struct mctp_pcc_hdr));
> + data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
> +
> + if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) {
> + mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> + return;
> + }
> +
> + skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len);
> + if (!skb) {
> + mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> + return;
> + }
> + mctp_pcc_dev->mdev.dev->stats.rx_packets++;
> + mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len;
> + skb->protocol = htons(ETH_P_MCTP);
> + skb_buf = skb_put(skb, data_len);
> + memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len);
> + skb_reset_mac_header(skb);
> + skb_pull(skb, sizeof(struct mctp_pcc_hdr));
> + skb_reset_network_header(skb);
> + cb = __mctp_cb(skb);
> + cb->halen = 0;
> + netif_rx(skb);
> +
> + flags = mctp_pcc_hdr.flags;
> + mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0;
The '>' comparison to a bit field is a bit odd, you could just:
mctp_pcc_dev->in_chan->ack_rx = !!(flags & PCC_ACK_FLAG_MASK);
> +static void
> +mctp_pcc_net_stats(struct net_device *net_dev,
> + struct rtnl_link_stats64 *stats)
> +{
> + struct mctp_pcc_ndev *mpnd;
> +
> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(net_dev);
No need to cast from void *.
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> + struct lookup_context context = {0, 0, 0};
> + struct mctp_pcc_ndev *mctp_pcc_dev;
> + struct net_device *ndev;
> + acpi_handle dev_handle;
> + acpi_status status;
> + struct device *dev;
> + int mctp_pcc_mtu;
> + int outbox_index;
> + int inbox_index;
> + char name[32];
> + int rc;
> +
> + dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
> + acpi_device_hid(acpi_dev));
> + dev_handle = acpi_device_handle(acpi_dev);
> + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
> + &context);
> + if (!ACPI_SUCCESS(status)) {
> + dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
> + return -EINVAL;
> + }
> + inbox_index = context.inbox_index;
> + outbox_index = context.outbox_index;
> + dev = &acpi_dev->dev;
> +
> + snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
> + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
> + mctp_pcc_setup);
> + if (!ndev)
> + return -ENOMEM;
> + mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev);
No need for the cast from void *.
> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> + struct list_head *ptr;
> + struct list_head *tmp;
> +
> + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
You can save the list_entry() below by using list_for_each_entry_safe()
here.
> + struct net_device *ndev;
> + struct mctp_pcc_ndev *mctp_pcc_dev;
> +
> + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> + if (mctp_pcc_dev->acpi_device != adev)
> + continue;
Wait, you removed the case where you clean up the whole list if adev is
NULL?
Now this contradicts your section of the changelog:
- Did not change the module init unload function to use the
ACPI equivalent as they do not remove all devices prior
to unload, leading to dangling references and seg faults.
Does this mean you no longer need to free all instances on unload? In
which case, that sounds great! that probably also means:
- all the list is doing is allowing you to find the mctp_pcc_dev from
the acpi_device
- but: you can turn an acpi_device into a mctp_pcc_dev from
adev->driver_data (which you're already setting), so you don't need
the list
- then, _remove just becomes something like:
static void mctp_pcc_driver_remove(struct acpi_device *adev)
{
struct mctp_pcc_dev *dev = acpi_driver_data(adev);
pcc_mbox_free_channel(dev->out_chan);
pcc_mbox_free_channel(dev->in_chan);
if (dev->mdev.dev)
mctp_unregister_netdev(dev->mdev.dev);
}
(but I can't see how dev->mdev.dev can be null, so you may be able
to remove the condition too)
> + pcc_mbox_free_channel(mctp_pcc_dev->out_chan);
> + pcc_mbox_free_channel(mctp_pcc_dev->in_chan);
> + ndev = mctp_pcc_dev->mdev.dev;
> + if (ndev)
> + mctp_unregister_netdev(ndev);
> + list_del(ptr);
> + break;
Unusual indent here.
Cheers,
Jeremy
Powered by blists - more mailing lists