Date: Wed, 29 May 2024 11:02:52 +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 v2 3/3] mctp pcc: Implement MCTP over PCC Transport

Hi Adam,

Thanks for the v2! Progress looks good, this seems simpler now too. Some
comments inline.

> From: Adam Young <admiyo@...erecomputing.com>
> 
> Implementation of DMTF DSP:0292
> Management Control Transport Protocol(MCTP)  over
> Platform Communication Channel(PCC)
> 
> MCTP devices are specified by entries in DSDT/SDST and
> reference channels specified in the PCCT.
> 
> Communication with other devices use the PCC based
> doorbell mechanism.

Signed-off-by?

And can you include a brief summary of changes since the prior version
you have sent? (see
https://lore.kernel.org/netdev/20240220081053.1439104-1-jk@codeconstruct.com.au/
for an example, the marker lines means that the changes don't get
included in the commit log; Jakub may also have other preferences around
this...)

> diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
> index ce9d2d2ccf3b..ff4effd8e99c 100644
> --- a/drivers/net/mctp/Kconfig
> +++ b/drivers/net/mctp/Kconfig
> @@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C
>           A MCTP protocol network device is created for each I3C bus
>           having a "mctp-controller" devicetree property.
>  
> +config MCTP_TRANSPORT_PCC
> +       tristate "MCTP  PCC transport"

Super minor: you have two spaces between "MCTP" and "PCC"

> +       select ACPI
> +       help
> +         Provides a driver to access MCTP devices over PCC transport,
> +         A MCTP protocol network device is created via ACPI for each
> +         entry in the DST/SDST that matches the identifier. The Platform
> +         commuinucation channels are selected from the corresponding
> +         entries in the PCCT.
> +
> +         Say y here if you need to connect to MCTP endpoints over PCC. To
> +         compile as a module, use m; the module will be called mctp-pcc.
> +
>  endmenu
>  
>  endif
> diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
> index e1cb99ced54a..492a9e47638f 100644
> --- a/drivers/net/mctp/Makefile
> +++ b/drivers/net/mctp/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o
>  obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
>  obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
>  obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
> new file mode 100644
> index 000000000000..d97f40789fd8
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,361 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mctp-pcc.c - Driver for MCTP over PCC.
> + * Copyright (c) 2024, Ampere Computing LLC
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/if_arp.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +#include <acpi/acrestyp.h>
> +#include <acpi/actbl.h>
> +#include <net/mctp.h>
> +#include <net/mctpdevice.h>
> +#include <acpi/pcc.h>
> +#include <net/pkt_sched.h>
> +
> +#define SPDM_VERSION_OFFSET 1
> +#define SPDM_REQ_RESP_OFFSET 2
> +#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_DWORD_TYPE 0x0c
> +
> +struct mctp_pcc_hdr {
> +       u32 signature;
> +       u32 flags;
> +       u32 length;
> +       char mctp_signature[4];
> +};

The usage of this struct isn't really consistent; you'll at least want
endian annotations on these members. More on that below though.

> +
> +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;
> +       void (*cleanup_channel)(struct pcc_mbox_chan *in_chan);

Why this as a callback? There's only one possible function it can be.

> +};
> +
> +static struct list_head mctp_pcc_ndevs;

I'm not clear on what this list is doing; it seems to be for freeing
devices on module unload (or device remove).

However, the module will be refcounted while there are devices bound, so
module unload shouldn't be possible in that state. So the only time
you'll be iterating this list to free everything will be when it's
empty.

You could replace this with the mctp_pcc_driver_remove() just removing the
device passed in the argument, rather than doing any list iteration.

... unless I've missed something?


> +
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_dev;
> +       struct mctp_skb_cb *cb;
> +       struct sk_buff *skb;
> +       u32 length_offset;
> +       u32 flags_offset;
> +       void *skb_buf;
> +       u32 data_len;
> +       u32 flags;
> +
> +       mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> +       length_offset = offsetof(struct mctp_pcc_hdr, length);
> +       data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
> +                  MCTP_HEADER_LENGTH;

Doing this using offsetof with separate readl()s is a bit clunky. Can
you memcpy_fromio the whole header, and use the appropriate endian
accessors?

(this would match the behaviour in the tx path)

Also, maybe check that data_len is sensible before allocating.

> +
> +       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));

Any benefit in including the pcc_hdr in the skb?

