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