Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: 2024-10-31 01:28:12
Also in:
lkml
Hi Adam,
From: Adam Young <redacted> Implementation of network driver for Management Control Transport Protocol(MCTP) over Platform Communication Channel(PCC) DMTF DSP:0292 https://www.dmtf.org/sites/default/files/standards/documents/DSP0292_1.0.0WIP50.pdf 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.
Nice progress on these. A few things inline, mainly the query on device addressing.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig index 15860d6ac39f..7e55db0fb7a0 100644 --- a/drivers/net/mctp/Kconfig +++ b/drivers/net/mctp/Kconfig@@ -47,6 +47,19 @@ config MCTP_TRANSPORT_I3CA 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
Minor typo: "communication"
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c new file mode 100644 index 000000000000..b21fdca69538 --- /dev/null +++ b/drivers/net/mctp/mctp-pcc.c@@ -0,0 +1,332 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * mctp-pcc.c - Driver for MCTP over PCC. + * Copyright (c) 2024, Ampere Computing LLC + */ + +/* Implelmentation of MCTP over PCC DMTF Specification 256 + * https://www.dmtf.org/sites/default/files/standards/documents/DSP0256_2.0.0WIP50.pdf + */ + +#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 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 MCTP_SIGNATURE_LENGTH (sizeof(MCTP_SIGNATURE) - 1) +#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 + +struct mctp_pcc_hdr { + u32 signature; + u32 flags; + u32 length; + char mctp_signature[MCTP_SIGNATURE_LENGTH]; +};
These signature/flags/length still don't have the endian annotations (nor conversions on access). This was raised on v2, but looks like that got lost?
quoted hunk ↗ jump to hunk
+ +struct mctp_pcc_mailbox { + u32 index; + struct pcc_mbox_chan *chan; + struct mbox_client client; + void __iomem *addr; +}; + +struct mctp_pcc_hw_addr { + __be32 parent_id; + __be16 inbox_id; + __be16 outbox_id; +}; + +/* The netdev structure. One of these per PCC adapter. */ +struct mctp_pcc_ndev { + /* spinlock to serialize access to PCC outbox buffer and registers + * Note that what PCC calls registers are memory locations, not CPU + * Registers. They include the fields used to synchronize access + * between the OS and remote endpoints. + * + * Only the Outbox needs a spinlock, to prevent multiple + * sent packets triggering multiple attempts to over write + * the outbox. The Inbox buffer is controlled by the remote + * service and a spinlock would have no effect. + */
Nice!
quoted hunk ↗ jump to hunk
+static void +mctp_pcc_net_stats(struct net_device *net_dev, + struct rtnl_link_stats64 *stats) +{ + stats->rx_errors = 0; + stats->rx_packets = net_dev->stats.rx_packets; + stats->tx_packets = net_dev->stats.tx_packets; + stats->rx_dropped = 0; + stats->tx_bytes = net_dev->stats.tx_bytes; + stats->rx_bytes = net_dev->stats.rx_bytes; +}
Is this missing the rx_dropped stat (which you're updating in _rx_callback)? If you like, there are some new tstats helpers available, meaning you wouldn't need the ndo_get_stats64 op at all. Let me know if you're interested in using those, and would like a hand doing so.
quoted hunk ↗ jump to hunk
+static int mctp_pcc_initialize_mailbox(struct device *dev, + struct mctp_pcc_mailbox *box, u32 index) +{ + pr_info("index = %u", index);
Left over from debug?
quoted hunk ↗ jump to hunk
+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev) +{ + struct mctp_pcc_lookup_context context = {0, 0, 0}; + struct mctp_pcc_hw_addr mctp_pcc_hw_addr; + struct mctp_pcc_ndev *mctp_pcc_ndev; + struct device *dev = &acpi_dev->dev; + struct net_device *ndev; + acpi_handle dev_handle; + acpi_status status; + int mctp_pcc_mtu; + char name[32]; + int rc; + + dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
Super minor: double space before the %s here
quoted hunk ↗ jump to hunk
+ acpi_device_hid(acpi_dev)); + dev_handle = acpi_device_handle(acpi_dev); + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, + &context); + if (!ACPI_SUCCESS(status)) { + dev_err(dev, "FAILURE to lookup PCC indexes from CRS");
+ trailing newline
quoted hunk ↗ jump to hunk
+ return -EINVAL; + } + + //inbox initialization + snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index); + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM, + mctp_pcc_setup); + if (!ndev) + return -ENOMEM; + + mctp_pcc_ndev = netdev_priv(ndev); + rc = devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev); + if (rc) + goto cleanup_netdev; + spin_lock_init(&mctp_pcc_ndev->lock); + + rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox, + context.inbox_index); + if (rc) + goto cleanup_netdev; + mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback; + + //outbox initialization + rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox, + context.outbox_index); + if (rc) + goto cleanup_netdev; + + mctp_pcc_hw_addr.parent_id = cpu_to_be32(0); + mctp_pcc_hw_addr.inbox_id = cpu_to_be16(context.inbox_index); + mctp_pcc_hw_addr.outbox_id = cpu_to_be16(context.outbox_index); + ndev->addr_len = sizeof(mctp_pcc_hw_addr); + dev_addr_set(ndev, (const u8 *)&mctp_pcc_hw_addr);
I recall querying this in v1, not sure if there was a response, but: Given there is no hardware addressing in the packet format, what is the meaning of the physical address on the interface? It's a little strange to define a hardware address here that isn't used for actual addressing. For point-to-point links like this (and the serial transport), it's fine to have no hw address on the device. If this is purely local-machine-specific instance data, I suspect that this belongs elsewhere. A read-only sysfs attribute could work? Cheers, Jeremy