(not necessarily an issue, just asking...)

> +       skb_reset_network_header(skb);
> +       cb = __mctp_cb(skb);
> +       cb->halen = 0;
> +       skb->dev =  mctp_pcc_dev->mdev.dev;
> +       netif_rx(skb);
> +
> +       flags_offset = offsetof(struct mctp_pcc_hdr, flags);
> +       flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
> +       mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 0;

Might be best to define what the flags bits mean, rather than
magic-numbering this.

Does anything need to tell the mailbox driver to do that ack after
setting ack_rx?

> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +       struct mctp_pcc_hdr pcc_header;
> +       struct mctp_pcc_ndev *mpnd;
> +       void __iomem *buffer;
> +       unsigned long flags;
> +       int rc;
> +
> +       netif_stop_queue(ndev);

Do you need to stop and restart the queue? Your handling is atomic.

> +       ndev->stats.tx_bytes += skb->len;
> +       ndev->stats.tx_packets++;
> +       mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);

no need for this cast, netdev_priv() returns void *

> +
> +       spin_lock_irqsave(&mpnd->lock, flags);
> +       buffer = mpnd->pcc_comm_outbox_addr;
> +       pcc_header.signature = PCC_MAGIC;
> +       pcc_header.flags = 0x1;

Magic numbers for flags here too

> +       memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
> +       pcc_header.length = skb->len + SIGNATURE_LENGTH;
> +       memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
> +       memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
> +       rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
> +                                                        NULL);
> +       spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> +       dev_consume_skb_any(skb);
> +       netif_start_queue(ndev);
> +       if (!rc)
> +               return NETDEV_TX_OK;
> +       return NETDEV_TX_BUSY;

I think you want to return NETDEV_TX_OK unconditionally here, or at
least you need to change the queue handling; see the comment for the
ndo_start_xmit callback.

> +}
> +
> +static const struct net_device_ops mctp_pcc_netdev_ops = {
> +       .ndo_start_xmit = mctp_pcc_tx,
> +       .ndo_uninit = NULL

No need for this assignment.

> +};
> +
> +static void  mctp_pcc_setup(struct net_device *ndev)
> +{
> +       ndev->type = ARPHRD_MCTP;
> +       ndev->hard_header_len = 0;
> +       ndev->addr_len = 0;
> +       ndev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> +       ndev->flags = IFF_NOARP;
> +       ndev->netdev_ops = &mctp_pcc_netdev_ops;
> +       ndev->needs_free_netdev = true;
> +}
> +
> +static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
> +                                 struct device *dev, int inbox_index,
> +                                 int outbox_index)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_dev;
> +       struct net_device *ndev;
> +       int mctp_pcc_mtu;
> +       char name[32];
> +       int rc;
> +
> +       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);
> +       INIT_LIST_HEAD(&mctp_pcc_dev->next);
> +       spin_lock_init(&mctp_pcc_dev->lock);
> +
> +       mctp_pcc_dev->hw_addr.inbox_index = inbox_index;
> +       mctp_pcc_dev->hw_addr.outbox_index = outbox_index;
> +       mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback;
> +       mctp_pcc_dev->cleanup_channel = pcc_mbox_free_channel;
> +       mctp_pcc_dev->out_chan =
> +               pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client,
> +                                        outbox_index);
> +       if (IS_ERR(mctp_pcc_dev->out_chan)) {
> +               rc = PTR_ERR(mctp_pcc_dev->out_chan);
> +               goto free_netdev;
> +       }
> +       mctp_pcc_dev->in_chan =
> +               pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client,
> +                                        inbox_index);
> +       if (IS_ERR(mctp_pcc_dev->in_chan)) {
> +               rc = PTR_ERR(mctp_pcc_dev->in_chan);
> +               goto cleanup_out_channel;
> +       }
> +       mctp_pcc_dev->pcc_comm_inbox_addr =
> +               devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
> +                            mctp_pcc_dev->in_chan->shmem_size);
> +       if (!mctp_pcc_dev->pcc_comm_inbox_addr) {
> +               rc = -EINVAL;
> +               goto cleanup_in_channel;
> +       }
> +       mctp_pcc_dev->pcc_comm_outbox_addr =
> +               devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
> +                            mctp_pcc_dev->out_chan->shmem_size);
> +       if (!mctp_pcc_dev->pcc_comm_outbox_addr) {
> +               rc = -EINVAL;
> +               goto cleanup_in_channel;
> +       }
> +       mctp_pcc_dev->acpi_device = acpi_dev;

