LinuxLists.cc - [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

2024-05-13 17:36:34

Subject: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

From: Adam Young <[email protected]>

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 <[email protected]>
---
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 <[email protected]>");
--
2.34.1

2024-05-13 18:31:26

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

On Mon, May 13, 2024 at 01:35:44PM -0400, [email protected] wrote:
> From: Adam Young <[email protected]>
>
> 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 <[email protected]>

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

2024-05-13 20:08:39

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

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

2024-05-13 20:22:24

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

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

2024-05-13 20:27:06

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

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

2024-05-14 05:24:54

by Jeremy Kerr

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

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

2024-05-14 10:44:52

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

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/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240514/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2024-05-14 11:46:20

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2024-05-14 16:31:03

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

2024-05-29 13:12:36

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

2024-05-29 15:08:51

by kernel test robot

[permalink] [raw]