* [PATCH 0/3] MCTP over PCC
@ 2024-05-13 17:35 admiyo
2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo
` (4 more replies)
0 siblings, 5 replies; 43+ messages in thread
From: admiyo @ 2024-05-13 17:35 UTC (permalink / raw)
Cc: netdev, linux-kernel, Adam Young
From: Adam Young <admiyo@os.amperecomputing.com>
This series adds support for the Management Control Transport Protocol (MCTP)
over the Platform Communication Channel (PCC) mechanism.
MCTP defines a communication model intended to
facilitate communication between Management controllers
and other management controllers, and between Management
controllers and management devices
PCC is a mechanism for communication between components within
the Platform. It is a composed of shared memory regions,
interrupt registers, and status registers.
The MCTP over PCC driver makes use of two PCC channels. For
sending messages, it uses a Type 3 channel, and for receiving
messages it uses the paired Type 4 channel. The device
and its corresponding channels are specified via ACPI.
Adam Young (3):
mctp pcc: Implement MCTP over PCC Transport
mctp pcc: Allow PCC Data Type in MCTP resource.
mctp pcc: RFC Check before sending MCTP PCC response ACK
drivers/acpi/acpica/rsaddr.c | 2 +-
drivers/mailbox/pcc.c | 5 +-
drivers/net/mctp/Kconfig | 12 ++
drivers/net/mctp/Makefile | 1 +
drivers/net/mctp/mctp-pcc.c | 372 +++++++++++++++++++++++++++++++++++
include/acpi/pcc.h | 1 +
6 files changed, 391 insertions(+), 2 deletions(-)
create mode 100644 drivers/net/mctp/mctp-pcc.c
--
2.34.1
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 [PATCH 0/3] MCTP over PCC admiyo @ 2024-05-13 17:35 ` admiyo 2024-05-13 18:31 ` Simon Horman ` (9 more replies) 2024-05-13 17:35 ` [PATCH 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo ` (3 subsequent siblings) 4 siblings, 10 replies; 43+ messages in thread From: admiyo @ 2024-05-13 17:35 UTC (permalink / raw) To: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel, Adam Young From: Adam Young <admiyo@os.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: Adam Young <admiyo@os.amperecomputing.com> --- drivers/net/mctp/Kconfig | 12 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 368 ++++++++++++++++++++++++++++++++++++ 3 files changed, 381 insertions(+) create mode 100644 drivers/net/mctp/mctp-pcc.c diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig index ce9d2d2ccf3b..16bad87c9a6f 100644 --- a/drivers/net/mctp/Kconfig +++ b/drivers/net/mctp/Kconfig @@ -42,6 +42,18 @@ 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 MCTP_FLOWS + help + Provides a driver to access MCTP devices over PCC transport, + A MCTP protocol network device is created for each entry int the DST/SDST + that matches the identifier. The PCC 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..7242eedd2759 --- /dev/null +++ b/drivers/net/mctp/mctp-pcc.c @@ -0,0 +1,368 @@ +// 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 + +struct mctp_pcc_hdr { + u32 signature; + u32 flags; + u32 length; + char mctp_signature[4]; +}; + +struct mctp_pcc_packet { + struct mctp_pcc_hdr pcc_header; + union { + struct mctp_hdr mctp_header; + unsigned char header_data[sizeof(struct mctp_hdr)]; + }; + unsigned char payload[MCTP_PAYLOAD_LENGTH]; +}; + +struct mctp_pcc_hw_addr { + int inbox_index; + int outbox_index; +}; + +/* The netdev structure. One of these per PCC adapter. */ +struct mctp_pcc_ndev { + struct list_head head; + /* spinlock to serialize access from netdev to pcc buffer*/ + 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 struct mctp_pcc_packet *mctp_pcc_extract_data(struct sk_buff *old_skb, + void *buffer, int outbox_index) +{ + struct mctp_pcc_packet *mpp; + + mpp = buffer; + writel(PCC_MAGIC | outbox_index, &mpp->pcc_header.signature); + writel(0x1, &mpp->pcc_header.flags); + memcpy_toio(mpp->pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); + writel(old_skb->len + SIGNATURE_LENGTH, &mpp->pcc_header.length); + memcpy_toio(mpp->header_data, old_skb->data, old_skb->len); + return mpp; +} + +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) +{ + struct sk_buff *skb; + struct mctp_pcc_packet *mpp; + struct mctp_skb_cb *cb; + int data_len; + unsigned long buf_ptr_val; + struct mctp_pcc_ndev *mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); + void *skb_buf; + + mpp = (struct mctp_pcc_packet *)mctp_pcc_dev->pcc_comm_inbox_addr; + buf_ptr_val = (unsigned long)mpp; + data_len = readl(&mpp->pcc_header.length) + 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; + } + skb->protocol = htons(ETH_P_MCTP); + skb_buf = skb_put(skb, data_len); + memcpy_fromio(skb_buf, mpp, 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); +} + +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) +{ + unsigned char *buffer; + struct mctp_pcc_ndev *mpnd; + struct mctp_pcc_packet *mpp; + unsigned long flags; + int rc; + + netif_stop_queue(ndev); + ndev->stats.tx_bytes += skb->len; + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); + spin_lock_irqsave(&mpnd->lock, flags); + buffer = mpnd->pcc_comm_outbox_addr; + mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index); + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, mpp); + 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 = sizeof(struct mctp_pcc_hw_addr); + 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) +{ + int rc; + int mctp_pcc_mtu; + char name[32]; + struct net_device *ndev; + struct mctp_pcc_ndev *mctp_pcc_dev; + struct mctp_pcc_hw_addr physical_link_addr; + + snprintf(name, sizeof(name), "mctpipcc%x", 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->head); + spin_lock_init(&mctp_pcc_dev->lock); + + mctp_pcc_dev->outbox_client.tx_prepare = NULL; + mctp_pcc_dev->outbox_client.tx_done = NULL; + 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; + + physical_link_addr.inbox_index = + htonl(mctp_pcc_dev->hw_addr.inbox_index); + physical_link_addr.outbox_index = + htonl(mctp_pcc_dev->hw_addr.outbox_index); + dev_addr_set(ndev, (const u8 *)&physical_link_addr); + rc = register_netdev(ndev); + if (rc) + goto cleanup_in_channel; + list_add_tail(&mctp_pcc_dev->head, &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; + int inbox_index; + int 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 0x0c: + case 0x0a: + 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 inbox_index; + int outbox_index; + acpi_handle dev_handle; + acpi_status status; + struct lookup_context context = {0, 0, 0}; + + dev_info(&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)) { + inbox_index = context.inbox_index; + outbox_index = context.outbox_index; + return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); + } + dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); + return -EINVAL; +}; + +/* 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) { + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head); + if (!adev || mctp_pcc_dev->acpi_device == adev) { + struct net_device *ndev; + + 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_info("initializing MCTP over PCC\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_info("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>"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo @ 2024-05-13 18:31 ` Simon Horman 2024-05-13 20:08 ` Andrew Lunn ` (8 subsequent siblings) 9 siblings, 0 replies; 43+ messages in thread From: Simon Horman @ 2024-05-13 18:31 UTC (permalink / raw) To: admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel On Mon, May 13, 2024 at 01:35:44PM -0400, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@os.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: Adam Young <admiyo@os.amperecomputing.com> Hi Adam, Some minor feedback from my side. ... > +static struct mctp_pcc_packet *mctp_pcc_extract_data(struct sk_buff *old_skb, > + void *buffer, int outbox_index) > +{ > + struct mctp_pcc_packet *mpp; > + > + mpp = buffer; > + writel(PCC_MAGIC | outbox_index, &mpp->pcc_header.signature); > + writel(0x1, &mpp->pcc_header.flags); > + memcpy_toio(mpp->pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); > + writel(old_skb->len + SIGNATURE_LENGTH, &mpp->pcc_header.length); > + memcpy_toio(mpp->header_data, old_skb->data, old_skb->len); > + return mpp; > +} > + > +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) Please include a name for all function parameters. Flagged by W=1 builds. > +{ > + struct sk_buff *skb; > + struct mctp_pcc_packet *mpp; > + struct mctp_skb_cb *cb; > + int data_len; > + unsigned long buf_ptr_val; buf_ptr_val is assigned but otherwise unused. Flagged by W=1 builds. > + struct mctp_pcc_ndev *mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); > + void *skb_buf; For Networking code please consider: 1. Using reverse xmas tree order - longest line to shortest - for local variable declarations. This tool can be of assistance: https://github.com/ecree-solarflare/xmastree 2. Restricting lines to 80 columns wide where this can be trivially achieved. ./scripts/checkpatch.pl --max-line-length=80 In this case, perhaps: struct mctp_pcc_ndev *mctp_pcc_dev; struct mctp_pcc_packet *mpp; struct mctp_skb_cb *cb; struct sk_buff *skb; void *skb_buf; int data_len; mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); > + > + mpp = (struct mctp_pcc_packet *)mctp_pcc_dev->pcc_comm_inbox_addr; > + buf_ptr_val = (unsigned long)mpp; > + data_len = readl(&mpp->pcc_header.length) + 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; > + } > + skb->protocol = htons(ETH_P_MCTP); > + skb_buf = skb_put(skb, data_len); > + memcpy_fromio(skb_buf, mpp, 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); > +} > + > +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) > +{ > + unsigned char *buffer; > + struct mctp_pcc_ndev *mpnd; > + struct mctp_pcc_packet *mpp; > + unsigned long flags; > + int rc; > + > + netif_stop_queue(ndev); > + ndev->stats.tx_bytes += skb->len; > + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); > + spin_lock_irqsave(&mpnd->lock, flags); > + buffer = mpnd->pcc_comm_outbox_addr; buffer is assigned but otherwise unused in this function. Flagged by W=1 builds. > + mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index); > + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, mpp); > + 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; > +} ... -- pw-bot: changes-requested ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-05-13 18:31 ` Simon Horman @ 2024-05-13 20:08 ` Andrew Lunn 2024-05-13 20:17 ` Andrew Lunn ` (7 subsequent siblings) 9 siblings, 0 replies; 43+ messages in thread From: Andrew Lunn @ 2024-05-13 20:08 UTC (permalink / raw) To: admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel > +struct mctp_pcc_hdr { > + u32 signature; > + u32 flags; There looks to be an extra space here, or a tab vs space issue. > + u32 length; > + char mctp_signature[4]; > +}; > + > +struct mctp_pcc_packet { > + struct mctp_pcc_hdr pcc_header; > + union { > + struct mctp_hdr mctp_header; and more here. I would expect checkpatch to point these out. > +struct mctp_pcc_hw_addr { > + int inbox_index; > + int outbox_index; > +}; > + physical_link_addr.inbox_index = > + htonl(mctp_pcc_dev->hw_addr.inbox_index); These are {in|out}box_index are u32s right? Otherwise you would not be using htonl() on them. Maybe specify the type correctly. > + physical_link_addr.outbox_index = > + htonl(mctp_pcc_dev->hw_addr.outbox_index); You should also mark the physical_link_addr members as being big endian so sparse can check you are not missing any byte swaps. > + dev_addr_set(ndev, (const u8 *)&physical_link_addr); > + rc = register_netdev(ndev); > + if (rc) > + goto cleanup_in_channel; > + list_add_tail(&mctp_pcc_dev->head, &mctp_pcc_ndevs); > + return 0; > +cleanup_in_channel: It would be normal to add a blink line after the return, just to make it easier to see where the error cleanup code starts. > + 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); Can you get here with the ndev actually registered? > +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 0x0c: > + case 0x0a: Please replace these magic numbers of #defines. > +static int mctp_pcc_driver_add(struct acpi_device *adev) > +{ > + int inbox_index; > + int outbox_index; > + acpi_handle dev_handle; > + acpi_status status; > + struct lookup_context context = {0, 0, 0}; > + > + dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); It would be better to not spam the logs when a driver probes, unless there is an actual error. > + dev_handle = acpi_device_handle(adev); > + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, &context); > + if (ACPI_SUCCESS(status)) { > + inbox_index = context.inbox_index; > + outbox_index = context.outbox_index; > + return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); > + } > + dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); > + return -EINVAL; > +}; > + > +/* 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) { > + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head); > + if (!adev || mctp_pcc_dev->acpi_device == adev) { > + struct net_device *ndev; > + > + 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_info("initializing MCTP over PCC\n"); More useless log spamming... pr_dbg(), or remove altogether. Andrew ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-05-13 18:31 ` Simon Horman 2024-05-13 20:08 ` Andrew Lunn @ 2024-05-13 20:17 ` Andrew Lunn 2024-05-13 20:22 ` Andrew Lunn ` (6 subsequent siblings) 9 siblings, 0 replies; 43+ messages in thread From: Andrew Lunn @ 2024-05-13 20:17 UTC (permalink / raw) To: admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel > +static struct mctp_pcc_packet *mctp_pcc_extract_data(struct sk_buff *old_skb, > + void *buffer, int outbox_index) > +{ > + struct mctp_pcc_packet *mpp; > + > + mpp = buffer; > + writel(PCC_MAGIC | outbox_index, &mpp->pcc_header.signature); > + writel(0x1, &mpp->pcc_header.flags); > + memcpy_toio(mpp->pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); > + writel(old_skb->len + SIGNATURE_LENGTH, &mpp->pcc_header.length); > + memcpy_toio(mpp->header_data, old_skb->data, old_skb->len); > + return mpp; > +} ... > +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) > +{ > + unsigned char *buffer; > + struct mctp_pcc_ndev *mpnd; > + struct mctp_pcc_packet *mpp; > + unsigned long flags; > + int rc; > + > + netif_stop_queue(ndev); > + ndev->stats.tx_bytes += skb->len; > + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); > + spin_lock_irqsave(&mpnd->lock, flags); > + buffer = mpnd->pcc_comm_outbox_addr; > + mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index); I don't see any length checks here. How do you know the skb contains sizeof(struct mctp_pcc_packet)? > +static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev, > + struct device *dev, int inbox_index, > + int outbox_index) > +{ > + int rc; > + int mctp_pcc_mtu; > + char name[32]; > + struct net_device *ndev; > + struct mctp_pcc_ndev *mctp_pcc_dev; > + struct mctp_pcc_hw_addr physical_link_addr; Since this is networking code, you should be using reverse christmas tree for all your functions. > + snprintf(name, sizeof(name), "mctpipcc%x", inbox_index); > + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM, mctp_pcc_setup); %x is very unusual for network device names. Andrew ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (2 preceding siblings ...) 2024-05-13 20:17 ` Andrew Lunn @ 2024-05-13 20:22 ` Andrew Lunn 2024-05-14 5:24 ` Jeremy Kerr ` (5 subsequent siblings) 9 siblings, 0 replies; 43+ messages in thread From: Andrew Lunn @ 2024-05-13 20:22 UTC (permalink / raw) To: admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel > +struct mctp_pcc_hw_addr { > + int inbox_index; > + int outbox_index; > +}; > +static void mctp_pcc_setup(struct net_device *ndev) > +{ > + ndev->type = ARPHRD_MCTP; > + ndev->hard_header_len = 0; > + ndev->addr_len = sizeof(struct mctp_pcc_hw_addr); Another reason you should be using u32. ndev->addr_len is going to vary between 32 and 64 bit systems. I also wounder if when calling dev_addr_set() these need to be byte swapped? Does the specification define this? Andrew ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (3 preceding siblings ...) 2024-05-13 20:22 ` Andrew Lunn @ 2024-05-14 5:24 ` Jeremy Kerr 2024-05-14 10:12 ` kernel test robot ` (4 subsequent siblings) 9 siblings, 0 replies; 43+ messages in thread From: Jeremy Kerr @ 2024-05-14 5:24 UTC (permalink / raw) To: admiyo, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel Hi Adam, Nice work on sending this out. I have some comments inline, but in general, +1 for others' feedback on formatting and warning checks there. The checkpatch, NIPA and W=1 checks should help out with addressing those, so I'll focus more on the MCTP / net side. Some of these are just my unfamiliarity with the hardware mechanism, some are more fixes. > Implementation of DMTF DSP:0292 > Management Control Transport Protocol(MCTP) over > Platform Communication Channel(PCC) Could you add a bit on the platform support / requirements here (or maybe in the Kconfig help)? I figure this is entirely ACPI-based, is that correct? > --- a/drivers/net/mctp/Kconfig > +++ b/drivers/net/mctp/Kconfig > @@ -42,6 +42,18 @@ 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 MCTP_FLOWS Do you need MCTP_FLOWS here? I can't see any reference to using the flow data. Also, you probably need a dependency on ACPI. > + help > + Provides a driver to access MCTP devices over PCC transport, > + A MCTP protocol network device is created for each entry int the DST/SDST s/int/in/ > + that matches the identifier. The PCC 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/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c > new file mode 100644 > index 000000000000..7242eedd2759 > --- /dev/null > +++ b/drivers/net/mctp/mctp-pcc.c > @@ -0,0 +1,368 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * mctp_pcc.c - Driver for MCTP over PCC. Minor: mctp-pcc.c is the filename. > +struct mctp_pcc_hdr { > + u32 signature; > + u32 flags; > + u32 length; > + char mctp_signature[4]; > +}; > + > +struct mctp_pcc_packet { > + struct mctp_pcc_hdr pcc_header; > + union { > + struct mctp_hdr mctp_header; > + unsigned char header_data[sizeof(struct mctp_hdr)]; Is this worth the union? You're really referencing the header + body from your memcpy_toio(x->header_data), in which case you'd want the payload covered by that pointer too. Might be easier to just take the addresses as required, or you could drop this struct altogether (see below...) > + }; > + unsigned char payload[MCTP_PAYLOAD_LENGTH]; > +}; > + > +struct mctp_pcc_hw_addr { > + int inbox_index; > + int outbox_index; > +}; > + > +/* The netdev structure. One of these per PCC adapter. */ > +struct mctp_pcc_ndev { > + struct list_head head; Super minor, but this isn't the head of the list; better to just call this something like 'list' or 'entry' or 'node'. > + /* spinlock to serialize access from netdev to pcc buffer*/ > + spinlock_t lock; I'd suggest listing which members should be protected by that 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 struct mctp_pcc_packet *mctp_pcc_extract_data(struct sk_buff *old_skb, > + void *buffer, int outbox_index) You're mixing address spaces a little here. Looks like buffer should be void __iomem *, given you're using the io accessors in the function? [maybe do a sparse build too, to catch these] This seems to be sending the packet to hardware, so extract_data seems an odd name for the function. > +{ > + struct mctp_pcc_packet *mpp; > + > + mpp = buffer; > + writel(PCC_MAGIC | outbox_index, &mpp->pcc_header.signature); > + writel(0x1, &mpp->pcc_header.flags); > + memcpy_toio(mpp->pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); > + writel(old_skb->len + SIGNATURE_LENGTH, &mpp->pcc_header.length); > + memcpy_toio(mpp->header_data, old_skb->data, old_skb->len); You're writing the pcc_header fields individually, then writing the rest of the mctp header + payload at once. Maybe it would make more sense to drop struct mctp_pcc_packet, write a mctp_pcc_hdr, then write the packet data? > + return mpp; So this is just returning the input argument, is that necessary? > +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) > +{ > + struct sk_buff *skb; > + struct mctp_pcc_packet *mpp; > + struct mctp_skb_cb *cb; > + int data_len; > + unsigned long buf_ptr_val; > + struct mctp_pcc_ndev *mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); > + void *skb_buf; > + > + mpp = (struct mctp_pcc_packet *)mctp_pcc_dev->pcc_comm_inbox_addr; > + buf_ptr_val = (unsigned long)mpp; > + data_len = readl(&mpp->pcc_header.length) + 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; > + } > + skb->protocol = htons(ETH_P_MCTP); > + skb_buf = skb_put(skb, data_len); > + memcpy_fromio(skb_buf, mpp, data_len); > + skb_reset_mac_header(skb); > + skb_pull(skb, sizeof(struct mctp_pcc_hdr)); Any reason for including the mpp header in the skb data in the first place? Not necessarily an issue, but you may want to consider how that can be symmetrical with outgoing packets, and whether it's represented in dev->hard_header_len. My guess is that the mpp header doesn't need to be in the skb at all, as it seems more like hardware buffer layout. But you may have better designs! > + skb_reset_network_header(skb); > + cb = __mctp_cb(skb); > + cb->halen = 0; > + skb->dev = mctp_pcc_dev->mdev.dev; > + netif_rx(skb); > +} > + > +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) > +{ > + unsigned char *buffer; > + struct mctp_pcc_ndev *mpnd; > + struct mctp_pcc_packet *mpp; > + unsigned long flags; > + int rc; > + > + netif_stop_queue(ndev); > + ndev->stats.tx_bytes += skb->len; stats.tx_packets too? and what about the failure case? > + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); > + spin_lock_irqsave(&mpnd->lock, flags); > + buffer = mpnd->pcc_comm_outbox_addr; > + mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index); > + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, mpp); > + spin_unlock_irqrestore(&mpnd->lock, flags); > + > + dev_consume_skb_any(skb); > + netif_start_queue(ndev); This all looks fairly atomic, do we need to stop / start the queues here? Or: do we know that the IO region is safe to re-use immediately after send_data has returned? I'm unfamiliar with the mailbox interface. > + 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 = sizeof(struct mctp_pcc_hw_addr); Do you have any physical addressing on the PCC channels? Or is it point-to-point? If it's point to point, you probably don't need the device hardware addresses at all. Those seem to be synthetic in this driver, rather than based on a specified addressing scheme. In which case, it may be better to *not* include the hw addr on the device. > + 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) > +{ > + int rc; > + int mctp_pcc_mtu; > + char name[32]; > + struct net_device *ndev; > + struct mctp_pcc_ndev *mctp_pcc_dev; > + struct mctp_pcc_hw_addr physical_link_addr; > + > + snprintf(name, sizeof(name), "mctpipcc%x", inbox_index); What does the 'i' stand for? ('ipcc'?) Granted, this is probably more readable than mctppcc :D .. and it's convention to use %d there rather than %x, lest you get another 'c' from the twelfth 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->head); > + spin_lock_init(&mctp_pcc_dev->lock); > + > + mctp_pcc_dev->outbox_client.tx_prepare = NULL; > + mctp_pcc_dev->outbox_client.tx_done = NULL; These will already be zeroed. > + 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; So there's a bit of compatibility consideration here. Is it guaranteed that the other end of this link supports the MTU you're initially setting here? If so, defaulting to that size should be fine. However, if *not*: I'd suggest defaulting to the MCTP baseline MTU (68), and allowing upper layers to increase once we have established the capabilities of the remote endpoint. > + ndev->max_mtu = mctp_pcc_mtu; > + ndev->min_mtu = MCTP_MIN_MTU; > + > + physical_link_addr.inbox_index = > + htonl(mctp_pcc_dev->hw_addr.inbox_index); > + physical_link_addr.outbox_index = > + htonl(mctp_pcc_dev->hw_addr.outbox_index); > + dev_addr_set(ndev, (const u8 *)&physical_link_addr); Related to the physical addressing query above; it might be that you don't need any addresses here. > + rc = register_netdev(ndev); > + if (rc) > + goto cleanup_in_channel; > + list_add_tail(&mctp_pcc_dev->head, &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; > + int inbox_index; > + int 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 0x0c: > + case 0x0a: > + 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 inbox_index; > + int outbox_index; > + acpi_handle dev_handle; > + acpi_status status; > + struct lookup_context context = {0, 0, 0}; > + > + dev_info(&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)) { > + inbox_index = context.inbox_index; > + outbox_index = context.outbox_index; > + return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); > + } > + dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); > + return -EINVAL; > +}; I'd suggest flipping the conditional there, making it the error path: 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 > + */ Will that ever be needed? You should have all of the devices unbound before module exit, no? [not that I have much experience with ACPI at all...] > +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) { > + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head); > + if (!adev || mctp_pcc_dev->acpi_device == adev) { Possibly tidier to flip the condition here too, leaving fewer indents: if (!adev || mctp_pxx_dev->acpi_device != adev) continue; > + struct net_device *ndev; > + > + 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_info("initializing MCTP over PCC\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_info("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); This all looks fairly boilerplate, can you use module_acpi_driver()? (plus a static initialiser for mctp_pcc_ndevs) Overall, thanks for the contributions, with a few changes I think the transport should be good to go. Cheers, Jeremy ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (4 preceding siblings ...) 2024-05-14 5:24 ` Jeremy Kerr @ 2024-05-14 10:12 ` kernel test robot 2024-05-14 11:36 ` kernel test robot ` (3 subsequent siblings) 9 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-05-14 10:12 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: oe-kbuild-all, netdev, linux-kernel, Adam Young Hi, kernel test robot noticed the following build warnings: [auto build test WARNING on rafael-pm/linux-next] [also build test WARNING on rafael-pm/bleeding-edge linus/master v6.9 next-20240514] [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-Implement-MCTP-over-PCC-Transport/20240514-013734 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240513173546.679061-2-admiyo%40os.amperecomputing.com patch subject: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240514/202405141708.3nRcb6g3-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240514/202405141708.3nRcb6g3-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/202405141708.3nRcb6g3-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/net/mctp/mctp-pcc.c:17: >> include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration 72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root); | ^~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_client_rx_callback': >> drivers/net/mctp/mctp-pcc.c:96:23: warning: variable 'buf_ptr_val' set but not used [-Wunused-but-set-variable] 96 | unsigned long buf_ptr_val; | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_tx': >> drivers/net/mctp/mctp-pcc.c:122:24: warning: variable 'buffer' set but not used [-Wunused-but-set-variable] 122 | unsigned char *buffer; | ^~~~~~ In file included from include/linux/device.h:15, from include/linux/acpi.h:14, from drivers/net/mctp/mctp-pcc.c:7: drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add': drivers/net/mctp/mctp-pcc.c:287:23: error: invalid use of undefined type 'struct acpi_device' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~ include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ drivers/net/mctp/mctp-pcc.c:287:9: note: in expansion of macro 'dev_info' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~ drivers/net/mctp/mctp-pcc.c:287:70: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~~~~~~~~ include/linux/dev_printk.h:110:37: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:287:9: note: in expansion of macro 'dev_info' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~ drivers/net/mctp/mctp-pcc.c:288:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 288 | dev_handle = acpi_device_handle(adev); | ^~~~~~~~~~~~~~~~~~ | acpi_device_dep >> drivers/net/mctp/mctp-pcc.c:288:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion] 288 | dev_handle = acpi_device_handle(adev); | ^ drivers/net/mctp/mctp-pcc.c:293:58: error: invalid use of undefined type 'struct acpi_device' 293 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); | ^~ drivers/net/mctp/mctp-pcc.c:295:22: error: invalid use of undefined type 'struct acpi_device' 295 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~ include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ drivers/net/mctp/mctp-pcc.c:295:9: note: in expansion of macro 'dev_err' 295 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~~~~~~ drivers/net/mctp/mctp-pcc.c: At top level: drivers/net/mctp/mctp-pcc.c:329:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type 329 | static struct acpi_driver mctp_pcc_driver = { | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:330:10: error: 'struct acpi_driver' has no member named 'name' 330 | .name = "mctp_pcc", | ^~~~ >> drivers/net/mctp/mctp-pcc.c:330:17: warning: excess elements in struct initializer 330 | .name = "mctp_pcc", | ^~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:330:17: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:331:10: error: 'struct acpi_driver' has no member named 'class' 331 | .class = "Unknown", | ^~~~~ drivers/net/mctp/mctp-pcc.c:331:18: warning: excess elements in struct initializer 331 | .class = "Unknown", | ^~~~~~~~~ drivers/net/mctp/mctp-pcc.c:331:18: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:332:10: error: 'struct acpi_driver' has no member named 'ids' 332 | .ids = mctp_pcc_device_ids, | ^~~ drivers/net/mctp/mctp-pcc.c:332:16: warning: excess elements in struct initializer 332 | .ids = mctp_pcc_device_ids, | ^~~~~~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:332:16: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:333:10: error: 'struct acpi_driver' has no member named 'ops' 333 | .ops = { | ^~~ drivers/net/mctp/mctp-pcc.c:333:16: error: extra brace group at end of initializer 333 | .ops = { | ^ drivers/net/mctp/mctp-pcc.c:333:16: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:333:16: warning: excess elements in struct initializer drivers/net/mctp/mctp-pcc.c:333:16: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:338:10: error: 'struct acpi_driver' has no member named 'owner' 338 | .owner = THIS_MODULE, | ^~~~~ In file included from include/linux/printk.h:6, from include/asm-generic/bug.h:22, from arch/alpha/include/asm/bug.h:23, from include/linux/bug.h:5, from include/linux/thread_info.h:13, from include/asm-generic/preempt.h:5, from ./arch/alpha/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: include/linux/init.h:182:21: warning: excess elements in struct initializer 182 | #define THIS_MODULE ((struct module *)0) | ^ drivers/net/mctp/mctp-pcc.c:338:18: note: in expansion of macro 'THIS_MODULE' 338 | .owner = THIS_MODULE, | ^~~~~~~~~~~ include/linux/init.h:182:21: note: (near initialization for 'mctp_pcc_driver') 182 | #define THIS_MODULE ((struct module *)0) | ^ drivers/net/mctp/mctp-pcc.c:338:18: note: in expansion of macro 'THIS_MODULE' 338 | .owner = THIS_MODULE, | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_init': drivers/net/mctp/mctp-pcc.c:348:14: error: implicit declaration of function 'acpi_bus_register_driver' [-Werror=implicit-function-declaration] 348 | rc = acpi_bus_register_driver(&mctp_pcc_driver); | ^~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/net/mctp/mctp-pcc.c:350:80: warning: suggest braces around empty body in an 'if' statement [-Wempty-body] 350 | ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); | ^ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_exit': drivers/net/mctp/mctp-pcc.c:358:9: error: implicit declaration of function 'acpi_bus_unregister_driver'; did you mean 'platform_unregister_drivers'? [-Werror=implicit-function-declaration] 358 | acpi_bus_unregister_driver(&mctp_pcc_driver); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ | platform_unregister_drivers drivers/net/mctp/mctp-pcc.c: At top level: drivers/net/mctp/mctp-pcc.c:329:27: error: storage size of 'mctp_pcc_driver' isn't known 329 | static struct acpi_driver mctp_pcc_driver = { | ^~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +72 include/acpi/acpi_drivers.h ^1da177e4c3f41 Linus Torvalds 2005-04-16 71 57283776b2b821 Bjorn Helgaas 2010-03-11 @72 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root); 6b90f55f63c75c Hanjun Guo 2014-05-06 73 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (5 preceding siblings ...) 2024-05-14 10:12 ` kernel test robot @ 2024-05-14 11:36 ` kernel test robot 2024-05-14 16:29 ` kernel test robot ` (2 subsequent siblings) 9 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-05-14 11:36 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: llvm, oe-kbuild-all, netdev, linux-kernel, Adam Young 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.9 next-20240514] [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-Implement-MCTP-over-PCC-Transport/20240514-013734 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240513173546.679061-2-admiyo%40os.amperecomputing.com patch subject: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240514/202405141800.J2dxEpiu-lkp@intel.com/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project b910bebc300dafb30569cecc3017b446ea8eafa0) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240514/202405141800.J2dxEpiu-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/202405141800.J2dxEpiu-lkp@intel.com/ All error/warnings (new ones prefixed by >>): 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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:10: In file included from include/linux/mm.h:2210: include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ 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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 547 | val = __raw_readb(PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 584 | __raw_writeb(value, PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ 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:90:70: warning: omitting the parameter name in a function definition is a C23 extension [-Wc23-extensions] 90 | static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) | ^ drivers/net/mctp/mctp-pcc.c:96:16: warning: variable 'buf_ptr_val' set but not used [-Wunused-but-set-variable] 96 | unsigned long buf_ptr_val; | ^ drivers/net/mctp/mctp-pcc.c:122:17: warning: variable 'buffer' set but not used [-Wunused-but-set-variable] 122 | unsigned char *buffer; | ^ >> drivers/net/mctp/mctp-pcc.c:287:16: error: incomplete definition of type 'struct acpi_device' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ~~~~^ include/linux/dev_printk.h:150:46: note: expanded from macro 'dev_info' 150 | dev_printk_index_wrap(_dev_info, KERN_INFO, 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:63: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^ drivers/net/mctp/mctp-pcc.c:287:63: 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:288:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 288 | dev_handle = acpi_device_handle(adev); | ^ >> drivers/net/mctp/mctp-pcc.c:288:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion] 288 | dev_handle = acpi_device_handle(adev); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:293:44: error: incomplete definition of type 'struct acpi_device' 293 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); | ~~~~^ include/linux/acpi.h:794:8: note: forward declaration of 'struct acpi_device' 794 | struct acpi_device; | ^ drivers/net/mctp/mctp-pcc.c:295:15: error: incomplete definition of type 'struct acpi_device' 295 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ~~~~^ include/linux/dev_printk.h:144:44: note: expanded from macro 'dev_err' 144 | 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:329:27: error: variable has incomplete type 'struct acpi_driver' 329 | static struct acpi_driver mctp_pcc_driver = { | ^ drivers/net/mctp/mctp-pcc.c:329:15: note: forward declaration of 'struct acpi_driver' 329 | static struct acpi_driver mctp_pcc_driver = { | ^ >> drivers/net/mctp/mctp-pcc.c:348:7: error: call to undeclared function 'acpi_bus_register_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 348 | rc = acpi_bus_register_driver(&mctp_pcc_driver); | ^ >> drivers/net/mctp/mctp-pcc.c:358:2: error: call to undeclared function 'acpi_bus_unregister_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 358 | acpi_bus_unregister_driver(&mctp_pcc_driver); | ^ 11 warnings and 9 errors generated. vim +287 drivers/net/mctp/mctp-pcc.c 89 > 90 static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) 91 { 92 struct sk_buff *skb; 93 struct mctp_pcc_packet *mpp; 94 struct mctp_skb_cb *cb; 95 int data_len; 96 unsigned long buf_ptr_val; 97 struct mctp_pcc_ndev *mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); 98 void *skb_buf; 99 100 mpp = (struct mctp_pcc_packet *)mctp_pcc_dev->pcc_comm_inbox_addr; 101 buf_ptr_val = (unsigned long)mpp; 102 data_len = readl(&mpp->pcc_header.length) + MCTP_HEADER_LENGTH; 103 skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len); 104 if (!skb) { 105 mctp_pcc_dev->mdev.dev->stats.rx_dropped++; 106 return; 107 } 108 skb->protocol = htons(ETH_P_MCTP); 109 skb_buf = skb_put(skb, data_len); 110 memcpy_fromio(skb_buf, mpp, data_len); 111 skb_reset_mac_header(skb); 112 skb_pull(skb, sizeof(struct mctp_pcc_hdr)); 113 skb_reset_network_header(skb); 114 cb = __mctp_cb(skb); 115 cb->halen = 0; 116 skb->dev = mctp_pcc_dev->mdev.dev; 117 netif_rx(skb); 118 } 119 120 static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) 121 { 122 unsigned char *buffer; 123 struct mctp_pcc_ndev *mpnd; 124 struct mctp_pcc_packet *mpp; 125 unsigned long flags; 126 int rc; 127 128 netif_stop_queue(ndev); 129 ndev->stats.tx_bytes += skb->len; 130 mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev); 131 spin_lock_irqsave(&mpnd->lock, flags); 132 buffer = mpnd->pcc_comm_outbox_addr; 133 mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index); 134 rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, mpp); 135 spin_unlock_irqrestore(&mpnd->lock, flags); 136 137 dev_consume_skb_any(skb); 138 netif_start_queue(ndev); 139 if (!rc) 140 return NETDEV_TX_OK; 141 return NETDEV_TX_BUSY; 142 } 143 144 static const struct net_device_ops mctp_pcc_netdev_ops = { 145 .ndo_start_xmit = mctp_pcc_tx, 146 .ndo_uninit = NULL 147 }; 148 149 static void mctp_pcc_setup(struct net_device *ndev) 150 { 151 ndev->type = ARPHRD_MCTP; 152 ndev->hard_header_len = 0; 153 ndev->addr_len = sizeof(struct mctp_pcc_hw_addr); 154 ndev->tx_queue_len = DEFAULT_TX_QUEUE_LEN; 155 ndev->flags = IFF_NOARP; 156 ndev->netdev_ops = &mctp_pcc_netdev_ops; 157 ndev->needs_free_netdev = true; 158 } 159 160 static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev, 161 struct device *dev, int inbox_index, 162 int outbox_index) 163 { 164 int rc; 165 int mctp_pcc_mtu; 166 char name[32]; 167 struct net_device *ndev; 168 struct mctp_pcc_ndev *mctp_pcc_dev; 169 struct mctp_pcc_hw_addr physical_link_addr; 170 171 snprintf(name, sizeof(name), "mctpipcc%x", inbox_index); 172 ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM, mctp_pcc_setup); 173 if (!ndev) 174 return -ENOMEM; 175 mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev); 176 INIT_LIST_HEAD(&mctp_pcc_dev->head); 177 spin_lock_init(&mctp_pcc_dev->lock); 178 179 mctp_pcc_dev->outbox_client.tx_prepare = NULL; 180 mctp_pcc_dev->outbox_client.tx_done = NULL; 181 mctp_pcc_dev->hw_addr.inbox_index = inbox_index; 182 mctp_pcc_dev->hw_addr.outbox_index = outbox_index; 183 mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback; 184 mctp_pcc_dev->cleanup_channel = pcc_mbox_free_channel; 185 mctp_pcc_dev->out_chan = 186 pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client, 187 outbox_index); 188 if (IS_ERR(mctp_pcc_dev->out_chan)) { 189 rc = PTR_ERR(mctp_pcc_dev->out_chan); 190 goto free_netdev; 191 } 192 mctp_pcc_dev->in_chan = 193 pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client, 194 inbox_index); 195 if (IS_ERR(mctp_pcc_dev->in_chan)) { 196 rc = PTR_ERR(mctp_pcc_dev->in_chan); 197 goto cleanup_out_channel; 198 } 199 mctp_pcc_dev->pcc_comm_inbox_addr = 200 devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr, 201 mctp_pcc_dev->in_chan->shmem_size); 202 if (!mctp_pcc_dev->pcc_comm_inbox_addr) { 203 rc = -EINVAL; 204 goto cleanup_in_channel; 205 } 206 mctp_pcc_dev->pcc_comm_outbox_addr = 207 devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr, 208 mctp_pcc_dev->out_chan->shmem_size); 209 if (!mctp_pcc_dev->pcc_comm_outbox_addr) { 210 rc = -EINVAL; 211 goto cleanup_in_channel; 212 } 213 mctp_pcc_dev->acpi_device = acpi_dev; 214 mctp_pcc_dev->inbox_client.dev = dev; 215 mctp_pcc_dev->outbox_client.dev = dev; 216 mctp_pcc_dev->mdev.dev = ndev; 217 218 /* There is no clean way to pass the MTU to the callback function 219 * used for registration, so set the values ahead of time. 220 */ 221 mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size - 222 sizeof(struct mctp_pcc_hdr); 223 ndev->mtu = mctp_pcc_mtu; 224 ndev->max_mtu = mctp_pcc_mtu; 225 ndev->min_mtu = MCTP_MIN_MTU; 226 227 physical_link_addr.inbox_index = 228 htonl(mctp_pcc_dev->hw_addr.inbox_index); 229 physical_link_addr.outbox_index = 230 htonl(mctp_pcc_dev->hw_addr.outbox_index); 231 dev_addr_set(ndev, (const u8 *)&physical_link_addr); 232 rc = register_netdev(ndev); 233 if (rc) 234 goto cleanup_in_channel; 235 list_add_tail(&mctp_pcc_dev->head, &mctp_pcc_ndevs); 236 return 0; 237 cleanup_in_channel: 238 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan); 239 cleanup_out_channel: 240 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan); 241 free_netdev: 242 unregister_netdev(ndev); 243 free_netdev(ndev); 244 return rc; 245 } 246 247 struct lookup_context { 248 int index; 249 int inbox_index; 250 int outbox_index; 251 }; 252 253 static acpi_status lookup_pcct_indices(struct acpi_resource *ares, void *context) 254 { 255 struct acpi_resource_address32 *addr; 256 struct lookup_context *luc = context; 257 258 switch (ares->type) { 259 case 0x0c: 260 case 0x0a: 261 break; 262 default: 263 return AE_OK; 264 } 265 266 addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data); 267 switch (luc->index) { 268 case 0: 269 luc->outbox_index = addr[0].address.minimum; 270 break; 271 case 1: 272 luc->inbox_index = addr[0].address.minimum; 273 break; 274 } 275 luc->index++; 276 return AE_OK; 277 } 278 279 static int mctp_pcc_driver_add(struct acpi_device *adev) 280 { 281 int inbox_index; 282 int outbox_index; 283 acpi_handle dev_handle; 284 acpi_status status; 285 struct lookup_context context = {0, 0, 0}; 286 > 287 dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); > 288 dev_handle = acpi_device_handle(adev); 289 status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, &context); 290 if (ACPI_SUCCESS(status)) { 291 inbox_index = context.inbox_index; 292 outbox_index = context.outbox_index; 293 return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); 294 } 295 dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); 296 return -EINVAL; 297 }; 298 299 /* pass in adev=NULL to remove all devices 300 */ 301 static void mctp_pcc_driver_remove(struct acpi_device *adev) 302 { 303 struct mctp_pcc_ndev *mctp_pcc_dev = NULL; 304 struct list_head *ptr; 305 struct list_head *tmp; 306 307 list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) { 308 mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head); 309 if (!adev || mctp_pcc_dev->acpi_device == adev) { 310 struct net_device *ndev; 311 312 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan); 313 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan); 314 ndev = mctp_pcc_dev->mdev.dev; 315 if (ndev) 316 mctp_unregister_netdev(ndev); 317 list_del(ptr); 318 if (adev) 319 break; 320 } 321 } 322 }; 323 324 static const struct acpi_device_id mctp_pcc_device_ids[] = { 325 { "DMT0001", 0}, 326 { "", 0}, 327 }; 328 > 329 static struct acpi_driver mctp_pcc_driver = { 330 .name = "mctp_pcc", 331 .class = "Unknown", 332 .ids = mctp_pcc_device_ids, 333 .ops = { 334 .add = mctp_pcc_driver_add, 335 .remove = mctp_pcc_driver_remove, 336 .notify = NULL, 337 }, 338 .owner = THIS_MODULE, 339 340 }; 341 342 static int __init mctp_pcc_mod_init(void) 343 { 344 int rc; 345 346 pr_info("initializing MCTP over PCC\n"); 347 INIT_LIST_HEAD(&mctp_pcc_ndevs); > 348 rc = acpi_bus_register_driver(&mctp_pcc_driver); 349 if (rc < 0) 350 ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); 351 return rc; 352 } 353 354 static __exit void mctp_pcc_mod_exit(void) 355 { 356 pr_info("Removing MCTP over PCC transport driver\n"); 357 mctp_pcc_driver_remove(NULL); > 358 acpi_bus_unregister_driver(&mctp_pcc_driver); 359 } 360 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (6 preceding siblings ...) 2024-05-14 11:36 ` kernel test robot @ 2024-05-14 16:29 ` kernel test robot 2024-05-29 13:10 ` kernel test robot 2024-05-29 14:56 ` kernel test robot 9 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-05-14 16:29 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: oe-kbuild-all, netdev, linux-kernel, Adam Young 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.9 next-20240514] [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-Implement-MCTP-over-PCC-Transport/20240514-013734 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240513173546.679061-2-admiyo%40os.amperecomputing.com patch subject: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240515/202405150032.NpUqkzOG-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240515/202405150032.NpUqkzOG-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/202405150032.NpUqkzOG-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/net/mctp/mctp-pcc.c:17: include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration 72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root); | ^~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_client_rx_callback': drivers/net/mctp/mctp-pcc.c:96:23: warning: variable 'buf_ptr_val' set but not used [-Wunused-but-set-variable] 96 | unsigned long buf_ptr_val; | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_tx': drivers/net/mctp/mctp-pcc.c:122:24: warning: variable 'buffer' set but not used [-Wunused-but-set-variable] 122 | unsigned char *buffer; | ^~~~~~ In file included from include/linux/device.h:15, from include/linux/acpi.h:14, from drivers/net/mctp/mctp-pcc.c:7: drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add': >> drivers/net/mctp/mctp-pcc.c:287:23: error: invalid use of undefined type 'struct acpi_device' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~ include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ drivers/net/mctp/mctp-pcc.c:287:9: note: in expansion of macro 'dev_info' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~ >> drivers/net/mctp/mctp-pcc.c:287:70: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~~~~~~~~ include/linux/dev_printk.h:110:37: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:287:9: note: in expansion of macro 'dev_info' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~ >> drivers/net/mctp/mctp-pcc.c:288:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 288 | dev_handle = acpi_device_handle(adev); | ^~~~~~~~~~~~~~~~~~ | acpi_device_dep drivers/net/mctp/mctp-pcc.c:288:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion] 288 | dev_handle = acpi_device_handle(adev); | ^ drivers/net/mctp/mctp-pcc.c:293:58: error: invalid use of undefined type 'struct acpi_device' 293 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); | ^~ drivers/net/mctp/mctp-pcc.c:295:22: error: invalid use of undefined type 'struct acpi_device' 295 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~ include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ drivers/net/mctp/mctp-pcc.c:295:9: note: in expansion of macro 'dev_err' 295 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~~~~~~ drivers/net/mctp/mctp-pcc.c: At top level: >> drivers/net/mctp/mctp-pcc.c:329:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type 329 | static struct acpi_driver mctp_pcc_driver = { | ^~~~~~~~~~~ >> drivers/net/mctp/mctp-pcc.c:330:10: error: 'struct acpi_driver' has no member named 'name' 330 | .name = "mctp_pcc", | ^~~~ drivers/net/mctp/mctp-pcc.c:330:17: warning: excess elements in struct initializer 330 | .name = "mctp_pcc", | ^~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:330:17: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:331:10: error: 'struct acpi_driver' has no member named 'class' 331 | .class = "Unknown", | ^~~~~ drivers/net/mctp/mctp-pcc.c:331:18: warning: excess elements in struct initializer 331 | .class = "Unknown", | ^~~~~~~~~ drivers/net/mctp/mctp-pcc.c:331:18: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:332:10: error: 'struct acpi_driver' has no member named 'ids' 332 | .ids = mctp_pcc_device_ids, | ^~~ drivers/net/mctp/mctp-pcc.c:332:16: warning: excess elements in struct initializer 332 | .ids = mctp_pcc_device_ids, | ^~~~~~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:332:16: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:333:10: error: 'struct acpi_driver' has no member named 'ops' 333 | .ops = { | ^~~ >> drivers/net/mctp/mctp-pcc.c:333:16: error: extra brace group at end of initializer 333 | .ops = { | ^ drivers/net/mctp/mctp-pcc.c:333:16: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:333:16: warning: excess elements in struct initializer drivers/net/mctp/mctp-pcc.c:333:16: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:338:10: error: 'struct acpi_driver' has no member named 'owner' 338 | .owner = THIS_MODULE, | ^~~~~ In file included from include/linux/printk.h:6, from include/asm-generic/bug.h:22, from arch/alpha/include/asm/bug.h:23, from include/linux/bug.h:5, from include/linux/thread_info.h:13, from include/asm-generic/preempt.h:5, from ./arch/alpha/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: include/linux/init.h:182:21: warning: excess elements in struct initializer 182 | #define THIS_MODULE ((struct module *)0) | ^ drivers/net/mctp/mctp-pcc.c:338:18: note: in expansion of macro 'THIS_MODULE' 338 | .owner = THIS_MODULE, | ^~~~~~~~~~~ include/linux/init.h:182:21: note: (near initialization for 'mctp_pcc_driver') 182 | #define THIS_MODULE ((struct module *)0) | ^ drivers/net/mctp/mctp-pcc.c:338:18: note: in expansion of macro 'THIS_MODULE' 338 | .owner = THIS_MODULE, | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_init': >> drivers/net/mctp/mctp-pcc.c:348:14: error: implicit declaration of function 'acpi_bus_register_driver' [-Werror=implicit-function-declaration] 348 | rc = acpi_bus_register_driver(&mctp_pcc_driver); | ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:350:80: warning: suggest braces around empty body in an 'if' statement [-Wempty-body] 350 | ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); | ^ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_exit': >> drivers/net/mctp/mctp-pcc.c:358:9: error: implicit declaration of function 'acpi_bus_unregister_driver'; did you mean 'platform_unregister_drivers'? [-Werror=implicit-function-declaration] 358 | acpi_bus_unregister_driver(&mctp_pcc_driver); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ | platform_unregister_drivers drivers/net/mctp/mctp-pcc.c: At top level: >> drivers/net/mctp/mctp-pcc.c:329:27: error: storage size of 'mctp_pcc_driver' isn't known 329 | static struct acpi_driver mctp_pcc_driver = { | ^~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +287 drivers/net/mctp/mctp-pcc.c 278 279 static int mctp_pcc_driver_add(struct acpi_device *adev) 280 { 281 int inbox_index; 282 int outbox_index; 283 acpi_handle dev_handle; 284 acpi_status status; 285 struct lookup_context context = {0, 0, 0}; 286 > 287 dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); > 288 dev_handle = acpi_device_handle(adev); 289 status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, &context); 290 if (ACPI_SUCCESS(status)) { 291 inbox_index = context.inbox_index; 292 outbox_index = context.outbox_index; 293 return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); 294 } 295 dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); 296 return -EINVAL; 297 }; 298 299 /* pass in adev=NULL to remove all devices 300 */ 301 static void mctp_pcc_driver_remove(struct acpi_device *adev) 302 { 303 struct mctp_pcc_ndev *mctp_pcc_dev = NULL; 304 struct list_head *ptr; 305 struct list_head *tmp; 306 307 list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) { 308 mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head); 309 if (!adev || mctp_pcc_dev->acpi_device == adev) { 310 struct net_device *ndev; 311 312 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan); 313 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan); 314 ndev = mctp_pcc_dev->mdev.dev; 315 if (ndev) 316 mctp_unregister_netdev(ndev); 317 list_del(ptr); 318 if (adev) 319 break; 320 } 321 } 322 }; 323 324 static const struct acpi_device_id mctp_pcc_device_ids[] = { 325 { "DMT0001", 0}, 326 { "", 0}, 327 }; 328 > 329 static struct acpi_driver mctp_pcc_driver = { > 330 .name = "mctp_pcc", > 331 .class = "Unknown", > 332 .ids = mctp_pcc_device_ids, > 333 .ops = { 334 .add = mctp_pcc_driver_add, 335 .remove = mctp_pcc_driver_remove, 336 .notify = NULL, 337 }, > 338 .owner = THIS_MODULE, 339 340 }; 341 342 static int __init mctp_pcc_mod_init(void) 343 { 344 int rc; 345 346 pr_info("initializing MCTP over PCC\n"); 347 INIT_LIST_HEAD(&mctp_pcc_ndevs); > 348 rc = acpi_bus_register_driver(&mctp_pcc_driver); 349 if (rc < 0) 350 ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); 351 return rc; 352 } 353 354 static __exit void mctp_pcc_mod_exit(void) 355 { 356 pr_info("Removing MCTP over PCC transport driver\n"); 357 mctp_pcc_driver_remove(NULL); > 358 acpi_bus_unregister_driver(&mctp_pcc_driver); 359 } 360 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (7 preceding siblings ...) 2024-05-14 16:29 ` kernel test robot @ 2024-05-29 13:10 ` kernel test robot 2024-05-29 14:56 ` kernel test robot 9 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-05-29 13:10 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: oe-kbuild-all, netdev, linux-kernel, Adam Young 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-Implement-MCTP-over-PCC-Transport/20240529-072115 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240513173546.679061-2-admiyo%40os.amperecomputing.com patch subject: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240529/202405292029.IXat0564-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405292029.IXat0564-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/202405292029.IXat0564-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/net/mctp/mctp-pcc.c:17: include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration 72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root); | ^~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_client_rx_callback': drivers/net/mctp/mctp-pcc.c:96:23: warning: variable 'buf_ptr_val' set but not used [-Wunused-but-set-variable] 96 | unsigned long buf_ptr_val; | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_tx': drivers/net/mctp/mctp-pcc.c:122:24: warning: variable 'buffer' set but not used [-Wunused-but-set-variable] 122 | unsigned char *buffer; | ^~~~~~ In file included from include/linux/device.h:15, from include/linux/acpi.h:14, from drivers/net/mctp/mctp-pcc.c:7: drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add': >> drivers/net/mctp/mctp-pcc.c:287:23: error: invalid use of undefined type 'struct acpi_device' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~ include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ drivers/net/mctp/mctp-pcc.c:287:9: note: in expansion of macro 'dev_info' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~ >> drivers/net/mctp/mctp-pcc.c:287:70: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~~~~~~~~ include/linux/dev_printk.h:110:37: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:287:9: note: in expansion of macro 'dev_info' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^~~~~~~~ >> drivers/net/mctp/mctp-pcc.c:288:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 288 | dev_handle = acpi_device_handle(adev); | ^~~~~~~~~~~~~~~~~~ | acpi_device_dep drivers/net/mctp/mctp-pcc.c:288:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion] 288 | dev_handle = acpi_device_handle(adev); | ^ drivers/net/mctp/mctp-pcc.c:293:58: error: invalid use of undefined type 'struct acpi_device' 293 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); | ^~ drivers/net/mctp/mctp-pcc.c:295:22: error: invalid use of undefined type 'struct acpi_device' 295 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~ include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ drivers/net/mctp/mctp-pcc.c:295:9: note: in expansion of macro 'dev_err' 295 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~~~~~~ drivers/net/mctp/mctp-pcc.c: At top level: >> drivers/net/mctp/mctp-pcc.c:329:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type 329 | static struct acpi_driver mctp_pcc_driver = { | ^~~~~~~~~~~ >> drivers/net/mctp/mctp-pcc.c:330:10: error: 'struct acpi_driver' has no member named 'name' 330 | .name = "mctp_pcc", | ^~~~ drivers/net/mctp/mctp-pcc.c:330:17: warning: excess elements in struct initializer 330 | .name = "mctp_pcc", | ^~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:330:17: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:331:10: error: 'struct acpi_driver' has no member named 'class' 331 | .class = "Unknown", | ^~~~~ drivers/net/mctp/mctp-pcc.c:331:18: warning: excess elements in struct initializer 331 | .class = "Unknown", | ^~~~~~~~~ drivers/net/mctp/mctp-pcc.c:331:18: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:332:10: error: 'struct acpi_driver' has no member named 'ids' 332 | .ids = mctp_pcc_device_ids, | ^~~ drivers/net/mctp/mctp-pcc.c:332:16: warning: excess elements in struct initializer 332 | .ids = mctp_pcc_device_ids, | ^~~~~~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:332:16: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:333:10: error: 'struct acpi_driver' has no member named 'ops' 333 | .ops = { | ^~~ >> drivers/net/mctp/mctp-pcc.c:333:16: error: extra brace group at end of initializer 333 | .ops = { | ^ drivers/net/mctp/mctp-pcc.c:333:16: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:333:16: warning: excess elements in struct initializer drivers/net/mctp/mctp-pcc.c:333:16: note: (near initialization for 'mctp_pcc_driver') >> drivers/net/mctp/mctp-pcc.c:338:10: error: 'struct acpi_driver' has no member named 'owner' 338 | .owner = THIS_MODULE, | ^~~~~ In file included from include/linux/printk.h:6, from include/asm-generic/bug.h:22, from arch/alpha/include/asm/bug.h:23, from include/linux/bug.h:5, from include/linux/thread_info.h:13, from include/asm-generic/preempt.h:5, from ./arch/alpha/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: include/linux/init.h:182:21: warning: excess elements in struct initializer 182 | #define THIS_MODULE ((struct module *)0) | ^ drivers/net/mctp/mctp-pcc.c:338:18: note: in expansion of macro 'THIS_MODULE' 338 | .owner = THIS_MODULE, | ^~~~~~~~~~~ include/linux/init.h:182:21: note: (near initialization for 'mctp_pcc_driver') 182 | #define THIS_MODULE ((struct module *)0) | ^ drivers/net/mctp/mctp-pcc.c:338:18: note: in expansion of macro 'THIS_MODULE' 338 | .owner = THIS_MODULE, | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_init': >> drivers/net/mctp/mctp-pcc.c:348:14: error: implicit declaration of function 'acpi_bus_register_driver' [-Werror=implicit-function-declaration] 348 | rc = acpi_bus_register_driver(&mctp_pcc_driver); | ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:350:80: warning: suggest braces around empty body in an 'if' statement [-Wempty-body] 350 | ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); | ^ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_exit': >> drivers/net/mctp/mctp-pcc.c:358:9: error: implicit declaration of function 'acpi_bus_unregister_driver'; did you mean 'platform_unregister_drivers'? [-Werror=implicit-function-declaration] 358 | acpi_bus_unregister_driver(&mctp_pcc_driver); | ^~~~~~~~~~~~~~~~~~~~~~~~~~ | platform_unregister_drivers drivers/net/mctp/mctp-pcc.c: At top level: >> drivers/net/mctp/mctp-pcc.c:329:27: error: storage size of 'mctp_pcc_driver' isn't known 329 | static struct acpi_driver mctp_pcc_driver = { | ^~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +287 drivers/net/mctp/mctp-pcc.c 278 279 static int mctp_pcc_driver_add(struct acpi_device *adev) 280 { 281 int inbox_index; 282 int outbox_index; 283 acpi_handle dev_handle; 284 acpi_status status; 285 struct lookup_context context = {0, 0, 0}; 286 > 287 dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); > 288 dev_handle = acpi_device_handle(adev); 289 status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, &context); 290 if (ACPI_SUCCESS(status)) { 291 inbox_index = context.inbox_index; 292 outbox_index = context.outbox_index; 293 return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); 294 } 295 dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); 296 return -EINVAL; 297 }; 298 299 /* pass in adev=NULL to remove all devices 300 */ 301 static void mctp_pcc_driver_remove(struct acpi_device *adev) 302 { 303 struct mctp_pcc_ndev *mctp_pcc_dev = NULL; 304 struct list_head *ptr; 305 struct list_head *tmp; 306 307 list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) { 308 mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head); 309 if (!adev || mctp_pcc_dev->acpi_device == adev) { 310 struct net_device *ndev; 311 312 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan); 313 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan); 314 ndev = mctp_pcc_dev->mdev.dev; 315 if (ndev) 316 mctp_unregister_netdev(ndev); 317 list_del(ptr); 318 if (adev) 319 break; 320 } 321 } 322 }; 323 324 static const struct acpi_device_id mctp_pcc_device_ids[] = { 325 { "DMT0001", 0}, 326 { "", 0}, 327 }; 328 > 329 static struct acpi_driver mctp_pcc_driver = { > 330 .name = "mctp_pcc", > 331 .class = "Unknown", > 332 .ids = mctp_pcc_device_ids, > 333 .ops = { 334 .add = mctp_pcc_driver_add, 335 .remove = mctp_pcc_driver_remove, 336 .notify = NULL, 337 }, > 338 .owner = THIS_MODULE, 339 340 }; 341 342 static int __init mctp_pcc_mod_init(void) 343 { 344 int rc; 345 346 pr_info("initializing MCTP over PCC\n"); 347 INIT_LIST_HEAD(&mctp_pcc_ndevs); > 348 rc = acpi_bus_register_driver(&mctp_pcc_driver); 349 if (rc < 0) 350 ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); 351 return rc; 352 } 353 354 static __exit void mctp_pcc_mod_exit(void) 355 { 356 pr_info("Removing MCTP over PCC transport driver\n"); 357 mctp_pcc_driver_remove(NULL); > 358 acpi_bus_unregister_driver(&mctp_pcc_driver); 359 } 360 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (8 preceding siblings ...) 2024-05-29 13:10 ` kernel test robot @ 2024-05-29 14:56 ` kernel test robot 9 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-05-29 14:56 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: llvm, oe-kbuild-all, netdev, linux-kernel, Adam Young 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-Implement-MCTP-over-PCC-Transport/20240529-072115 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240513173546.679061-2-admiyo%40os.amperecomputing.com patch subject: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20240529/202405292251.9wZslCTL-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/202405292251.9wZslCTL-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/202405292251.9wZslCTL-lkp@intel.com/ All errors (new ones prefixed by >>): 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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:10: In file included from include/linux/mm.h:2253: 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_" | ~~~~~~~~~~~ ^ ~~~ 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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: 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/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: 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/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(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:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: 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); | ~~~~~~~~~~ ^ 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:90:70: warning: omitting the parameter name in a function definition is a C23 extension [-Wc23-extensions] 90 | static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) | ^ drivers/net/mctp/mctp-pcc.c:96:16: warning: variable 'buf_ptr_val' set but not used [-Wunused-but-set-variable] 96 | unsigned long buf_ptr_val; | ^ drivers/net/mctp/mctp-pcc.c:122:17: warning: variable 'buffer' set but not used [-Wunused-but-set-variable] 122 | unsigned char *buffer; | ^ >> drivers/net/mctp/mctp-pcc.c:287:16: error: incomplete definition of type 'struct acpi_device' 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ~~~~^ include/linux/dev_printk.h:160:46: note: expanded from macro 'dev_info' 160 | dev_printk_index_wrap(_dev_info, KERN_INFO, 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:63: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 287 | dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); | ^ drivers/net/mctp/mctp-pcc.c:287:63: 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:288:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 288 | dev_handle = acpi_device_handle(adev); | ^ >> drivers/net/mctp/mctp-pcc.c:288:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion] 288 | dev_handle = acpi_device_handle(adev); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:293:44: error: incomplete definition of type 'struct acpi_device' 293 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); | ~~~~^ include/linux/acpi.h:794:8: note: forward declaration of 'struct acpi_device' 794 | struct acpi_device; | ^ drivers/net/mctp/mctp-pcc.c:295:15: error: incomplete definition of type 'struct acpi_device' 295 | 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:329:27: error: variable has incomplete type 'struct acpi_driver' 329 | static struct acpi_driver mctp_pcc_driver = { | ^ drivers/net/mctp/mctp-pcc.c:329:15: note: forward declaration of 'struct acpi_driver' 329 | static struct acpi_driver mctp_pcc_driver = { | ^ >> drivers/net/mctp/mctp-pcc.c:348:7: error: call to undeclared function 'acpi_bus_register_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 348 | rc = acpi_bus_register_driver(&mctp_pcc_driver); | ^ >> drivers/net/mctp/mctp-pcc.c:358:2: error: call to undeclared function 'acpi_bus_unregister_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 358 | acpi_bus_unregister_driver(&mctp_pcc_driver); | ^ 11 warnings and 9 errors generated. vim +287 drivers/net/mctp/mctp-pcc.c 278 279 static int mctp_pcc_driver_add(struct acpi_device *adev) 280 { 281 int inbox_index; 282 int outbox_index; 283 acpi_handle dev_handle; 284 acpi_status status; 285 struct lookup_context context = {0, 0, 0}; 286 > 287 dev_info(&adev->dev, "Adding mctp_pcc device for HID %s\n", acpi_device_hid(adev)); > 288 dev_handle = acpi_device_handle(adev); 289 status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, &context); 290 if (ACPI_SUCCESS(status)) { 291 inbox_index = context.inbox_index; 292 outbox_index = context.outbox_index; 293 return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index); 294 } 295 dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); 296 return -EINVAL; 297 }; 298 299 /* pass in adev=NULL to remove all devices 300 */ 301 static void mctp_pcc_driver_remove(struct acpi_device *adev) 302 { 303 struct mctp_pcc_ndev *mctp_pcc_dev = NULL; 304 struct list_head *ptr; 305 struct list_head *tmp; 306 307 list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) { 308 mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head); 309 if (!adev || mctp_pcc_dev->acpi_device == adev) { 310 struct net_device *ndev; 311 312 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan); 313 mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan); 314 ndev = mctp_pcc_dev->mdev.dev; 315 if (ndev) 316 mctp_unregister_netdev(ndev); 317 list_del(ptr); 318 if (adev) 319 break; 320 } 321 } 322 }; 323 324 static const struct acpi_device_id mctp_pcc_device_ids[] = { 325 { "DMT0001", 0}, 326 { "", 0}, 327 }; 328 > 329 static struct acpi_driver mctp_pcc_driver = { 330 .name = "mctp_pcc", 331 .class = "Unknown", 332 .ids = mctp_pcc_device_ids, 333 .ops = { 334 .add = mctp_pcc_driver_add, 335 .remove = mctp_pcc_driver_remove, 336 .notify = NULL, 337 }, 338 .owner = THIS_MODULE, 339 340 }; 341 342 static int __init mctp_pcc_mod_init(void) 343 { 344 int rc; 345 346 pr_info("initializing MCTP over PCC\n"); 347 INIT_LIST_HEAD(&mctp_pcc_ndevs); > 348 rc = acpi_bus_register_driver(&mctp_pcc_driver); 349 if (rc < 0) 350 ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); 351 return rc; 352 } 353 354 static __exit void mctp_pcc_mod_exit(void) 355 { 356 pr_info("Removing MCTP over PCC transport driver\n"); 357 mctp_pcc_driver_remove(NULL); > 358 acpi_bus_unregister_driver(&mctp_pcc_driver); 359 } 360 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 2/3] mctp pcc: Allow PCC Data Type in MCTP resource. 2024-05-13 17:35 [PATCH 0/3] MCTP over PCC admiyo 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo @ 2024-05-13 17:35 ` admiyo 2024-05-13 20:23 ` Andrew Lunn 2024-05-13 17:35 ` [PATCH 3/3] mctp pcc: RFC Check before sending MCTP PCC response ACK admiyo ` (2 subsequent siblings) 4 siblings, 1 reply; 43+ messages in thread From: admiyo @ 2024-05-13 17:35 UTC (permalink / raw) To: Robert Moore, Rafael J. Wysocki, Len Brown Cc: netdev, linux-kernel, Adam Young From: Adam Young <admiyo@os.amperecomputing.com> Note that this patch sfor code that will be merged in via ACPICA changes. The corresponding patch in ACPCA has already merged. Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> --- drivers/acpi/acpica/rsaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c index fff48001d7ef..6bd9704f17b0 100644 --- a/drivers/acpi/acpica/rsaddr.c +++ b/drivers/acpi/acpica/rsaddr.c @@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource, /* Validate the Resource Type */ - if ((address.resource_type > 2) && (address.resource_type < 0xC0)) { + if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 10)) { return (FALSE); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 2/3] mctp pcc: Allow PCC Data Type in MCTP resource. 2024-05-13 17:35 ` [PATCH 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo @ 2024-05-13 20:23 ` Andrew Lunn 0 siblings, 0 replies; 43+ messages in thread From: Andrew Lunn @ 2024-05-13 20:23 UTC (permalink / raw) To: admiyo; +Cc: Robert Moore, Rafael J. Wysocki, Len Brown, netdev, linux-kernel On Mon, May 13, 2024 at 01:35:45PM -0400, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@os.amperecomputing.com> > > Note that this patch sfor code that will be merged > in via ACPICA changes. The corresponding patch in ACPCA > has already merged. > > Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> > --- > drivers/acpi/acpica/rsaddr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c > index fff48001d7ef..6bd9704f17b0 100644 > --- a/drivers/acpi/acpica/rsaddr.c > +++ b/drivers/acpi/acpica/rsaddr.c > @@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource, > > /* Validate the Resource Type */ > > - if ((address.resource_type > 2) && (address.resource_type < 0xC0)) { > + if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 10)) { > return (FALSE); More magic numbers. Please add some #defines. Andrew ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 3/3] mctp pcc: RFC Check before sending MCTP PCC response ACK 2024-05-13 17:35 [PATCH 0/3] MCTP over PCC admiyo 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-05-13 17:35 ` [PATCH 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo @ 2024-05-13 17:35 ` admiyo 2024-05-13 20:26 ` Andrew Lunn 2024-05-28 19:18 ` [PATCH v2 0/3] MCTP over PCC admiyo 2024-06-19 20:05 ` admiyo 4 siblings, 1 reply; 43+ messages in thread From: admiyo @ 2024-05-13 17:35 UTC (permalink / raw) To: Sudeep Holla, Jassi Brar, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Robert Moore, Rafael J. Wysocki, Len Brown Cc: netdev, linux-kernel, Adam Young From: Adam Young <admiyo@os.amperecomputing.com> Type 4 PCC channels have an option to send back a response to the platform when they are done processing the request. The flag to indicate whether or not to respond is inside the message body, and thus is not available to the pcc mailbox. Since only one message can be processed at once per channel, the value of this flag is checked during message processing and passed back via the channels global structure. Ideally, the mailbox callback function would return a value indicating whether the message requires an ACK, but that would be a change to the mailbox API. That would involve some change to all of the mailbox based drivers. Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> --- drivers/mailbox/pcc.c | 5 ++++- drivers/net/mctp/mctp-pcc.c | 4 ++++ include/acpi/pcc.h | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 94885e411085..774727b89693 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) { struct pcc_chan_info *pchan; struct mbox_chan *chan = p; + struct pcc_mbox_chan *pmchan; u64 val; int ret; @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) return IRQ_NONE; + pmchan = &pchan->chan; + pmchan->ack_rx = true; //TODO default to False mbox_chan_received_data(chan, NULL); /* @@ -312,7 +315,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) * * The PCC master subspace channel clears chan_in_use to free channel. */ - if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + if ((pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) && pmchan->ack_rx) pcc_send_data(chan, NULL); pchan->chan_in_use = false; diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c index 7242eedd2759..3df45b86ea03 100644 --- a/drivers/net/mctp/mctp-pcc.c +++ b/drivers/net/mctp/mctp-pcc.c @@ -96,6 +96,7 @@ static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) unsigned long buf_ptr_val; struct mctp_pcc_ndev *mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); void *skb_buf; + u32 flags; mpp = (struct mctp_pcc_packet *)mctp_pcc_dev->pcc_comm_inbox_addr; buf_ptr_val = (unsigned long)mpp; @@ -115,6 +116,9 @@ static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *) cb->halen = 0; skb->dev = mctp_pcc_dev->mdev.dev; netif_rx(skb); + + flags = readl(&mpp->pcc_header.flags); + 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) diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h index 9b373d172a77..297913378c2b 100644 --- a/include/acpi/pcc.h +++ b/include/acpi/pcc.h @@ -16,6 +16,7 @@ struct pcc_mbox_chan { u32 latency; u32 max_access_rate; u16 min_turnaround_time; + bool ack_rx; }; /* Generic Communications Channel Shared Memory Region */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 3/3] mctp pcc: RFC Check before sending MCTP PCC response ACK 2024-05-13 17:35 ` [PATCH 3/3] mctp pcc: RFC Check before sending MCTP PCC response ACK admiyo @ 2024-05-13 20:26 ` Andrew Lunn 0 siblings, 0 replies; 43+ messages in thread From: Andrew Lunn @ 2024-05-13 20:26 UTC (permalink / raw) To: admiyo Cc: Sudeep Holla, Jassi Brar, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Robert Moore, Rafael J. Wysocki, Len Brown, netdev, linux-kernel On Mon, May 13, 2024 at 01:35:46PM -0400, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@os.amperecomputing.com> > > Type 4 PCC channels have an option to send back a response > to the platform when they are done processing the request. > The flag to indicate whether or not to respond is inside > the message body, and thus is not available to the pcc > mailbox. Since only one message can be processed at once per > channel, the value of this flag is checked during message processing > and passed back via the channels global structure. > > Ideally, the mailbox callback function would return a value > indicating whether the message requires an ACK, but that > would be a change to the mailbox API. That would involve > some change to all of the mailbox based drivers. How many mailbox drivers are there? Generally, taking the path of least resistance will cost more in the long run. It is better to do it properly from the start. Andrew ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 0/3] MCTP over PCC 2024-05-13 17:35 [PATCH 0/3] MCTP over PCC admiyo ` (2 preceding siblings ...) 2024-05-13 17:35 ` [PATCH 3/3] mctp pcc: RFC Check before sending MCTP PCC response ACK admiyo @ 2024-05-28 19:18 ` admiyo 2024-05-28 19:18 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo ` (3 more replies) 2024-06-19 20:05 ` admiyo 4 siblings, 4 replies; 43+ messages in thread From: admiyo @ 2024-05-28 19:18 UTC (permalink / raw) Cc: netdev, linux-kernel From: Adam Young <admiyo@os.amperecomputing.com> This series adds support for the Management Control Transport Protocol (MCTP) over the Platform Communication Channel (PCC) mechanism. MCTP defines a communication model intended to facilitate communication between Management controllers and other management controllers, and between Management controllers and management devices PCC is a mechanism for communication between components within the Platform. It is a composed of shared memory regions, interrupt registers, and status registers. The MCTP over PCC driver makes use of two PCC channels. For sending messages, it uses a Type 3 channel, and for receiving messages it uses the paired Type 4 channel. The device and corresponding channels are specified via ACPI. Changes in V2 - All Variable Declarations are in reverse Xmass Tree Format - All Checkpatch Warnings Are Fixed - Removed Dead code - Added packet tx/rx stats - Removed network physical address. This is still in disucssion in the spec, and will be added once there is consensus. The protocol can be used with out it. This also lead to the removal of the Big Endian conversions. - Avoided using non volatile pointers in copy to and from io space - Reorderd the patches to put the ACK check for the PCC Mailbox as a pre-requisite. The corresponding change for the MCTP driver has been inlined in the main patch. - Replaced magic numbers with constants, fixed typos, and other minor changes from code review. Code Review Change not made - Did not change the module init unload function to use the ACPI equivalent as they do not remove all devices prior to unload, leading to dangling references and seg faults. Adam Young (3): mctp pcc: Check before sending MCTP PCC response ACK mctp pcc: Allow PCC Data Type in MCTP resource. mctp pcc: Implement MCTP over PCC Transport drivers/acpi/acpica/rsaddr.c | 2 +- drivers/mailbox/pcc.c | 5 +- drivers/net/mctp/Kconfig | 13 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 361 +++++++++++++++++++++++++++++++++++ include/acpi/pcc.h | 1 + 6 files changed, 381 insertions(+), 2 deletions(-) create mode 100644 drivers/net/mctp/mctp-pcc.c -- 2.34.1 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK 2024-05-28 19:18 ` [PATCH v2 0/3] MCTP over PCC admiyo @ 2024-05-28 19:18 ` admiyo 2024-05-29 3:26 ` Ratheesh Kannoth 2024-06-03 9:07 ` Sudeep Holla 2024-05-28 19:18 ` [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo ` (2 subsequent siblings) 3 siblings, 2 replies; 43+ messages in thread From: admiyo @ 2024-05-28 19:18 UTC (permalink / raw) To: Sudeep Holla, Jassi Brar, Robert Moore, Rafael J. Wysocki, Len Brown Cc: netdev, linux-kernel From: Adam Young <admiyo@amperecomputing.com> Type 4 PCC channels have an option to send back a response to the platform when they are done processing the request. The flag to indicate whether or not to respond is inside the message body, and thus is not available to the pcc mailbox. Since only one message can be processed at once per channel, the value of this flag is checked during message processing and passed back via the channels global structure. Ideally, the mailbox callback function would return a value indicating whether the message requires an ACK, but that would be a change to the mailbox API. That would involve some change to all (about 12) of the mailbox based drivers, and the majority of them would not need to know about the ACK call. Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> --- drivers/mailbox/pcc.c | 5 ++++- include/acpi/pcc.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 94885e411085..774727b89693 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) { struct pcc_chan_info *pchan; struct mbox_chan *chan = p; + struct pcc_mbox_chan *pmchan; u64 val; int ret; @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) return IRQ_NONE; + pmchan = &pchan->chan; + pmchan->ack_rx = true; //TODO default to False mbox_chan_received_data(chan, NULL); /* @@ -312,7 +315,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) * * The PCC master subspace channel clears chan_in_use to free channel. */ - if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + if ((pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) && pmchan->ack_rx) pcc_send_data(chan, NULL); pchan->chan_in_use = false; diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h index 9b373d172a77..297913378c2b 100644 --- a/include/acpi/pcc.h +++ b/include/acpi/pcc.h @@ -16,6 +16,7 @@ struct pcc_mbox_chan { u32 latency; u32 max_access_rate; u16 min_turnaround_time; + bool ack_rx; }; /* Generic Communications Channel Shared Memory Region */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK 2024-05-28 19:18 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo @ 2024-05-29 3:26 ` Ratheesh Kannoth 2024-06-03 9:07 ` Sudeep Holla 1 sibling, 0 replies; 43+ messages in thread From: Ratheesh Kannoth @ 2024-05-29 3:26 UTC (permalink / raw) To: admiyo Cc: Sudeep Holla, Jassi Brar, Robert Moore, Rafael J. Wysocki, Len Brown, netdev, linux-kernel On 2024-05-29 at 00:48:21, admiyo@os.amperecomputing.com (admiyo@os.amperecomputing.com) wrote: > From: Adam Young <admiyo@amperecomputing.com> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index 94885e411085..774727b89693 100644 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > { > struct pcc_chan_info *pchan; > struct mbox_chan *chan = p; > + struct pcc_mbox_chan *pmchan; Reverse xmas tree. > u64 val; > int ret; > > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK 2024-05-28 19:18 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo 2024-05-29 3:26 ` Ratheesh Kannoth @ 2024-06-03 9:07 ` Sudeep Holla 1 sibling, 0 replies; 43+ messages in thread From: Sudeep Holla @ 2024-06-03 9:07 UTC (permalink / raw) To: admiyo Cc: Jassi Brar, Sudeep Holla, Robert Moore, Rafael J. Wysocki, Len Brown, netdev, linux-kernel On Tue, May 28, 2024 at 03:18:21PM -0400, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@amperecomputing.com> > > Type 4 PCC channels have an option to send back a response > to the platform when they are done processing the request. > The flag to indicate whether or not to respond is inside > the message body, and thus is not available to the pcc > mailbox. Since only one message can be processed at once per > channel, the value of this flag is checked during message processing > and passed back via the channels global structure. > > Ideally, the mailbox callback function would return a value > indicating whether the message requires an ACK, but that > would be a change to the mailbox API. That would involve > some change to all (about 12) of the mailbox based drivers, > and the majority of them would not need to know about the > ACK call. > I don't have all the 3 patches. Is this sent by error or am I expected to just review this patch while other 2 are not mailbox related ? > Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> > --- > drivers/mailbox/pcc.c | 5 ++++- > include/acpi/pcc.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index 94885e411085..774727b89693 100644 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > { > struct pcc_chan_info *pchan; > struct mbox_chan *chan = p; > + struct pcc_mbox_chan *pmchan; > u64 val; > int ret; > > @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) > return IRQ_NONE; > > + pmchan = &pchan->chan; > + pmchan->ack_rx = true; //TODO default to False We need to remove this and detect when it can be true if the default expected is false. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource. 2024-05-28 19:18 ` [PATCH v2 0/3] MCTP over PCC admiyo 2024-05-28 19:18 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo @ 2024-05-28 19:18 ` admiyo 2024-05-29 3:25 ` Ratheesh Kannoth 2024-05-28 19:18 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-06-07 7:06 ` [PATCH v2 0/3] MCTP over PCC John Chung 3 siblings, 1 reply; 43+ messages in thread From: admiyo @ 2024-05-28 19:18 UTC (permalink / raw) To: Robert Moore, Rafael J. Wysocki, Len Brown; +Cc: netdev, linux-kernel From: Adam Young <admiyo@amperecomputing.com> Note that this patch sfor code that will be merged in via ACPICA changes. The corresponding patch in ACPCA has already merged. Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> --- drivers/acpi/acpica/rsaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c index fff48001d7ef..6bd9704f17b0 100644 --- a/drivers/acpi/acpica/rsaddr.c +++ b/drivers/acpi/acpica/rsaddr.c @@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource, /* Validate the Resource Type */ - if ((address.resource_type > 2) && (address.resource_type < 0xC0)) { + if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 10)) { return (FALSE); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource. 2024-05-28 19:18 ` [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo @ 2024-05-29 3:25 ` Ratheesh Kannoth 2024-05-30 16:24 ` Adam Young 0 siblings, 1 reply; 43+ messages in thread From: Ratheesh Kannoth @ 2024-05-29 3:25 UTC (permalink / raw) To: admiyo; +Cc: Robert Moore, Rafael J. Wysocki, Len Brown, netdev, linux-kernel On 2024-05-29 at 00:48:22, admiyo@os.amperecomputing.com (admiyo@os.amperecomputing.com) wrote: > From: Adam Young <admiyo@amperecomputing.com> > --- a/drivers/acpi/acpica/rsaddr.c > +++ b/drivers/acpi/acpica/rsaddr.c > @@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource, > > /* Validate the Resource Type */ > > - if ((address.resource_type > 2) && (address.resource_type < 0xC0)) { > + if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 10)) { use macros or enums instead of hard coded numbers. That will improve code readability. > return (FALSE); > } > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource. 2024-05-29 3:25 ` Ratheesh Kannoth @ 2024-05-30 16:24 ` Adam Young 0 siblings, 0 replies; 43+ messages in thread From: Adam Young @ 2024-05-30 16:24 UTC (permalink / raw) To: Ratheesh Kannoth, admiyo Cc: Robert Moore, Rafael J. Wysocki, Len Brown, netdev, linux-kernel On 5/28/24 23:25, Ratheesh Kannoth wrote: > On 2024-05-29 at 00:48:22, admiyo@os.amperecomputing.com (admiyo@os.amperecomputing.com) wrote: >> From: Adam Young <admiyo@amperecomputing.com> >> --- a/drivers/acpi/acpica/rsaddr.c >> +++ b/drivers/acpi/acpica/rsaddr.c >> @@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource, >> >> /* Validate the Resource Type */ >> >> - if ((address.resource_type > 2) && (address.resource_type < 0xC0)) { >> + if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 10)) { > use macros or enums instead of hard coded numbers. That will improve code readability. In general I agree, but this code is generated from ACPICA and will not be directly commited. The corresponding patch to acpica has already merged. What you see here is what the code will look like post-process from ACPICA conversion. > >> return (FALSE); >> } >> >> -- >> 2.34.1 >> ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-28 19:18 ` [PATCH v2 0/3] MCTP over PCC admiyo 2024-05-28 19:18 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo 2024-05-28 19:18 ` [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo @ 2024-05-28 19:18 ` admiyo 2024-05-29 2:45 ` Jakub Kicinski ` (3 more replies) 2024-06-07 7:06 ` [PATCH v2 0/3] MCTP over PCC John Chung 3 siblings, 4 replies; 43+ messages in thread From: admiyo @ 2024-05-28 19:18 UTC (permalink / raw) To: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel 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 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>"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-28 19:18 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo @ 2024-05-29 2:45 ` Jakub Kicinski 2024-05-29 3:30 ` Jeremy Kerr 2024-05-30 23:51 ` Adam Young 2024-05-29 3:02 ` Jeremy Kerr ` (2 subsequent siblings) 3 siblings, 2 replies; 43+ messages in thread From: Jakub Kicinski @ 2024-05-29 2:45 UTC (permalink / raw) To: admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Paolo Abeni, netdev, linux-kernel 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 > +}; > + -- pw-bot: cr ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-29 2:45 ` Jakub Kicinski @ 2024-05-29 3:30 ` Jeremy Kerr 2024-05-30 23:51 ` Adam Young 1 sibling, 0 replies; 43+ messages in thread From: Jeremy Kerr @ 2024-05-29 3:30 UTC (permalink / raw) To: Jakub Kicinski, admiyo Cc: Matt Johnston, David S. Miller, Eric Dumazet, Paolo Abeni, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-29 2:45 ` Jakub Kicinski 2024-05-29 3:30 ` Jeremy Kerr @ 2024-05-30 23:51 ` Adam Young 1 sibling, 0 replies; 43+ messages in thread From: Adam Young @ 2024-05-30 23:51 UTC (permalink / raw) To: Jakub Kicinski, admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Paolo Abeni, netdev, linux-kernel 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 > >> +}; >> + ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-28 19:18 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-05-29 2:45 ` Jakub Kicinski @ 2024-05-29 3:02 ` Jeremy Kerr 2024-06-03 17:53 ` Adam Young 2024-05-29 13:21 ` kernel test robot 2024-05-29 14:03 ` kernel test robot 3 siblings, 1 reply; 43+ messages in thread From: Jeremy Kerr @ 2024-05-29 3:02 UTC (permalink / raw) To: admiyo, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-29 3:02 ` Jeremy Kerr @ 2024-06-03 17:53 ` Adam Young 2024-06-04 1:15 ` Jeremy Kerr 0 siblings, 1 reply; 43+ messages in thread From: Adam Young @ 2024-06-03 17:53 UTC (permalink / raw) To: Jeremy Kerr, admiyo, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-06-03 17:53 ` Adam Young @ 2024-06-04 1:15 ` Jeremy Kerr 0 siblings, 0 replies; 43+ messages in thread From: Jeremy Kerr @ 2024-06-04 1:15 UTC (permalink / raw) To: Adam Young, admiyo, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-28 19:18 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-05-29 2:45 ` Jakub Kicinski 2024-05-29 3:02 ` Jeremy Kerr @ 2024-05-29 13:21 ` kernel test robot 2024-05-29 14:03 ` kernel test robot 3 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-05-29 13:21 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: oe-kbuild-all, netdev, linux-kernel 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 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-05-28 19:18 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo ` (2 preceding siblings ...) 2024-05-29 13:21 ` kernel test robot @ 2024-05-29 14:03 ` kernel test robot 3 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-05-29 14:03 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: llvm, oe-kbuild-all, netdev, linux-kernel 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 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/3] MCTP over PCC 2024-05-28 19:18 ` [PATCH v2 0/3] MCTP over PCC admiyo ` (2 preceding siblings ...) 2024-05-28 19:18 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo @ 2024-06-07 7:06 ` John Chung 3 siblings, 0 replies; 43+ messages in thread From: John Chung @ 2024-06-07 7:06 UTC (permalink / raw) To: admiyo; +Cc: netdev, linux-kernel, nd, john.chung On 5/29/2024 3:18 AM, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@os.amperecomputing.com> > > This series adds support for the Management Control Transport Protocol (MCTP) > over the Platform Communication Channel (PCC) mechanism. > > MCTP defines a communication model intended to > facilitate communication between Management controllers > and other management controllers, and between Management > controllers and management devices > > PCC is a mechanism for communication between components within > the Platform. It is a composed of shared memory regions, > interrupt registers, and status registers. > > The MCTP over PCC driver makes use of two PCC channels. For > sending messages, it uses a Type 3 channel, and for receiving > messages it uses the paired Type 4 channel. The device > and corresponding channels are specified via ACPI. > > Changes in V2 > > - All Variable Declarations are in reverse Xmass Tree Format > - All Checkpatch Warnings Are Fixed > - Removed Dead code > - Added packet tx/rx stats > - Removed network physical address. This is still in > disucssion in the spec, and will be added once there > is consensus. The protocol can be used with out it. > This also lead to the removal of the Big Endian > conversions. > - Avoided using non volatile pointers in copy to and from io space > - Reorderd the patches to put the ACK check for the PCC Mailbox > as a pre-requisite. The corresponding change for the MCTP > driver has been inlined in the main patch. > - Replaced magic numbers with constants, fixed typos, and other > minor changes from code review. > > Code Review Change not made > > - Did not change the module init unload function to use the > ACPI equivalent as they do not remove all devices prior > to unload, leading to dangling references and seg faults. > > Adam Young (3): > mctp pcc: Check before sending MCTP PCC response ACK > mctp pcc: Allow PCC Data Type in MCTP resource. > mctp pcc: Implement MCTP over PCC Transport > > drivers/acpi/acpica/rsaddr.c | 2 +- > drivers/mailbox/pcc.c | 5 +- > drivers/net/mctp/Kconfig | 13 ++ > drivers/net/mctp/Makefile | 1 + > drivers/net/mctp/mctp-pcc.c | 361 +++++++++++++++++++++++++++++++++++ > include/acpi/pcc.h | 1 + > 6 files changed, 381 insertions(+), 2 deletions(-) > create mode 100644 drivers/net/mctp/mctp-pcc.c > Tested OK on Arm FVP. Tested-by: John Chung <john.chung@arm.com> ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 0/3] MCTP over PCC 2024-05-13 17:35 [PATCH 0/3] MCTP over PCC admiyo ` (3 preceding siblings ...) 2024-05-28 19:18 ` [PATCH v2 0/3] MCTP over PCC admiyo @ 2024-06-19 20:05 ` admiyo 2024-06-19 20:05 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo ` (3 more replies) 4 siblings, 4 replies; 43+ messages in thread From: admiyo @ 2024-06-19 20:05 UTC (permalink / raw) Cc: netdev, linux-kernel From: Adam Young <admiyo@os.amperecomputing.com> This series adds support for the Management Control Transport Protocol (MCTP) over the Platform Communication Channel (PCC) mechanism. MCTP defines a communication model intended to facilitate communication between Management controllers and other management controllers, and between Management controllers and management devices PCC is a mechanism for communication between components within the Platform. It is a composed of shared memory regions, interrupt registers, and status registers. The MCTP over PCC driver makes use of two PCC channels. For sending messages, it uses a Type 3 channel, and for receiving messages it uses the paired Type 4 channel. The device and corresponding channels are specified via ACPI. The first patch in the series implements a mechanism to allow the driver to indicate whether an ACK should be sent back to the caller after processing the interrupt. This is an optional feature in the PCC code, but has been made explicitly required in another driver. The implementation here maintains the backwards compatibility of that driver. The second patch in the series is the required change from ACPICA code that will be imported into the Linux kernel when synchronized with the ACPICA repository. It ahs already merged there and will be merged in as is. It is included here so that the patch series can run and be tested prior to that merge. Changes in V3 - removed unused header - removed spurious space - removed spurious semis after functiomns - removed null assignment for init - remove redundant set of device on skb - tabify constant declarations - added rtnl_link_stats64 function - set MTU to minimum to start - clean up logic on driver removal - remove cast on void * assignment - call cleanup function directly - check received length before allocating skb - introduce symbolic constatn for ACK FLAG MASK - symbolic constant for PCC header flag. - Add namespace ID to PCC magic - replaced readls with copy from io of PCC header Changes in V2 - All Variable Declarations are in reverse Xmass Tree Format - All Checkpatch Warnings Are Fixed - Removed Dead code - Added packet tx/rx stats - Removed network physical address. This is still in disucssion in the spec, and will be added once there is consensus. The protocol can be used with out it. This also lead to the removal of the Big Endian conversions. - Avoided using non volatile pointers in copy to and from io space - Reorderd the patches to put the ACK check for the PCC Mailbox as a pre-requisite. The corresponding change for the MCTP driver has been inlined in the main patch. - Replaced magic numbers with constants, fixed typos, and other minor changes from code review. Code Review Change not made - Did not change the module init unload function to use the ACPI equivalent as they do not remove all devices prior to unload, leading to dangling references and seg faults. Adam Young (3): mctp pcc: Check before sending MCTP PCC response ACK mctp pcc: Allow PCC Data Type in MCTP resource. mctp pcc: Implement MCTP over PCC Transport drivers/acpi/acpica/rsaddr.c | 2 +- drivers/mailbox/pcc.c | 5 +- drivers/net/mctp/Kconfig | 13 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 373 +++++++++++++++++++++++++++++++++++ include/acpi/pcc.h | 1 + 6 files changed, 393 insertions(+), 2 deletions(-) create mode 100644 drivers/net/mctp/mctp-pcc.c -- 2.34.1 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK 2024-06-19 20:05 ` admiyo @ 2024-06-19 20:05 ` admiyo 2024-06-19 20:05 ` [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo ` (2 subsequent siblings) 3 siblings, 0 replies; 43+ messages in thread From: admiyo @ 2024-06-19 20:05 UTC (permalink / raw) To: Sudeep Holla, Jassi Brar, Rafael J. Wysocki, Len Brown, Robert Moore Cc: netdev, linux-kernel From: Adam Young <admiyo@amperecomputing.com> Type 4 PCC channels have an option to send back a response to the platform when they are done processing the request. The flag to indicate whether or not to respond is inside the message body, and thus is not available to the pcc mailbox. Since only one message can be processed at once per channel, the value of this flag is checked during message processing and passed back via the channels global structure. Ideally, the mailbox callback function would return a value indicating whether the message requires an ACK, but that would be a change to the mailbox API. That would involve some change to all (about 12) of the mailbox based drivers, and the majority of them would not need to know about the ACK call. Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> --- drivers/mailbox/pcc.c | 5 ++++- include/acpi/pcc.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 94885e411085..774727b89693 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) { struct pcc_chan_info *pchan; struct mbox_chan *chan = p; + struct pcc_mbox_chan *pmchan; u64 val; int ret; @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) return IRQ_NONE; + pmchan = &pchan->chan; + pmchan->ack_rx = true; //TODO default to False mbox_chan_received_data(chan, NULL); /* @@ -312,7 +315,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) * * The PCC master subspace channel clears chan_in_use to free channel. */ - if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + if ((pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) && pmchan->ack_rx) pcc_send_data(chan, NULL); pchan->chan_in_use = false; diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h index 9b373d172a77..297913378c2b 100644 --- a/include/acpi/pcc.h +++ b/include/acpi/pcc.h @@ -16,6 +16,7 @@ struct pcc_mbox_chan { u32 latency; u32 max_access_rate; u16 min_turnaround_time; + bool ack_rx; }; /* Generic Communications Channel Shared Memory Region */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource. 2024-06-19 20:05 ` admiyo 2024-06-19 20:05 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo @ 2024-06-19 20:05 ` admiyo 2024-06-19 20:05 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-06-20 3:10 ` [PATCH v2 0/3] MCTP over PCC Adam Young 3 siblings, 0 replies; 43+ messages in thread From: admiyo @ 2024-06-19 20:05 UTC (permalink / raw) To: Robert Moore, Rafael J. Wysocki, Len Brown; +Cc: netdev, linux-kernel From: Adam Young <admiyo@amperecomputing.com> Note that this patch is for code that will be merged in via ACPICA changes. The corresponding patch in ACPCA has already merged. Thus, no changes can be made to this patch. Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> --- drivers/acpi/acpica/rsaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c index fff48001d7ef..7932c2a6ddde 100644 --- a/drivers/acpi/acpica/rsaddr.c +++ b/drivers/acpi/acpica/rsaddr.c @@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource, /* Validate the Resource Type */ - if ((address.resource_type > 2) && (address.resource_type < 0xC0)) { + if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 0x0A)) { return (FALSE); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-06-19 20:05 ` admiyo 2024-06-19 20:05 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo 2024-06-19 20:05 ` [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo @ 2024-06-19 20:05 ` admiyo 2024-06-19 23:26 ` Jakub Kicinski ` (2 more replies) 2024-06-20 3:10 ` [PATCH v2 0/3] MCTP over PCC Adam Young 3 siblings, 3 replies; 43+ messages in thread From: admiyo @ 2024-06-19 20:05 UTC (permalink / raw) To: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel From: Adam Young <admiyo@amperecomputing.com> Implementation of DMTF DSP:0292 Management Control Transport Protocol(MCTP) over Platform Communication Channel(PCC) network driver. 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: Adam Young <admiyo@os.amperecomputing.com> --- drivers/net/mctp/Kconfig | 13 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 373 ++++++++++++++++++++++++++++++++++++ 3 files changed, 387 insertions(+) create mode 100644 drivers/net/mctp/mctp-pcc.c 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..1ce9c58099cb --- /dev/null +++ b/drivers/net/mctp/mctp-pcc.c @@ -0,0 +1,373 @@ +// 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> + +#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_HEADER_FLAG_REQ_INT 0x1 +#define PCC_HEADER_FLAGS PCC_HEADER_FLAG_REQ_INT +#define PCC_DWORD_TYPE 0x0c +#define PCC_ACK_FLAG_MASK 0x1 + +struct mctp_pcc_hdr { + u32 signature; + u32 flags; + u32 length; + char mctp_signature[4]; +}; + +struct mctp_pcc_hw_addr { + u32 inbox_index; + u32 outbox_index; +}; + +/* The netdev structure. One of these per PCC adapter. */ +struct mctp_pcc_ndev { + struct list_head next; + /* spinlock to serialize access to pcc buffer and registers*/ + spinlock_t lock; + struct mctp_dev mdev; + struct acpi_device *acpi_device; + struct pcc_mbox_chan *in_chan; + struct pcc_mbox_chan *out_chan; + struct mbox_client outbox_client; + struct mbox_client inbox_client; + void __iomem *pcc_comm_inbox_addr; + void __iomem *pcc_comm_outbox_addr; + struct mctp_pcc_hw_addr hw_addr; +}; + +static struct list_head mctp_pcc_ndevs; + +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer) +{ + struct mctp_pcc_ndev *mctp_pcc_dev; + struct mctp_pcc_hdr mctp_pcc_hdr; + struct mctp_skb_cb *cb; + struct sk_buff *skb; + void *skb_buf; + u32 data_len; + u32 flags; + + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); + memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr, + sizeof(struct mctp_pcc_hdr)); + data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH; + + if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) { + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; + return; + } + + skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len); + if (!skb) { + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; + return; + } + mctp_pcc_dev->mdev.dev->stats.rx_packets++; + mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len; + skb->protocol = htons(ETH_P_MCTP); + skb_buf = skb_put(skb, data_len); + memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len); + skb_reset_mac_header(skb); + skb_pull(skb, sizeof(struct mctp_pcc_hdr)); + skb_reset_network_header(skb); + cb = __mctp_cb(skb); + cb->halen = 0; + netif_rx(skb); + + flags = mctp_pcc_hdr.flags; + mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0; +} + +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; + + ndev->stats.tx_bytes += skb->len; + ndev->stats.tx_packets++; + mpnd = netdev_priv(ndev); + + spin_lock_irqsave(&mpnd->lock, flags); + buffer = mpnd->pcc_comm_outbox_addr; + pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index; + pcc_header.flags = PCC_HEADER_FLAGS; + 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); + mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, + NULL); + spin_unlock_irqrestore(&mpnd->lock, flags); + + dev_consume_skb_any(skb); + return NETDEV_TX_OK; +} + +static void +mctp_pcc_net_stats(struct net_device *net_dev, + struct rtnl_link_stats64 *stats) +{ + struct mctp_pcc_ndev *mpnd; + + mpnd = (struct mctp_pcc_ndev *)netdev_priv(net_dev); + stats->rx_errors = 0; + stats->rx_packets = mpnd->mdev.dev->stats.rx_packets; + stats->tx_packets = mpnd->mdev.dev->stats.tx_packets; + stats->rx_dropped = 0; + stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes; + stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes; +} + +static const struct net_device_ops mctp_pcc_netdev_ops = { + .ndo_start_xmit = mctp_pcc_tx, + .ndo_get_stats64 = mctp_pcc_net_stats, +}; + +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 = 0; + 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->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; + acpi_dev->driver_data = mctp_pcc_dev; + + /* 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_MIN_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: + pcc_mbox_free_channel(mctp_pcc_dev->in_chan); +cleanup_out_channel: + pcc_mbox_free_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; + pcc_mbox_free_channel(mctp_pcc_dev->out_chan); + pcc_mbox_free_channel(mctp_pcc_dev->in_chan); + ndev = mctp_pcc_dev->mdev.dev; + if (ndev) + mctp_unregister_netdev(ndev); + list_del(ptr); + 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, + }, + .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>"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-06-19 20:05 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo @ 2024-06-19 23:26 ` Jakub Kicinski 2024-06-20 3:24 ` Adam Young 2024-06-20 11:05 ` kernel test robot 2024-06-20 15:13 ` kernel test robot 2 siblings, 1 reply; 43+ messages in thread From: Jakub Kicinski @ 2024-06-19 23:26 UTC (permalink / raw) To: admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Paolo Abeni, netdev, linux-kernel On Wed, 19 Jun 2024 16:05:52 -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) network driver. > > 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. This patch breaks allmodconfig build: drivers/net/mctp/mctp-pcc.c:116:6: warning: unused variable 'rc' [-Wunused-variable] 116 | int rc; | ^~ drivers/net/mctp/mctp-pcc.c:344:3: error: field designator 'owner' does not refer to any field in type 'struct acpi_driver' 344 | .owner = THIS_MODULE, | ~^~~~~~~~~~~~~~~~~~~ In addition, please make sure you don't add new checkpatch warnings, use: ./scripts/checkpatch.pl --strict --max-line-length=80 $patch Please wait with the repost until next week, unless you get a review from Jeremy before that. When reposting start a new thread, don't repost in reply to previous posting. Instead add a lore link to the previous version, like this: https://lore.kernel.org/20240619200552.119080-1-admiyo@os.amperecomputing.com/ See also: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#changes-requested -- pw-bot: cr ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-06-19 23:26 ` Jakub Kicinski @ 2024-06-20 3:24 ` Adam Young 2024-06-20 13:26 ` Jakub Kicinski 0 siblings, 1 reply; 43+ messages in thread From: Adam Young @ 2024-06-20 3:24 UTC (permalink / raw) To: Jakub Kicinski, admiyo Cc: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Paolo Abeni, netdev, linux-kernel On 6/19/24 19:26, Jakub Kicinski wrote: > On Wed, 19 Jun 2024 16:05:52 -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) network driver. >> >> 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. > This patch breaks allmodconfig build: > > drivers/net/mctp/mctp-pcc.c:116:6: warning: unused variable 'rc' [-Wunused-variable] > 116 | int rc; > | ^~ Funny, I see that only when building on x86_64, not on arm64. I will fix. > drivers/net/mctp/mctp-pcc.c:344:3: error: field designator 'owner' does not refer to any field in type 'struct acpi_driver' > 344 | .owner = THIS_MODULE, > | ~^~~~~~~~~~~~~~~~~~~ Not sure how you are getting that last error. I do not, and the v6.9.3 code base has this in include/acpi/acpi_bus.h at line 166 struct acpi_driver { char name[80]; char class[80]; const struct acpi_device_id *ids; /* Supported Hardware IDs */ unsigned int flags; struct acpi_device_ops ops; struct device_driver drv; struct module *owner; }; > In addition, please make sure you don't add new checkpatch warnings, > use: > > ./scripts/checkpatch.pl --strict --max-line-length=80 $patch That runs clean. > > Please wait with the repost until next week, unless you get a review > from Jeremy before that. When reposting start a new thread, don't > repost in reply to previous posting. Instead add a lore link to the > previous version, like this: > > https://lore.kernel.org/20240619200552.119080-1-admiyo@os.amperecomputing.com/ > > See also: > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#changes-requested ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-06-20 3:24 ` Adam Young @ 2024-06-20 13:26 ` Jakub Kicinski 0 siblings, 0 replies; 43+ messages in thread From: Jakub Kicinski @ 2024-06-20 13:26 UTC (permalink / raw) To: Adam Young Cc: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Paolo Abeni, netdev, linux-kernel On Wed, 19 Jun 2024 23:24:41 -0400 Adam Young wrote: > > drivers/net/mctp/mctp-pcc.c:344:3: error: field designator 'owner' does not refer to any field in type 'struct acpi_driver' > > 344 | .owner = THIS_MODULE, > > | ~^~~~~~~~~~~~~~~~~~~ > > Not sure how you are getting that last error. I do not, and the v6.9.3 > code base has this in include/acpi/acpi_bus.h at line 166 v6.9.3 is not a development kernel. Your patches will be merged to net-next. Read more of the process doc I linked in the previous message. > That runs clean. It doesn't on patches 1 and 2, but you're right, I think it's intentional / because of the "historical" ACPI coding style :( ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-06-19 20:05 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-06-19 23:26 ` Jakub Kicinski @ 2024-06-20 11:05 ` kernel test robot 2024-06-20 15:13 ` kernel test robot 2 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-06-20 11:05 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: oe-kbuild-all, netdev, linux-kernel Hi, kernel test robot noticed the following build warnings: [auto build test WARNING on rafael-pm/linux-next] [also build test WARNING on rafael-pm/bleeding-edge linus/master v6.10-rc4 next-20240619] [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/20240620-040816 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240619200552.119080-4-admiyo%40os.amperecomputing.com patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20240620/202406201832.4oSZmptU-lkp@intel.com/config) compiler: arceb-elf-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240620/202406201832.4oSZmptU-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/202406201832.4oSZmptU-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/net/mctp/mctp-pcc.c:17: include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration 72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root); | ^~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_tx': >> drivers/net/mctp/mctp-pcc.c:116:13: warning: unused variable 'rc' [-Wunused-variable] 116 | int rc; | ^~ drivers/net/mctp/mctp-pcc.c: In function 'create_mctp_pcc_netdev': drivers/net/mctp/mctp-pcc.c:223:17: error: invalid use of undefined type 'struct acpi_device' 223 | acpi_dev->driver_data = mctp_pcc_dev; | ^~ In file included from include/linux/printk.h:573, from include/asm-generic/bug.h:22, from arch/arc/include/asm/bug.h:30, from include/linux/bug.h:5, from include/linux/thread_info.h:13, from include/asm-generic/preempt.h:5, from ./arch/arc/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: drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add': drivers/net/mctp/mctp-pcc.c:291:22: error: invalid use of undefined type 'struct acpi_device' 291 | dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n", | ^~ include/linux/dynamic_debug.h:224:29: note: in definition of macro '__dynamic_func_call_cls' 224 | func(&id, ##__VA_ARGS__); \ | ^~~~~~~~~~~ include/linux/dynamic_debug.h:250:9: note: in expansion of macro '_dynamic_func_call_cls' 250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__) | ^~~~~~~~~~~~~~~~~~~~~~ include/linux/dynamic_debug.h:273:9: note: in expansion of macro '_dynamic_func_call' 273 | _dynamic_func_call(fmt, __dynamic_dev_dbg, \ | ^~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg' 165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:291:9: note: in expansion of macro 'dev_dbg' 291 | dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n", | ^~~~~~~ drivers/net/mctp/mctp-pcc.c:292:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 292 | acpi_device_hid(adev)); | ^~~~~~~~~~~~~~~ include/linux/dynamic_debug.h:224:29: note: in definition of macro '__dynamic_func_call_cls' 224 | func(&id, ##__VA_ARGS__); \ | ^~~~~~~~~~~ include/linux/dynamic_debug.h:250:9: note: in expansion of macro '_dynamic_func_call_cls' 250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__) | ^~~~~~~~~~~~~~~~~~~~~~ include/linux/dynamic_debug.h:273:9: note: in expansion of macro '_dynamic_func_call' 273 | _dynamic_func_call(fmt, __dynamic_dev_dbg, \ | ^~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg' 165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:291:9: note: in expansion of macro 'dev_dbg' 291 | dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n", | ^~~~~~~ drivers/net/mctp/mctp-pcc.c:293:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration] 293 | dev_handle = acpi_device_handle(adev); | ^~~~~~~~~~~~~~~~~~ | acpi_device_dep drivers/net/mctp/mctp-pcc.c:293:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion] 293 | dev_handle = acpi_device_handle(adev); | ^ In file included from include/linux/device.h:15, from include/linux/acpi.h:14: drivers/net/mctp/mctp-pcc.c:297:30: error: invalid use of undefined type 'struct acpi_device' 297 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~ include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ drivers/net/mctp/mctp-pcc.c:297:17: note: in expansion of macro 'dev_err' 297 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS"); | ^~~~~~~ drivers/net/mctp/mctp-pcc.c:302:50: error: invalid use of undefined type 'struct acpi_device' 302 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, | ^~ drivers/net/mctp/mctp-pcc.c: At top level: drivers/net/mctp/mctp-pcc.c:336:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type 336 | static struct acpi_driver mctp_pcc_driver = { | ^~~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:337:10: error: 'struct acpi_driver' has no member named 'name' 337 | .name = "mctp_pcc", | ^~~~ drivers/net/mctp/mctp-pcc.c:337:17: warning: excess elements in struct initializer 337 | .name = "mctp_pcc", | ^~~~~~~~~~ drivers/net/mctp/mctp-pcc.c:337:17: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:338:10: error: 'struct acpi_driver' has no member named 'class' 338 | .class = "Unknown", | ^~~~~ drivers/net/mctp/mctp-pcc.c:338:18: warning: excess elements in struct initializer 338 | .class = "Unknown", | ^~~~~~~~~ drivers/net/mctp/mctp-pcc.c:338:18: note: (near initialization for 'mctp_pcc_driver') drivers/net/mctp/mctp-pcc.c:339:10: error: 'struct acpi_driver' has no member named 'ids' 339 | .ids = mctp_pcc_device_ids, | ^~~ vim +/rc +116 drivers/net/mctp/mctp-pcc.c 109 110 static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) 111 { 112 struct mctp_pcc_hdr pcc_header; 113 struct mctp_pcc_ndev *mpnd; 114 void __iomem *buffer; 115 unsigned long flags; > 116 int rc; 117 118 ndev->stats.tx_bytes += skb->len; 119 ndev->stats.tx_packets++; 120 mpnd = netdev_priv(ndev); 121 122 spin_lock_irqsave(&mpnd->lock, flags); 123 buffer = mpnd->pcc_comm_outbox_addr; 124 pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index; 125 pcc_header.flags = PCC_HEADER_FLAGS; 126 memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); 127 pcc_header.length = skb->len + SIGNATURE_LENGTH; 128 memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr)); 129 memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len); 130 mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, 131 NULL); 132 spin_unlock_irqrestore(&mpnd->lock, flags); 133 134 dev_consume_skb_any(skb); 135 return NETDEV_TX_OK; 136 } 137 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport 2024-06-19 20:05 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-06-19 23:26 ` Jakub Kicinski 2024-06-20 11:05 ` kernel test robot @ 2024-06-20 15:13 ` kernel test robot 2 siblings, 0 replies; 43+ messages in thread From: kernel test robot @ 2024-06-20 15:13 UTC (permalink / raw) To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Paul Gazzillo, Necip Fazil Yildiran, oe-kbuild-all, netdev, linux-kernel Hi, kernel test robot noticed the following build warnings: [auto build test WARNING on rafael-pm/linux-next] [also build test WARNING on rafael-pm/bleeding-edge linus/master v6.10-rc4 next-20240619] [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/20240620-040816 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240619200552.119080-4-admiyo%40os.amperecomputing.com patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport config: arm64-kismet-CONFIG_ACPI-CONFIG_MCTP_TRANSPORT_PCC-0-0 (https://download.01.org/0day-ci/archive/20240620/202406202206.9e7oGmfh-lkp@intel.com/config) reproduce: (https://download.01.org/0day-ci/archive/20240620/202406202206.9e7oGmfh-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/202406202206.9e7oGmfh-lkp@intel.com/ kismet warnings: (new ones prefixed by >>) >> kismet: WARNING: unmet direct dependencies detected for ACPI when selected by MCTP_TRANSPORT_PCC WARNING: unmet direct dependencies detected for ACPI Depends on [n]: ARCH_SUPPORTS_ACPI [=n] Selected by [y]: - MCTP_TRANSPORT_PCC [=y] && NETDEVICES [=y] && MCTP [=y] -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/3] MCTP over PCC 2024-06-19 20:05 ` admiyo ` (2 preceding siblings ...) 2024-06-19 20:05 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo @ 2024-06-20 3:10 ` Adam Young 3 siblings, 0 replies; 43+ messages in thread From: Adam Young @ 2024-06-20 3:10 UTC (permalink / raw) To: admiyo; +Cc: netdev, linux-kernel Apologies. I just saw that there was more feedback on the previous patchset. I am addressing that now. On 6/19/24 16:05, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@os.amperecomputing.com> > > This series adds support for the Management Control Transport Protocol (MCTP) > over the Platform Communication Channel (PCC) mechanism. > > MCTP defines a communication model intended to > facilitate communication between Management controllers > and other management controllers, and between Management > controllers and management devices > > PCC is a mechanism for communication between components within > the Platform. It is a composed of shared memory regions, > interrupt registers, and status registers. > > The MCTP over PCC driver makes use of two PCC channels. For > sending messages, it uses a Type 3 channel, and for receiving > messages it uses the paired Type 4 channel. The device > and corresponding channels are specified via ACPI. > > The first patch in the series implements a mechanism to allow the driver > to indicate whether an ACK should be sent back to the caller > after processing the interrupt. This is an optional feature in > the PCC code, but has been made explicitly required in another driver. > The implementation here maintains the backwards compatibility of that > driver. > > The second patch in the series is the required change from ACPICA > code that will be imported into the Linux kernel when synchronized > with the ACPICA repository. It ahs already merged there and will > be merged in as is. It is included here so that the patch series > can run and be tested prior to that merge. > > Changes in V3 > - removed unused header > - removed spurious space > - removed spurious semis after functiomns > - removed null assignment for init > - remove redundant set of device on skb > - tabify constant declarations > - added rtnl_link_stats64 function > - set MTU to minimum to start > - clean up logic on driver removal > - remove cast on void * assignment > - call cleanup function directly > - check received length before allocating skb > - introduce symbolic constatn for ACK FLAG MASK > - symbolic constant for PCC header flag. > - Add namespace ID to PCC magic > - replaced readls with copy from io of PCC header > > Changes in V2 > > - All Variable Declarations are in reverse Xmass Tree Format > - All Checkpatch Warnings Are Fixed > - Removed Dead code > - Added packet tx/rx stats > - Removed network physical address. This is still in > disucssion in the spec, and will be added once there > is consensus. The protocol can be used with out it. > This also lead to the removal of the Big Endian > conversions. > - Avoided using non volatile pointers in copy to and from io space > - Reorderd the patches to put the ACK check for the PCC Mailbox > as a pre-requisite. The corresponding change for the MCTP > driver has been inlined in the main patch. > - Replaced magic numbers with constants, fixed typos, and other > minor changes from code review. > > Code Review Change not made > > - Did not change the module init unload function to use the > ACPI equivalent as they do not remove all devices prior > to unload, leading to dangling references and seg faults. > > Adam Young (3): > mctp pcc: Check before sending MCTP PCC response ACK > mctp pcc: Allow PCC Data Type in MCTP resource. > mctp pcc: Implement MCTP over PCC Transport > > drivers/acpi/acpica/rsaddr.c | 2 +- > drivers/mailbox/pcc.c | 5 +- > drivers/net/mctp/Kconfig | 13 ++ > drivers/net/mctp/Makefile | 1 + > drivers/net/mctp/mctp-pcc.c | 373 +++++++++++++++++++++++++++++++++++ > include/acpi/pcc.h | 1 + > 6 files changed, 393 insertions(+), 2 deletions(-) > create mode 100644 drivers/net/mctp/mctp-pcc.c > ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2024-06-20 15:14 UTC | newest] Thread overview: 43+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-13 17:35 [PATCH 0/3] MCTP over PCC admiyo 2024-05-13 17:35 ` [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-05-13 18:31 ` Simon Horman 2024-05-13 20:08 ` Andrew Lunn 2024-05-13 20:17 ` Andrew Lunn 2024-05-13 20:22 ` Andrew Lunn 2024-05-14 5:24 ` Jeremy Kerr 2024-05-14 10:12 ` kernel test robot 2024-05-14 11:36 ` kernel test robot 2024-05-14 16:29 ` kernel test robot 2024-05-29 13:10 ` kernel test robot 2024-05-29 14:56 ` kernel test robot 2024-05-13 17:35 ` [PATCH 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo 2024-05-13 20:23 ` Andrew Lunn 2024-05-13 17:35 ` [PATCH 3/3] mctp pcc: RFC Check before sending MCTP PCC response ACK admiyo 2024-05-13 20:26 ` Andrew Lunn 2024-05-28 19:18 ` [PATCH v2 0/3] MCTP over PCC admiyo 2024-05-28 19:18 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo 2024-05-29 3:26 ` Ratheesh Kannoth 2024-06-03 9:07 ` Sudeep Holla 2024-05-28 19:18 ` [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo 2024-05-29 3:25 ` Ratheesh Kannoth 2024-05-30 16:24 ` Adam Young 2024-05-28 19:18 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-05-29 2:45 ` Jakub Kicinski 2024-05-29 3:30 ` Jeremy Kerr 2024-05-30 23:51 ` Adam Young 2024-05-29 3:02 ` Jeremy Kerr 2024-06-03 17:53 ` Adam Young 2024-06-04 1:15 ` Jeremy Kerr 2024-05-29 13:21 ` kernel test robot 2024-05-29 14:03 ` kernel test robot 2024-06-07 7:06 ` [PATCH v2 0/3] MCTP over PCC John Chung 2024-06-19 20:05 ` admiyo 2024-06-19 20:05 ` [PATCH v2 1/3] mctp pcc: Check before sending MCTP PCC response ACK admiyo 2024-06-19 20:05 ` [PATCH v2 2/3] mctp pcc: Allow PCC Data Type in MCTP resource admiyo 2024-06-19 20:05 ` [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport admiyo 2024-06-19 23:26 ` Jakub Kicinski 2024-06-20 3:24 ` Adam Young 2024-06-20 13:26 ` Jakub Kicinski 2024-06-20 11:05 ` kernel test robot 2024-06-20 15:13 ` kernel test robot 2024-06-20 3:10 ` [PATCH v2 0/3] MCTP over PCC Adam Young
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.