You probably want the link back too:

          acpi_dev->driver_data = mctp_pcc_dev;


> +       mctp_pcc_dev->inbox_client.dev = dev;
> +       mctp_pcc_dev->outbox_client.dev = dev;
> +       mctp_pcc_dev->mdev.dev = ndev;
> +
> +/* There is no clean way to pass the MTU to the callback function
> + * used for registration, so set the values ahead of time.
> + */

Super minor, but keep this aligned with the code indent.

> +       mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size -
> +               sizeof(struct mctp_pcc_hdr);
> +       ndev->mtu = mctp_pcc_mtu;
> +       ndev->max_mtu = mctp_pcc_mtu;
> +       ndev->min_mtu = MCTP_MIN_MTU;

Same as last review: I'd recommend setting the MTU to the minimum, and
leaving it up to userspace to do a `mctp link mctppcc0 mtu <whatever>`.
This means you don't break things if/when you ever encounter remote
endpoints that don't support the max mtu.

If the driver could reliably detect the remote max MTU, then what you
have is fine (and the driver would then set the appropriate MTU
here).

However, if that probing *cannot* be done at the driver level, or needs
any userspace interaction to do so (say, a higher level protocol), then
you've set a MTU that could be unusable in this initial state. In that
case, setting it low to start with means that the link is usable by
default.

Userspace can then hard-code a higher MTU if you like, and that can be
adjusted later as appropriate.

(this is a lesson learnt from the i2c transport...)

> +/* pass in adev=NULL to remove all devices
> + */
> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
> +       struct list_head *ptr;
> +       struct list_head *tmp;
> +
> +       list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
> +               struct net_device *ndev;
> +
> +               mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> +               if (adev && mctp_pcc_dev->acpi_device == adev)
> +                       continue;
> +
> +               mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
> +               mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
> +               ndev = mctp_pcc_dev->mdev.dev;
> +               if (ndev)
> +                       mctp_unregister_netdev(ndev);
> +               list_del(ptr);
> +               if (adev)
> +                       break;
> +       }
> +};

Assuming we don't need the free-everything case, how about something
like:

    static void mctp_pcc_driver_remove(struct acpi_device *adev)
    {
          struct mctp_pcc_ndev *mctp_pcc_dev = acpi_driver_data(adev);

          mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
          mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
          mctp_unregister_netdev(mctp_pcc_dev->mdev.dev);
     }

If you do need the list iteration: you're currently doing the cleanup on
every adev *except* the one you want:

> +               if (adev && mctp_pcc_dev->acpi_device == adev)
> +                       continue;

I think you meant '!=' instead of '=='?

> +
> +static const struct acpi_device_id mctp_pcc_device_ids[] = {
> +       { "DMT0001", 0},
> +       { "", 0},
> +};
> +
> +static struct acpi_driver mctp_pcc_driver = {
> +       .name = "mctp_pcc",
> +       .class = "Unknown",
> +       .ids = mctp_pcc_device_ids,
> +       .ops = {
> +               .add = mctp_pcc_driver_add,
> +               .remove = mctp_pcc_driver_remove,
> +               .notify = NULL,

Minor: don't need the zero assignment here.

> +       },
> +       .owner = THIS_MODULE,
> +
> +};
> +

[...]

> +static int __init mctp_pcc_mod_init(void)
> +{
> +       int rc;
> +
> +       pr_debug("Initializing MCTP over PCC transport driver\n");
> +       INIT_LIST_HEAD(&mctp_pcc_ndevs);
> +       rc = acpi_bus_register_driver(&mctp_pcc_driver);
> +       if (rc < 0)
> +               ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
> +       return rc;
> +}
> +
> +static __exit void mctp_pcc_mod_exit(void)
> +{
> +       pr_debug("Removing MCTP over PCC transport driver\n");
> +       mctp_pcc_driver_remove(NULL);
> +       acpi_bus_unregister_driver(&mctp_pcc_driver);
> +}
> +
> +module_init(mctp_pcc_mod_init);
> +module_exit(mctp_pcc_mod_exit);

If you end up removing the mctp_pcc_ndevs list, these can all be
replaced with module_acpi_driver(mctp_pcc_driver);

Cheers,


Jeremy

Powered by blists - more mailing lists