| Message ID | 20240528191823.17775-4-admiyo@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MCTP over PCC | expand |
On Tue, 28 May 2024 15:18:23 -0400 admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@amperecomputing.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. Missing your SoB, but please wait for more feedback before reposting. > +#include <net/pkt_sched.h> Hm, what do you need this include for? > +#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 Could you align the values using tabs? > +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; > + > + 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; Please implement ndo_get_stats64, use of the core dev stats in drivers is deprecated: * @stats: Statistics struct, which was left as a legacy, use * rtnl_link_stats64 instead > + 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; > + skb->dev = mctp_pcc_dev->mdev.dev; netdev_alloc_skb() already sets 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; > +} > + > +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); Why? > + ndev->stats.tx_bytes += skb->len; > + ndev->stats.tx_packets++; > + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); > + > + spin_lock_irqsave(&mpnd->lock, flags); > + buffer = mpnd->pcc_comm_outbox_addr; > + pcc_header.signature = PCC_MAGIC; > + pcc_header.flags = 0x1; > + 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; > +} > + > +static const struct net_device_ops mctp_pcc_netdev_ops = { > + .ndo_start_xmit = mctp_pcc_tx, > + .ndo_uninit = NULL No need to init things to NULL > +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; > + } > +}; spurious ; > + .owner = THIS_MODULE, > + suprious new line > +}; > +
Hi Adam, Thanks for the v2! Progress looks good, this seems simpler now too. Some comments inline. > From: Adam Young <admiyo@amperecomputing.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
Hi Jakub, > > +#include <net/pkt_sched.h> > > Hm, what do you need this include for? I've used this in the past for DEFAULT_TX_QUEUE_LEN in existing mctp drivers, but looks like setting tx_queue_len = 0 will do the right thing... Cheers, Jeremy
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc1 next-20240529]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240529-072116
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240528191823.17775-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240529/202405292136.ZyuCa1Fc-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405292136.ZyuCa1Fc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405292136.ZyuCa1Fc-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/net/mctp/mctp-pcc.c:331:10: error: 'struct acpi_driver' has no member named 'owner'
331 | .owner = THIS_MODULE,
| ^~~~~
In file included from include/linux/printk.h:6,
from include/asm-generic/bug.h:22,
from arch/loongarch/include/asm/bug.h:60,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/loongarch/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:79,
from include/linux/spinlock.h:56,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from drivers/net/mctp/mctp-pcc.c:7:
>> include/linux/init.h:180:21: error: initialization of 'const char *' from incompatible pointer type 'struct module *' [-Werror=incompatible-pointer-types]
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:331:18: note: in expansion of macro 'THIS_MODULE'
331 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
include/linux/init.h:180:21: note: (near initialization for 'mctp_pcc_driver.drv.name')
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:331:18: note: in expansion of macro 'THIS_MODULE'
331 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:322:45: warning: missing braces around initializer [-Wmissing-braces]
322 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_init':
drivers/net/mctp/mctp-pcc.c:343:80: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
343 | ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
| ^
cc1: some warnings being treated as errors
vim +180 include/linux/init.h
f2511774863487e Arjan van de Ven 2009-12-13 177
5b20755b7780464 Masahiro Yamada 2023-11-26 178 #ifdef MODULE
5b20755b7780464 Masahiro Yamada 2023-11-26 179 extern struct module __this_module;
5b20755b7780464 Masahiro Yamada 2023-11-26 @180 #define THIS_MODULE (&__this_module)
5b20755b7780464 Masahiro Yamada 2023-11-26 181 #else
5b20755b7780464 Masahiro Yamada 2023-11-26 182 #define THIS_MODULE ((struct module *)0)
5b20755b7780464 Masahiro Yamada 2023-11-26 183 #endif
5b20755b7780464 Masahiro Yamada 2023-11-26 184
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc1 next-20240529]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240529-072116
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240528191823.17775-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240529/202405292134.LNr25Q9s-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405292134.LNr25Q9s-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405292134.LNr25Q9s-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:173:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
500 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
501 | item];
| ~~~~
include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
507 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
508 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
519 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
520 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
528 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
529 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/net/mctp/mctp-pcc.c:8:
In file included from include/linux/if_arp.h:22:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from drivers/net/mctp/mctp-pcc.c:8:
In file included from include/linux/if_arp.h:22:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from drivers/net/mctp/mctp-pcc.c:8:
In file included from include/linux/if_arp.h:22:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from drivers/net/mctp/mctp-pcc.c:17:
include/acpi/acpi_drivers.h:72:43: warning: declaration of 'struct acpi_pci_root' will not be visible outside of this function [-Wvisibility]
72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
| ^
>> drivers/net/mctp/mctp-pcc.c:193:3: error: call to undeclared function 'devm_ioremap'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
193 | devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
| ^
>> drivers/net/mctp/mctp-pcc.c:192:36: error: incompatible integer to pointer conversion assigning to 'void *' from 'int' [-Wint-conversion]
192 | mctp_pcc_dev->pcc_comm_inbox_addr =
| ^
193 | devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
194 | mctp_pcc_dev->in_chan->shmem_size);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:199:37: error: incompatible integer to pointer conversion assigning to 'void *' from 'int' [-Wint-conversion]
199 | mctp_pcc_dev->pcc_comm_outbox_addr =
| ^
200 | devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
201 | mctp_pcc_dev->out_chan->shmem_size);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:276:15: error: incomplete definition of type 'struct acpi_device'
276 | dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n",
| ~~~~^
include/linux/dev_printk.h:165:18: note: expanded from macro 'dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:274:7: note: expanded from macro 'dynamic_dev_dbg'
274 | dev, fmt, ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:250:59: note: expanded from macro '_dynamic_func_call'
250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:248:65: note: expanded from macro '_dynamic_func_call_cls'
248 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:224:15: note: expanded from macro '__dynamic_func_call_cls'
224 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/acpi.h:794:8: note: forward declaration of 'struct acpi_device'
794 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:277:3: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
277 | acpi_device_hid(adev));
| ^
drivers/net/mctp/mctp-pcc.c:277:3: note: did you mean 'acpi_device_dep'?
include/acpi/acpi_bus.h:41:6: note: 'acpi_device_dep' declared here
41 | bool acpi_device_dep(acpi_handle target, acpi_handle match);
| ^
drivers/net/mctp/mctp-pcc.c:278:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
278 | dev_handle = acpi_device_handle(adev);
| ^
drivers/net/mctp/mctp-pcc.c:278:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion]
278 | dev_handle = acpi_device_handle(adev);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:282:16: error: incomplete definition of type 'struct acpi_device'
282 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS");
| ~~~~^
include/linux/dev_printk.h:154:44: note: expanded from macro 'dev_err'
154 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/acpi.h:794:8: note: forward declaration of 'struct acpi_device'
794 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:287:43: error: incomplete definition of type 'struct acpi_device'
287 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index,
| ~~~~^
include/linux/acpi.h:794:8: note: forward declaration of 'struct acpi_device'
794 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:322:27: error: variable has incomplete type 'struct acpi_driver'
322 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:322:15: note: forward declaration of 'struct acpi_driver'
322 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:341:7: error: call to undeclared function 'acpi_bus_register_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
341 | rc = acpi_bus_register_driver(&mctp_pcc_driver);
| ^
drivers/net/mctp/mctp-pcc.c:351:2: error: call to undeclared function 'acpi_bus_unregister_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
351 | acpi_bus_unregister_driver(&mctp_pcc_driver);
| ^
18 warnings and 12 errors generated.
vim +/devm_ioremap +193 drivers/net/mctp/mctp-pcc.c
154
155 static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
156 struct device *dev, int inbox_index,
157 int outbox_index)
158 {
159 struct mctp_pcc_ndev *mctp_pcc_dev;
160 struct net_device *ndev;
161 int mctp_pcc_mtu;
162 char name[32];
163 int rc;
164
165 snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
166 ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
167 mctp_pcc_setup);
168 if (!ndev)
169 return -ENOMEM;
170 mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev);
171 INIT_LIST_HEAD(&mctp_pcc_dev->next);
172 spin_lock_init(&mctp_pcc_dev->lock);
173
174 mctp_pcc_dev->hw_addr.inbox_index = inbox_index;
175 mctp_pcc_dev->hw_addr.outbox_index = outbox_index;
176 mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback;
177 mctp_pcc_dev->cleanup_channel = pcc_mbox_free_channel;
178 mctp_pcc_dev->out_chan =
179 pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client,
180 outbox_index);
181 if (IS_ERR(mctp_pcc_dev->out_chan)) {
182 rc = PTR_ERR(mctp_pcc_dev->out_chan);
183 goto free_netdev;
184 }
185 mctp_pcc_dev->in_chan =
186 pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client,
187 inbox_index);
188 if (IS_ERR(mctp_pcc_dev->in_chan)) {
189 rc = PTR_ERR(mctp_pcc_dev->in_chan);
190 goto cleanup_out_channel;
191 }
> 192 mctp_pcc_dev->pcc_comm_inbox_addr =
> 193 devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
194 mctp_pcc_dev->in_chan->shmem_size);
195 if (!mctp_pcc_dev->pcc_comm_inbox_addr) {
196 rc = -EINVAL;
197 goto cleanup_in_channel;
198 }
199 mctp_pcc_dev->pcc_comm_outbox_addr =
200 devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
201 mctp_pcc_dev->out_chan->shmem_size);
202 if (!mctp_pcc_dev->pcc_comm_outbox_addr) {
203 rc = -EINVAL;
204 goto cleanup_in_channel;
205 }
206 mctp_pcc_dev->acpi_device = acpi_dev;
207 mctp_pcc_dev->inbox_client.dev = dev;
208 mctp_pcc_dev->outbox_client.dev = dev;
209 mctp_pcc_dev->mdev.dev = ndev;
210
On 5/28/24 22:45, Jakub Kicinski wrote: > On Tue, 28 May 2024 15:18:23 -0400 admiyo@os.amperecomputing.com wrote: >> From: Adam Young <admiyo@amperecomputing.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. > Missing your SoB, but please wait for more feedback before reposting. Sorry, must have dropped it when redoing the last patch, It was on the original and will be on the next version. > >> +#include <net/pkt_sched.h> > Hm, what do you need this include for? Gets the symbolic constant > >> +#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 > Could you align the values using tabs? Will do > >> +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; >> + >> + 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; > Please implement ndo_get_stats64, use of the core dev stats in drivers > is deprecated: > > * @stats: Statistics struct, which was left as a legacy, use > * rtnl_link_stats64 instead Thanks. Was not aware. The other MCTP drivers need this as well. > >> + 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; >> + skb->dev = mctp_pcc_dev->mdev.dev; > netdev_alloc_skb() already sets 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; >> +} >> + >> +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); > Why? I saw this in other network drivers, both Ethernet and MCTP. As I understand it, without stopping the queue, we could take another packet before this one is sent and we only have one buffer; that said, it should be protected by the spin lock. I am tempted to leave this in here, but move all of the statistics into the spin lock. Is there a reason to not stop the queue? > >> + ndev->stats.tx_bytes += skb->len; >> + ndev->stats.tx_packets++; >> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); >> + >> + spin_lock_irqsave(&mpnd->lock, flags); >> + buffer = mpnd->pcc_comm_outbox_addr; >> + pcc_header.signature = PCC_MAGIC; >> + pcc_header.flags = 0x1; >> + 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; >> +} >> + >> +static const struct net_device_ops mctp_pcc_netdev_ops = { >> + .ndo_start_xmit = mctp_pcc_tx, >> + .ndo_uninit = NULL > No need to init things to NULL > >> +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; >> + } >> +}; > spurious ; > > >> + .owner = THIS_MODULE, >> + > suprious new line > >> +}; >> +
Pretty sure I have all these corrections in the next one. Some inline comments. On 5/28/24 23:02, Jeremy Kerr wrote: > Hi Adam, > > Thanks for the v2! Progress looks good, this seems simpler now too. Some > comments inline. > >> From: Adam Young <admiyo@amperecomputing.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...) They are all in the header patch. > >> 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. Vestige of code designed to work with multiple mailbox implementations. Will remove. > >> +}; >> + >> +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? There is no requirement that all the devices be unloaded in order for the module to get unloaded. It someone wants to disable the MCTP devices, they can unload the module, and it gets cleaned up. With ACPI, the devices never go away, they are defined in a table read at start up and stay there. So without this change there is no way to unload the module. Maybe it is just a convenience for development, but I think most modules behave this way. > > >> + >> +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? I was overthinking this. You are right. > > (this would match the behaviour in the tx path) > > Also, maybe check that data_len is sensible before allocating. Good idea > >> + >> + 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...) It shows up in tracing of the packet. Useful for debugging. > >> + 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. I'll make a constant for the mask. > > Does anything need to tell the mailbox driver to do that ack after > setting ack_rx? Yes. It is in the previous patch, in the pcc_mailbox code. I originally had it as a follow on, but reordered to make it a pre-req. That allows me to inline this logic, making the driver easier to review (I hope). > >> +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. I guess not. This was just from following the examples of others. Will remove. > >> + 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. Will Do. Thanks for pointing out that comment, would never have seen it. > >> +} >> + >> +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...) Ah...makes sense. I was not sure if our impl,enetation could handle that yet, but it appears to, If it can't, it is a bug we have to fix. > >> +/* 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 '=='? Yes. Yes I did. This is code that has to be there for completeness, but I don't really have a way to test, except for the "delete all" case. > >> + >> +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); Yeah, I can't get away with that. The ACPI devices may still be there when some one calls rmmod, and so we need to clean up the ndevs. It is the only case where the module will be removed, as the ACPI devices never go away. > > Cheers, > > > Jeremy
Hi Adam, > > And can you include a brief summary of changes since the prior version > > you have sent? > > > They are all in the header patch. Ah, neat. Can you include reviewers on CC for that 0/n patch then? > > > +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? > > There is no requirement that all the devices be unloaded in order for > the module to get unloaded. ... aside from the driver refcounting. You're essentially replicating the driver core's own facility for device-to-driver mappings here. > It someone wants to disable the MCTP devices, they can unload the > module, and it gets cleaned up. > > With ACPI, the devices never go away, they are defined in a table read > at start up and stay there. Sure, the ACPI bus devices may always be present, but you can still unbind the driver from one device: echo '<device-id>' > /sys/bus/acpi/drivers/mctp_pcc/unbind - where device-id is one of the links to a device in that mctp_pcc dir. then: > So without this change there is no way to unload the module. ... with no devices bound, you can safely unload the module (but the unload path will also perform that unbind anyway, more on that below). > Maybe it is just a convenience for development, but I think most > modules behave this way. If you can avoid holding internal references to devices, you have a whole class of bugs you can avoid. > > Any benefit in including the pcc_hdr in the skb? > > > > (not necessarily an issue, just asking...) > It shows up in tracing of the packet. Useful for debugging. Sounds good! > > Does anything need to tell the mailbox driver to do that ack after > > setting ack_rx? > > Yes. It is in the previous patch, in the pcc_mailbox code. I > originally had it as a follow on, but reordered to make it a pre-req. > That allows me to inline this logic, making the driver easier to review > (I hope). OK. As far as I can tell here this is just setting a member of the mailbox interface, but not calling back into the mailbox code. If this is okay, then all good. > > > + netif_stop_queue(ndev); > > Do you need to stop and restart the queue? Your handling is atomic. > I guess not. This was just from following the examples of others. Will > remove. Those examples (at least, in the MCTP drivers) will not have been able to complete transmission until way later - say, after a separate completion, or after a separate thread has processed the outgoing skb. While that is happening, we may have stopped the queue. In your case, you complete transmission entirely within the start_xmit operation (*and* that path is atomic), so the queue is fine to remain enabled. > > > + if (adev && mctp_pcc_dev->acpi_device == adev) > > > + continue; > > I think you meant '!=' instead of '=='? > Yes. Yes I did. This is code that has to be there for completeness, > but I don't really have a way to test, except for the "delete all" case. The 'unbind' example above will test this. > > > +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); > > Yeah, I can't get away with that. The ACPI devices may still be there > when some one calls rmmod, and so we need to clean up the ndevs. The core driver unregister path should unbind all devices before the module is removed, so your mctp_pcc_driver_remove() should get invoked on each individual device during that process. (with the above != vs. == bug fixed, you'll probably find that "delete-all" case will always hit an empty list) ... this is unless something is different about the ACPI bus type, but it all looks standard from a brief look! Cheers, Jeremy
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" + 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]; +}; + +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); +}; + +static struct list_head mctp_pcc_ndevs; + +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; + + 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; + 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; +} + +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); + ndev->stats.tx_bytes += skb->len; + ndev->stats.tx_packets++; + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); + + spin_lock_irqsave(&mpnd->lock, flags); + buffer = mpnd->pcc_comm_outbox_addr; + pcc_header.signature = PCC_MAGIC; + pcc_header.flags = 0x1; + 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; +} + +static const struct net_device_ops mctp_pcc_netdev_ops = { + .ndo_start_xmit = mctp_pcc_tx, + .ndo_uninit = NULL +}; + +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; + 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. + */ + 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; + + rc = register_netdev(ndev); + if (rc) + goto cleanup_in_channel; + list_add_tail(&mctp_pcc_dev->next, &mctp_pcc_ndevs); + return 0; + +cleanup_in_channel: + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan); +cleanup_out_channel: + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan); +free_netdev: + unregister_netdev(ndev); + free_netdev(ndev); + return rc; +} + +struct lookup_context { + int index; + u32 inbox_index; + u32 outbox_index; +}; + +static acpi_status lookup_pcct_indices(struct acpi_resource *ares, + void *context) +{ + struct acpi_resource_address32 *addr; + struct lookup_context *luc = context; + + switch (ares->type) { + case PCC_DWORD_TYPE: + break; + default: + return AE_OK; + } + + addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data); + switch (luc->index) { + case 0: + luc->outbox_index = addr[0].address.minimum; + break; + case 1: + luc->inbox_index = addr[0].address.minimum; + break; + } + luc->index++; + return AE_OK; +} + +static int mctp_pcc_driver_add(struct acpi_device *adev) +{ + int outbox_index; + int inbox_index; + acpi_handle dev_handle; + acpi_status status; + struct lookup_context context = {0, 0, 0}; + + dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n", + acpi_device_hid(adev)); + dev_handle = acpi_device_handle(adev); + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, + &context); + if (!ACPI_SUCCESS(status)) { + dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); + return -EINVAL; + } + inbox_index = context.inbox_index; + outbox_index = context.outbox_index; + return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, + outbox_index); +}; + +/* 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; + } +}; + +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, + }, + .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); + +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids); + +MODULE_DESCRIPTION("MCTP PCC device"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");
From: Adam Young <admiyo@amperecomputing.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. --- drivers/net/mctp/Kconfig | 13 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 361 ++++++++++++++++++++++++++++++++++++ 3 files changed, 375 insertions(+) create mode 100644 drivers/net/mctp/mctp-pcc.c