From: Adam Young <admiyo@os.amperecomputing.com>
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.
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 | 332 ++++++++++++++++++++++++++++++++++++
3 files changed, 346 insertions(+)
create mode 100644 drivers/net/mctp/mctp-pcc.c
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_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..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];
+};
+
+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.
+ */
+ spinlock_t lock;
+ struct mctp_dev mdev;
+ struct acpi_device *acpi_device;
+ struct mctp_pcc_mailbox inbox;
+ struct mctp_pcc_mailbox outbox;
+};
+
+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;
+
+ mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+ memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->inbox.addr,
+ sizeof(struct mctp_pcc_hdr));
+ data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
+
+ if (data_len > mctp_pcc_dev->mdev.dev->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->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);
+}
+
+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+ struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
+ struct mctp_pcc_hdr mctp_pcc_header;
+ void __iomem *buffer;
+ unsigned long flags;
+
+ ndev->stats.tx_bytes += skb->len;
+ ndev->stats.tx_packets++;
+
+ spin_lock_irqsave(&mpnd->lock, flags);
+ buffer = mpnd->outbox.addr;
+ mctp_pcc_header.signature = PCC_MAGIC | mpnd->outbox.index;
+ mctp_pcc_header.flags = PCC_HEADER_FLAGS;
+ memcpy(mctp_pcc_header.mctp_signature, MCTP_SIGNATURE,
+ MCTP_SIGNATURE_LENGTH);
+ mctp_pcc_header.length = skb->len + MCTP_SIGNATURE_LENGTH;
+ memcpy_toio(buffer, &mctp_pcc_header, sizeof(struct mctp_pcc_hdr));
+ memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
+ mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.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)
+{
+ 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;
+}
+
+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->tx_queue_len = 0;
+ ndev->flags = IFF_NOARP;
+ ndev->netdev_ops = &mctp_pcc_netdev_ops;
+ ndev->needs_free_netdev = true;
+}
+
+struct mctp_pcc_lookup_context {
+ int index;
+ u32 inbox_index;
+ u32 outbox_index;
+};
+
+static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
+ void *context)
+{
+ struct mctp_pcc_lookup_context *luc = context;
+ struct acpi_resource_address32 *addr;
+
+ 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 void mctp_cleanup_netdev(void *data)
+{
+ struct net_device *ndev = data;
+
+ mctp_unregister_netdev(ndev);
+}
+
+static void mctp_cleanup_channel(void *data)
+{
+ struct pcc_mbox_chan *chan = data;
+
+ pcc_mbox_free_channel(chan);
+}
+
+static int mctp_pcc_initialize_mailbox(struct device *dev,
+ struct mctp_pcc_mailbox *box, u32 index)
+{
+ pr_info("index = %u", index);
+ box->index = index;
+ box->chan = pcc_mbox_request_channel(&box->client, index);
+ if (IS_ERR(box->chan))
+ return PTR_ERR(box->chan);
+ box->addr = devm_ioremap(dev, box->chan->shmem_base_addr,
+ box->chan->shmem_size);
+ if (!box->addr)
+ return -EINVAL;
+ return devm_add_action_or_reset(dev, mctp_cleanup_channel, box->chan);
+}
+
+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",
+ 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");
+ 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);
+
+ mctp_pcc_ndev->acpi_device = acpi_dev;
+ mctp_pcc_ndev->inbox.client.dev = dev;
+ mctp_pcc_ndev->outbox.client.dev = dev;
+ mctp_pcc_ndev->mdev.dev = ndev;
+ acpi_dev->driver_data = mctp_pcc_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_ndev->outbox.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;
+
+ /* ndev needs to be freed before the iomemory (mapped above) gets
+ * unmapped, devm resources get freed in reverse to the order they
+ * are added.
+ */
+ rc = register_netdev(ndev);
+ return rc;
+cleanup_netdev:
+ free_netdev(ndev);
+ return rc;
+}
+
+static const struct acpi_device_id mctp_pcc_device_ids[] = {
+ { "DMT0001"},
+ {}
+};
+
+static struct acpi_driver mctp_pcc_driver = {
+ .name = "mctp_pcc",
+ .class = "Unknown",
+ .ids = mctp_pcc_device_ids,
+ .ops = {
+ .add = mctp_pcc_driver_add,
+ },
+};
+
+module_acpi_driver(mctp_pcc_driver);
+
+MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
+
+MODULE_DESCRIPTION("MCTP PCC ACPI device");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");
--
2.34.1Hi,
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.12-rc5 next-20241031]
[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/20241030-005644
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20241029165414.58746-3-admiyo%40os.amperecomputing.com
patch subject: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241031/202410311922.C37GzI3p-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241031/202410311922.C37GzI3p-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/202410311922.C37GzI3p-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/net/mctp/mctp-pcc.c:21:
>> 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_driver_add':
>> drivers/net/mctp/mctp-pcc.c:237:39: error: invalid use of undefined type 'struct acpi_device'
237 | struct device *dev = &acpi_dev->dev;
| ^~
In file included from include/linux/printk.h:599,
from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/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:11:
>> drivers/net/mctp/mctp-pcc.c:246:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Wimplicit-function-declaration]
246 | acpi_device_hid(acpi_dev));
| ^~~~~~~~~~~~~~~
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:245:9: note: in expansion of macro 'dev_dbg'
245 | dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
>> drivers/net/mctp/mctp-pcc.c:245:22: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'int' [-Wformat=]
245 | dev_dbg(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__)
| ^~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:30: note: in expansion of macro 'dev_fmt'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:245:9: note: in expansion of macro 'dev_dbg'
245 | dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:245:56: note: format string is defined here
245 | dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
| ~^
| |
| char *
| %d
>> drivers/net/mctp/mctp-pcc.c:247:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Wimplicit-function-declaration]
247 | dev_handle = acpi_device_handle(acpi_dev);
| ^~~~~~~~~~~~~~~~~~
| acpi_device_dep
>> drivers/net/mctp/mctp-pcc.c:247:20: error: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
247 | dev_handle = acpi_device_handle(acpi_dev);
| ^
drivers/net/mctp/mctp-pcc.c:290:17: error: invalid use of undefined type 'struct acpi_device'
290 | acpi_dev->driver_data = mctp_pcc_ndev;
| ^~
drivers/net/mctp/mctp-pcc.c: At top level:
>> drivers/net/mctp/mctp-pcc.c:317:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
317 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:318:10: error: 'struct acpi_driver' has no member named 'name'
318 | .name = "mctp_pcc",
| ^~~~
>> drivers/net/mctp/mctp-pcc.c:318:17: warning: excess elements in struct initializer
318 | .name = "mctp_pcc",
| ^~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:318:17: note: (near initialization for 'mctp_pcc_driver')
>> drivers/net/mctp/mctp-pcc.c:319:10: error: 'struct acpi_driver' has no member named 'class'
319 | .class = "Unknown",
| ^~~~~
drivers/net/mctp/mctp-pcc.c:319:18: warning: excess elements in struct initializer
319 | .class = "Unknown",
| ^~~~~~~~~
drivers/net/mctp/mctp-pcc.c:319:18: note: (near initialization for 'mctp_pcc_driver')
>> drivers/net/mctp/mctp-pcc.c:320:10: error: 'struct acpi_driver' has no member named 'ids'
320 | .ids = mctp_pcc_device_ids,
| ^~~
drivers/net/mctp/mctp-pcc.c:320:16: warning: excess elements in struct initializer
320 | .ids = mctp_pcc_device_ids,
| ^~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:320:16: note: (near initialization for 'mctp_pcc_driver')
>> drivers/net/mctp/mctp-pcc.c:321:10: error: 'struct acpi_driver' has no member named 'ops'
321 | .ops = {
| ^~~
>> drivers/net/mctp/mctp-pcc.c:321:16: error: extra brace group at end of initializer
321 | .ops = {
| ^
drivers/net/mctp/mctp-pcc.c:321:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:321:16: warning: excess elements in struct initializer
drivers/net/mctp/mctp-pcc.c:321:16: note: (near initialization for 'mctp_pcc_driver')
>> drivers/net/mctp/mctp-pcc.c:326:1: warning: data definition has no type or storage class
326 | module_acpi_driver(mctp_pcc_driver);
| ^~~~~~~~~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:326:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Wimplicit-int]
>> drivers/net/mctp/mctp-pcc.c:326:1: error: parameter names (without types) in function declaration [-Wdeclaration-missing-parameter-type]
>> drivers/net/mctp/mctp-pcc.c:317:27: error: storage size of 'mctp_pcc_driver' isn't known
317 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:317:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [m]:
- RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
vim +237 drivers/net/mctp/mctp-pcc.c
231
232 static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
233 {
234 struct mctp_pcc_lookup_context context = {0, 0, 0};
235 struct mctp_pcc_hw_addr mctp_pcc_hw_addr;
236 struct mctp_pcc_ndev *mctp_pcc_ndev;
> 237 struct device *dev = &acpi_dev->dev;
238 struct net_device *ndev;
239 acpi_handle dev_handle;
240 acpi_status status;
241 int mctp_pcc_mtu;
242 char name[32];
243 int rc;
244
> 245 dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
> 246 acpi_device_hid(acpi_dev));
> 247 dev_handle = acpi_device_handle(acpi_dev);
248 status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
249 &context);
250 if (!ACPI_SUCCESS(status)) {
251 dev_err(dev, "FAILURE to lookup PCC indexes from CRS");
252 return -EINVAL;
253 }
254
255 //inbox initialization
256 snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
257 ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
258 mctp_pcc_setup);
259 if (!ndev)
260 return -ENOMEM;
261
262 mctp_pcc_ndev = netdev_priv(ndev);
263 rc = devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
264 if (rc)
265 goto cleanup_netdev;
266 spin_lock_init(&mctp_pcc_ndev->lock);
267
268 rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
269 context.inbox_index);
270 if (rc)
271 goto cleanup_netdev;
272 mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
273
274 //outbox initialization
275 rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
276 context.outbox_index);
277 if (rc)
278 goto cleanup_netdev;
279
280 mctp_pcc_hw_addr.parent_id = cpu_to_be32(0);
281 mctp_pcc_hw_addr.inbox_id = cpu_to_be16(context.inbox_index);
282 mctp_pcc_hw_addr.outbox_id = cpu_to_be16(context.outbox_index);
283 ndev->addr_len = sizeof(mctp_pcc_hw_addr);
284 dev_addr_set(ndev, (const u8 *)&mctp_pcc_hw_addr);
285
286 mctp_pcc_ndev->acpi_device = acpi_dev;
287 mctp_pcc_ndev->inbox.client.dev = dev;
288 mctp_pcc_ndev->outbox.client.dev = dev;
289 mctp_pcc_ndev->mdev.dev = ndev;
> 290 acpi_dev->driver_data = mctp_pcc_ndev;
291
292 /* There is no clean way to pass the MTU to the callback function
293 * used for registration, so set the values ahead of time.
294 */
295 mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
296 sizeof(struct mctp_pcc_hdr);
297 ndev->mtu = MCTP_MIN_MTU;
298 ndev->max_mtu = mctp_pcc_mtu;
299 ndev->min_mtu = MCTP_MIN_MTU;
300
301 /* ndev needs to be freed before the iomemory (mapped above) gets
302 * unmapped, devm resources get freed in reverse to the order they
303 * are added.
304 */
305 rc = register_netdev(ndev);
306 return rc;
307 cleanup_netdev:
308 free_netdev(ndev);
309 return rc;
310 }
311
312 static const struct acpi_device_id mctp_pcc_device_ids[] = {
313 { "DMT0001"},
314 {}
315 };
316
> 317 static struct acpi_driver mctp_pcc_driver = {
> 318 .name = "mctp_pcc",
> 319 .class = "Unknown",
> 320 .ids = mctp_pcc_device_ids,
> 321 .ops = {
322 .add = mctp_pcc_driver_add,
323 },
324 };
325
> 326 module_acpi_driver(mctp_pcc_driver);
327
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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.12-rc5 next-20241031]
[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/20241030-005644
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20241029165414.58746-3-admiyo%40os.amperecomputing.com
patch subject: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20241031/202410311939.4FK9lgPt-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241031/202410311939.4FK9lgPt-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/202410311939.4FK9lgPt-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/mctp/mctp-pcc.c:21:
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_driver_add':
drivers/net/mctp/mctp-pcc.c:237:39: error: invalid use of undefined type 'struct acpi_device'
237 | struct device *dev = &acpi_dev->dev;
| ^~
In file included from include/linux/printk.h:599,
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,
from drivers/net/mctp/mctp-pcc.c:11:
drivers/net/mctp/mctp-pcc.c:246:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
246 | acpi_device_hid(acpi_dev));
| ^~~~~~~~~~~~~~~
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:245:9: note: in expansion of macro 'dev_dbg'
245 | dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:245:22: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'int' [-Wformat=]
245 | dev_dbg(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__)
| ^~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:30: note: in expansion of macro 'dev_fmt'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:245:9: note: in expansion of macro 'dev_dbg'
245 | dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:245:56: note: format string is defined here
245 | dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
| ~^
| |
| char *
| %d
drivers/net/mctp/mctp-pcc.c:247:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
247 | dev_handle = acpi_device_handle(acpi_dev);
| ^~~~~~~~~~~~~~~~~~
| acpi_device_dep
>> drivers/net/mctp/mctp-pcc.c:247:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
247 | dev_handle = acpi_device_handle(acpi_dev);
| ^
drivers/net/mctp/mctp-pcc.c:290:17: error: invalid use of undefined type 'struct acpi_device'
290 | acpi_dev->driver_data = mctp_pcc_ndev;
| ^~
drivers/net/mctp/mctp-pcc.c: At top level:
drivers/net/mctp/mctp-pcc.c:317:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
317 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:318:10: error: 'struct acpi_driver' has no member named 'name'
318 | .name = "mctp_pcc",
| ^~~~
drivers/net/mctp/mctp-pcc.c:318:17: warning: excess elements in struct initializer
318 | .name = "mctp_pcc",
| ^~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:318:17: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:319:10: error: 'struct acpi_driver' has no member named 'class'
319 | .class = "Unknown",
| ^~~~~
drivers/net/mctp/mctp-pcc.c:319:18: warning: excess elements in struct initializer
319 | .class = "Unknown",
| ^~~~~~~~~
drivers/net/mctp/mctp-pcc.c:319:18: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:320:10: error: 'struct acpi_driver' has no member named 'ids'
320 | .ids = mctp_pcc_device_ids,
| ^~~
drivers/net/mctp/mctp-pcc.c:320:16: warning: excess elements in struct initializer
320 | .ids = mctp_pcc_device_ids,
| ^~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:320:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:321:10: error: 'struct acpi_driver' has no member named 'ops'
321 | .ops = {
| ^~~
drivers/net/mctp/mctp-pcc.c:321:16: error: extra brace group at end of initializer
321 | .ops = {
| ^
drivers/net/mctp/mctp-pcc.c:321:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:321:16: warning: excess elements in struct initializer
drivers/net/mctp/mctp-pcc.c:321:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:326:1: warning: data definition has no type or storage class
326 | module_acpi_driver(mctp_pcc_driver);
| ^~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:326:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Werror=implicit-int]
>> drivers/net/mctp/mctp-pcc.c:326:1: warning: parameter names (without types) in function declaration
drivers/net/mctp/mctp-pcc.c:317:27: error: storage size of 'mctp_pcc_driver' isn't known
317 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:317:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]
cc1: some warnings being treated as errors
vim +247 drivers/net/mctp/mctp-pcc.c
231
232 static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
233 {
234 struct mctp_pcc_lookup_context context = {0, 0, 0};
235 struct mctp_pcc_hw_addr mctp_pcc_hw_addr;
236 struct mctp_pcc_ndev *mctp_pcc_ndev;
237 struct device *dev = &acpi_dev->dev;
238 struct net_device *ndev;
239 acpi_handle dev_handle;
240 acpi_status status;
241 int mctp_pcc_mtu;
242 char name[32];
243 int rc;
244
245 dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
246 acpi_device_hid(acpi_dev));
> 247 dev_handle = acpi_device_handle(acpi_dev);
248 status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
249 &context);
250 if (!ACPI_SUCCESS(status)) {
251 dev_err(dev, "FAILURE to lookup PCC indexes from CRS");
252 return -EINVAL;
253 }
254
255 //inbox initialization
256 snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
257 ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
258 mctp_pcc_setup);
259 if (!ndev)
260 return -ENOMEM;
261
262 mctp_pcc_ndev = netdev_priv(ndev);
263 rc = devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
264 if (rc)
265 goto cleanup_netdev;
266 spin_lock_init(&mctp_pcc_ndev->lock);
267
268 rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
269 context.inbox_index);
270 if (rc)
271 goto cleanup_netdev;
272 mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
273
274 //outbox initialization
275 rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
276 context.outbox_index);
277 if (rc)
278 goto cleanup_netdev;
279
280 mctp_pcc_hw_addr.parent_id = cpu_to_be32(0);
281 mctp_pcc_hw_addr.inbox_id = cpu_to_be16(context.inbox_index);
282 mctp_pcc_hw_addr.outbox_id = cpu_to_be16(context.outbox_index);
283 ndev->addr_len = sizeof(mctp_pcc_hw_addr);
284 dev_addr_set(ndev, (const u8 *)&mctp_pcc_hw_addr);
285
286 mctp_pcc_ndev->acpi_device = acpi_dev;
287 mctp_pcc_ndev->inbox.client.dev = dev;
288 mctp_pcc_ndev->outbox.client.dev = dev;
289 mctp_pcc_ndev->mdev.dev = ndev;
290 acpi_dev->driver_data = mctp_pcc_ndev;
291
292 /* There is no clean way to pass the MTU to the callback function
293 * used for registration, so set the values ahead of time.
294 */
295 mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
296 sizeof(struct mctp_pcc_hdr);
297 ndev->mtu = MCTP_MIN_MTU;
298 ndev->max_mtu = mctp_pcc_mtu;
299 ndev->min_mtu = MCTP_MIN_MTU;
300
301 /* ndev needs to be freed before the iomemory (mapped above) gets
302 * unmapped, devm resources get freed in reverse to the order they
303 * are added.
304 */
305 rc = register_netdev(ndev);
306 return rc;
307 cleanup_netdev:
308 free_netdev(ndev);
309 return rc;
310 }
311
312 static const struct acpi_device_id mctp_pcc_device_ids[] = {
313 { "DMT0001"},
314 {}
315 };
316
317 static struct acpi_driver mctp_pcc_driver = {
318 .name = "mctp_pcc",
319 .class = "Unknown",
320 .ids = mctp_pcc_device_ids,
321 .ops = {
322 .add = mctp_pcc_driver_add,
323 },
324 };
325
> 326 module_acpi_driver(mctp_pcc_driver);
327
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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.12-rc5 next-20241031]
[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/20241030-005644
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20241029165414.58746-3-admiyo%40os.amperecomputing.com
patch subject: [PATCH v6 2/2] 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/20241031/202410312023.JZ5q2dNz-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20241031/202410312023.JZ5q2dNz-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/202410312023.JZ5q2dNz-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
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.12-rc5 next-20241031]
[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/20241030-005644
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20241029165414.58746-3-admiyo%40os.amperecomputing.com
patch subject: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20241031/202410312048.W6PV1dIU-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 639a7ac648f1e50ccd2556e17d401c04f9cce625)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241031/202410312048.W6PV1dIU-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/202410312048.W6PV1dIU-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/net/mctp/mctp-pcc.c:12:
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:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from drivers/net/mctp/mctp-pcc.c:12:
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:12:
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:12:
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:21:
>> 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:237:32: error: incomplete definition of type 'struct acpi_device'
237 | struct device *dev = &acpi_dev->dev;
| ~~~~~~~~^
include/linux/acpi.h:801:8: note: forward declaration of 'struct acpi_device'
801 | struct acpi_device;
| ^
>> drivers/net/mctp/mctp-pcc.c:246:3: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
246 | acpi_device_hid(acpi_dev));
| ^
drivers/net/mctp/mctp-pcc.c:246: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:246:3: warning: format specifies type 'char *' but the argument has type 'int' [-Wformat]
245 | dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
| ~~
| %d
246 | acpi_device_hid(acpi_dev));
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:39: note: expanded from macro 'dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:274:19: 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__); \
| ^~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:247:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
247 | dev_handle = acpi_device_handle(acpi_dev);
| ^
>> drivers/net/mctp/mctp-pcc.c:247:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion]
247 | dev_handle = acpi_device_handle(acpi_dev);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:290:10: error: incomplete definition of type 'struct acpi_device'
290 | acpi_dev->driver_data = mctp_pcc_ndev;
| ~~~~~~~~^
include/linux/acpi.h:801:8: note: forward declaration of 'struct acpi_device'
801 | struct acpi_device;
| ^
>> drivers/net/mctp/mctp-pcc.c:317:27: error: variable has incomplete type 'struct acpi_driver'
317 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:317:15: note: forward declaration of 'struct acpi_driver'
317 | static struct acpi_driver mctp_pcc_driver = {
| ^
>> drivers/net/mctp/mctp-pcc.c:326:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
326 | module_acpi_driver(mctp_pcc_driver);
| ^
| int
>> drivers/net/mctp/mctp-pcc.c:326:20: error: a parameter list without types is only allowed in a function definition
326 | module_acpi_driver(mctp_pcc_driver);
| ^
9 warnings and 8 errors generated.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [m]:
- RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
vim +237 drivers/net/mctp/mctp-pcc.c
231
232 static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
233 {
234 struct mctp_pcc_lookup_context context = {0, 0, 0};
235 struct mctp_pcc_hw_addr mctp_pcc_hw_addr;
236 struct mctp_pcc_ndev *mctp_pcc_ndev;
> 237 struct device *dev = &acpi_dev->dev;
238 struct net_device *ndev;
239 acpi_handle dev_handle;
240 acpi_status status;
241 int mctp_pcc_mtu;
242 char name[32];
243 int rc;
244
245 dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
> 246 acpi_device_hid(acpi_dev));
> 247 dev_handle = acpi_device_handle(acpi_dev);
248 status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
249 &context);
250 if (!ACPI_SUCCESS(status)) {
251 dev_err(dev, "FAILURE to lookup PCC indexes from CRS");
252 return -EINVAL;
253 }
254
255 //inbox initialization
256 snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
257 ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
258 mctp_pcc_setup);
259 if (!ndev)
260 return -ENOMEM;
261
262 mctp_pcc_ndev = netdev_priv(ndev);
263 rc = devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
264 if (rc)
265 goto cleanup_netdev;
266 spin_lock_init(&mctp_pcc_ndev->lock);
267
268 rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
269 context.inbox_index);
270 if (rc)
271 goto cleanup_netdev;
272 mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
273
274 //outbox initialization
275 rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
276 context.outbox_index);
277 if (rc)
278 goto cleanup_netdev;
279
280 mctp_pcc_hw_addr.parent_id = cpu_to_be32(0);
281 mctp_pcc_hw_addr.inbox_id = cpu_to_be16(context.inbox_index);
282 mctp_pcc_hw_addr.outbox_id = cpu_to_be16(context.outbox_index);
283 ndev->addr_len = sizeof(mctp_pcc_hw_addr);
284 dev_addr_set(ndev, (const u8 *)&mctp_pcc_hw_addr);
285
286 mctp_pcc_ndev->acpi_device = acpi_dev;
287 mctp_pcc_ndev->inbox.client.dev = dev;
288 mctp_pcc_ndev->outbox.client.dev = dev;
289 mctp_pcc_ndev->mdev.dev = ndev;
290 acpi_dev->driver_data = mctp_pcc_ndev;
291
292 /* There is no clean way to pass the MTU to the callback function
293 * used for registration, so set the values ahead of time.
294 */
295 mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
296 sizeof(struct mctp_pcc_hdr);
297 ndev->mtu = MCTP_MIN_MTU;
298 ndev->max_mtu = mctp_pcc_mtu;
299 ndev->min_mtu = MCTP_MIN_MTU;
300
301 /* ndev needs to be freed before the iomemory (mapped above) gets
302 * unmapped, devm resources get freed in reverse to the order they
303 * are added.
304 */
305 rc = register_netdev(ndev);
306 return rc;
307 cleanup_netdev:
308 free_netdev(ndev);
309 return rc;
310 }
311
312 static const struct acpi_device_id mctp_pcc_device_ids[] = {
313 { "DMT0001"},
314 {}
315 };
316
> 317 static struct acpi_driver mctp_pcc_driver = {
318 .name = "mctp_pcc",
319 .class = "Unknown",
320 .ids = mctp_pcc_device_ids,
321 .ops = {
322 .add = mctp_pcc_driver_add,
323 },
324 };
325
> 326 module_acpi_driver(mctp_pcc_driver);
327
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Adam,
> From: Adam Young <admiyo@os.amperecomputing.com>
>
> 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.
> 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_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
Minor typo: "communication"
> 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?
> +
> +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!
> +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.
> +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?
> +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
> + 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
> + 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
We need a hardware address to create a socket without an EID in order to know where we are sending the packets. The Hardware addressing is needed to be able to use the device in point-to-point mode. It is possible to have multiple devices at the hardware level, and also to not use EID based routing. Thus, the kernel needs to expose which device is which. The Essential piece of information is the outbox, which identifies which channel the message will be sent on. The inbox is in the hardware address as well as a confirmation of on which channel the messages are expected to return. In the future, it is possible to reuse channels and IRQs in constrained situations, and the driver would then be able to deduce from the packet which remote device sent it. Probably not correct to say the there is no hardware addressing on the packet. Instead, there is a portion of it on outgoing packets, and a different portion on incoming packets. The hardware address format is in an upcoming version of the spec not yet published. The namespace is for future expansion. I expect this to always be 0. I'll fix the other review corrections shortly. On 10/30/24 21:28, Jeremy Kerr wrote: > 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?
Hi Adam,
Thanks for the quick response. I think the dev lladdr is the main thing
to work out here, and it's not something we can change that post-merge.
I'm not yet convinced on your current approach, but a few
comments/queries that may help us get a consensus there, one way or the
other:
> We need a hardware address to create a socket without an EID in order
> to know where we are sending the packets.
Just to clarify that: for physical (ie, null-EID) addressing, you don't
need the hardware address, you need:
1) the outgoing interface's ifindex; and
2) the hardware address of the *remote* endpoint, in whatever
format is appropriate for link type
In cases where there is no hardware addressing in the tx packet (which
looks to apply to PCC), (2) is empty.
I understand that you're needing some mechanism for finding the correct
ifindex, but I don't think using the device lladdr is the correct
approach.
We have this model already for mctp-over-serial, which is another
point-to-point link type. MCTP-over-serial devices have no hardware
address, as there is no hardware addressing in the packet format. In
EID-less routing, it's up to the application to determine the ifindex,
using whatever existing device-identification mechanism is suitable.
> The Hardware addressing is needed to be able to use the device in
> point-to-point mode. It is possible to have multiple devices at the
> hardware level, and also to not use EID based routing. Thus, the
> kernel needs to expose which device is which.
Yes, that's totally fine to expect find a specific device - but the
device's hardware address is not the conventional place to do that.
> The Essential piece of information is the outbox, which identifies
> which channel the message will be sent on. The inbox is in the
> hardware address as well as a confirmation of on which channel the
> messages are expected to return.
Those are the indices of the shared memory regions used for the
transfer, right?
> In the future, it is possible to reuse channels and IRQs in
> constrained situations, and the driver would then be able to deduce
> from the packet which remote device sent it.
The spec mentions:
A single PCC instance shall serve as a communication channel
between at most two MCTP capable entities
so how is it ambiguous which remote device has sent a packet? Am I
misinterpreting "channel" there?
In which case, how does the driver RX path do that deduction? There is
no hardware addressing information in the DSP0292 packet format.
> Instead, there is a portion of it on outgoing packets, and a
> different portion on incoming packets.
>
> The hardware address format is in an upcoming version of the spec not
> yet published.
I can't really comment on non-published specs, but that looks more like
identifiers for the tx/rx channel pair (ie., maps to a device
identifier) rather than physical packet addressing data (ie., an
interface lladdr). Happy to be corrected on that though!
Cheers,
Jeremy
On 11/1/24 04:55, Jeremy Kerr wrote: > Just to clarify that: for physical (ie, null-EID) addressing, you don't > need the hardware address, you need: > > 1) the outgoing interface's ifindex; and > 2) the hardware address of the*remote* endpoint, in whatever > format is appropriate for link type So Here is what I was thinking: Lets ignore the namespace for now, as that is a future-proofing thing and will be all 0. If The OS listens on index 11 and the PLatform listens index 22, the HW address for the OS would be 00001122 and for the Platform 00002211 This is all the info for the calling application to know both the ifindex and the remote endpoint. They can re-order the address to 00002211 for the remote endpoint. If they have the link they have the ifindex. It seems like a clean solution. Adding the inbox id ( to the HW address does not harm anything, and it makes things much more explicit. It seems like removing either the inbox or the outbox id from the HW address is hiding information that should be exposed. And the two together make up the hardware addressing for the device, just not in that exact format, but it maps directly. That is what will be in the upcoming version of the spec as well.
Hi Adam, > Adding the inbox id ( to the HW address does not harm anything, and > it makes things much more explicit. My issue is that these inbox/outbox/subspace IDs do not align with what the device lladdr represents. From what you have said so far, and from what I can glean from the spec, what you have here is device *instance* information, not device *address* information. For an address, I would expect that to represent the address of the interface on whatever downstream bus protocol is being used. Because the packet formats do not define any addressing mechanism (ie, packets are point-to-point), there is no link-layer addressing performed by the device. You mentioned that there may, in future, be shared resources between multiple PCC interfaces (eg, a shared interrupt), but that doesn't change the point-to-point nature of the packet format, and hence the lack of bus/device addresses. This is under my assumption that a PCC interface will always represent a pair of in/out channels, to a single remote endpoint. If that won't be the case in future, then two things will need to happen: - we will need a change in the packet format to specify the source/destination addresses for a tx/rx-ed packet; and - we will *then* need to store a local address on the lladdr of the device, and implement neighbour-table lookups to query remote lladdrs. is that what the upcoming changes are intended to do? A change to the packet format seems like a fundamental rework to the design here. > It seems like removing either the inbox or the outbox id from the HW > address is hiding information that should be exposed. We can definitely expose all of the necessary instance data, but it sounds like the lladdr is not the correct facility for this. We already have examples of this instance information, like the persistent onboard-naming scheme of ethernet devices. These are separate from lladdr of those devices. Cheers, Jeremy
On 11/1/24 04:55, Jeremy Kerr wrote: > [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.] > > > Hi Adam, > > Thanks for the quick response. I think the dev lladdr is the main thing > to work out here, and it's not something we can change that post-merge. > I'm not yet convinced on your current approach, but a few > comments/queries that may help us get a consensus there, one way or the > other: Thanks so much for your review, and yes, this is one part of the code that commits us to a course of action, as it defines the userland interface. > >> We need a hardware address to create a socket without an EID in order >> to know where we are sending the packets. > Just to clarify that: for physical (ie, null-EID) addressing, you don't > need the hardware address, you need: > > 1) the outgoing interface's ifindex; and > 2) the hardware address of the *remote* endpoint, in whatever > format is appropriate for link type > > In cases where there is no hardware addressing in the tx packet (which > looks to apply to PCC), (2) is empty. > > I understand that you're needing some mechanism for finding the correct > ifindex, but I don't think using the device lladdr is the correct > approach. > > We have this model already for mctp-over-serial, which is another > point-to-point link type. MCTP-over-serial devices have no hardware > address, as there is no hardware addressing in the packet format. In > EID-less routing, it's up to the application to determine the ifindex, > using whatever existing device-identification mechanism is suitable. I'd like to avoid having a custom mechanism to find the right interface. Agreed that this is really find 1) above: selecting the outgoing interface. There is already an example of using the HW address in the interface: the loopback has an address in it, for some reason. Probably because it is inherited from the Ethernet loopback. > >> The Hardware addressing is needed to be able to use the device in >> point-to-point mode. It is possible to have multiple devices at the >> hardware level, and also to not use EID based routing. Thus, the >> kernel needs to expose which device is which. > Yes, that's totally fine to expect find a specific device - but the > device's hardware address is not the conventional place to do that. True. Typically, the hardware interface is linked to a physical device, and the operator know a-priori which network is plugged into that device. Really, device selection is a collection of heuristics. In our use case, we expect there to be two MCTP-PCC links available on a 2 Socket System, one per socket. The end user needs a way to know which device talks to which socket. In the case of a single socket system, there should only be one. However, there is no telling how this mechanism will be used in the future, and there may be MCTP-PCC enabled devices that are not bound to a CPU. > >> The Essential piece of information is the outbox, which identifies >> which channel the message will be sent on. The inbox is in the >> hardware address as well as a confirmation of on which channel the >> messages are expected to return. > Those are the indices of the shared memory regions used for the > transfer, right? Correct. And, strictly speaking, only the outbox index is in the message, but it is in there. Technically we get the signature field in the first four bytes of the PCC Generic Comunications channel Shared memory region: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#generic-communications-channel-shared-memory-region "The PCC signature. The signature of a subspace is computed by a bitwise-or of the value 0x50434300 with the subspace ID. For example, subspace 3 has the signature 0x50434303." So we could use that. And it is sufficient to let the receiver know which channel sent the message, if there are multiples: > >> In the future, it is possible to reuse channels and IRQs in >> constrained situations, and the driver would then be able to deduce >> from the packet which remote device sent it. > The spec mentions: > > A single PCC instance shall serve as a communication channel > between at most two MCTP capable entities > > so how is it ambiguous which remote device has sent a packet? Am I > misinterpreting "channel" there? Yes, the spec does say that a single channel is only ever used for a single pair of communicators. However, we have seen cases where interrupts are used for more than just a single channel, and thus I don't want to assume that it will stay that way for ever: pub-sub mechanisms are fairly common. Thus, this address does not tell the receiver where it came from, and, more importantly where to send responses to. Hence a push for a better addressing scheme. There really is no reason a single channel cannot be used by multiple publishers. > In which case, how does the driver RX path do that deduction? There is > no hardware addressing information in the DSP0292 packet format. > >> Instead, there is a portion of it on outgoing packets, and a >> different portion on incoming packets. >> >> The hardware address format is in an upcoming version of the spec not >> yet published. > I can't really comment on non-published specs, but that looks more like > identifiers for the tx/rx channel pair (ie., maps to a device > identifier) rather than physical packet addressing data (ie., an > interface lladdr). Happy to be corrected on that though! In this case, they really are the same thing: the index of the channel is used to look up the rest of the information. And the index of the outbox is the address to send the packet to, the index of the inbox is where the packet will be received. One possibility is to do another revision that uses the SIGNATURE as the HW address, with an understanding that if the signature changes, there will be a corresponding change in the HW address, and thus userland and kernel space will be kept in sync. This is an ugly format. The format suggested above is easier to parse and work with. > > Cheers, > > > Jeremy
Hi Adam, > > > We need a hardware address to create a socket without an EID in > > > order > > > to know where we are sending the packets. > > Just to clarify that: for physical (ie, null-EID) addressing, you > > don't > > need the hardware address, you need: > > > > 1) the outgoing interface's ifindex; and > > 2) the hardware address of the *remote* endpoint, in whatever > > format is appropriate for link type > > > > In cases where there is no hardware addressing in the tx packet > > (which > > looks to apply to PCC), (2) is empty. > > > > I understand that you're needing some mechanism for finding the > > correct > > ifindex, but I don't think using the device lladdr is the correct > > approach. > > > > We have this model already for mctp-over-serial, which is another > > point-to-point link type. MCTP-over-serial devices have no hardware > > address, as there is no hardware addressing in the packet format. > > In > > EID-less routing, it's up to the application to determine the > > ifindex, > > using whatever existing device-identification mechanism is > > suitable. > > I'd like to avoid having a custom mechanism to find the right > interface. Agreed that this is really find 1) above: selecting the > outgoing interface. OK, but from what you're adding later it sounds like you already have part of that mechanism custom anyway: the mapping of a socket to a channel index? It sounds like there will always be some requirement for a platform-specific inventory-mapping mechanism; you're going from socket number to ifindex. It should be just as equivalent to implement that using a sysfs attribute rather than the device lladdr, no? > There is already an example of using the HW address in the interface: > the loopback has an address in it, for some reason. Probably because > it is inherited from the Ethernet loopback. Yes, and that the ethernet packet format does include a physical address, hence the lladdr being present on lo. > In our use case, we expect there to be two MCTP-PCC links available > on a > 2 Socket System, one per socket. The end user needs a way to know > which > device talks to which socket. In the case of a single socket system, > there should only be one. > > However, there is no telling how this mechanism will be used in the > future, and there may be MCTP-PCC enabled devices that are not bound > to a CPU. That's fine; I think finding an interface based on the channel numbers seems generally applicable. > Technically we get the signature field in the first four bytes of the > PCC Generic Comunications channel Shared memory region: > > https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#generic-communications-channel-shared-memory-region > > "The PCC signature. The signature of a subspace is computed by a > bitwise-or of the value 0x50434300 with the subspace ID. For example, > subspace 3 has the signature 0x50434303." ok! so there is some form of addressing on the packet. Can we use this subspace ID as a form of lladdr? Could this be interpreted as the "destination" of a packet? You do mention that it may not be suitable though: > One possibility is to do another revision that uses the SIGNATURE as > the HW address, with an understanding that if the signature changes, > there will be a corresponding change in the HW address, Is that signature format expected to change across DSP0292 versions? Cheers, Jeremy
On 11/5/24 09:09, Jeremy Kerr wrote: > ok! so there is some form of addressing on the packet. Can we use this > subspace ID as a form of lladdr? Could this be interpreted as the > "destination" of a packet? > > You do mention that it may not be suitable though: In the header of the packet is a signature: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#generic-communications-channel-shared-memory-region "The PCC signature. The signature of a subspace is computed by a bitwise-or of the value 0x50434300 with the subspace ID. For example, subspace 3 has the signature 0x50434303." This could be used, but the inclusion of the "PCC" is unnecessary, as it is on every packet. Thus only the subspace ID is relevant. This is the index of the entry in the PCCT, and is what I have been calling the outbox ID. Thus it is half of the address that I am proposing. Two way communication in MCTP over PCC requires two subspaces. The return packet would have a different subspace ID. Thus, the format for the physical address is combination of the two subspace IDs. Say the PCCT has two entries for MCTP: 0x12 and 0x13. 12 is the outgoing for the OS and incoming for the platform. 13 is outgoing for the platform and incoming for the OS. The signatures on the packets would be 0x50434312 and 0x50434313, with the last two digits being the only ones that would ever change. These two channels are Type3 and Type4 by the PCC spec, and are thus paired. So the physical addressing scheme for MCTP-PCC instead uses both of these address, and uses the order to distinguish which is which: for the OS endpoint, the hw address would be 0x00001312. For the platform, the HW address would be 0x00001213.
Hi Adam,
>
> "The PCC signature. The signature of a subspace is computed by a
> bitwise-or of the value 0x50434300 with the subspace ID. For example,
> subspace 3 has the signature 0x50434303."
>
> This could be used, but the inclusion of the "PCC" is unnecessary, as
> it is on every packet. Thus only the subspace ID is relevant. This
> is the index of the entry in the PCCT, and is what I have been
> calling the outbox ID. Thus it is half of the address that I am
> proposing.
But the signature value isn't implementing any MCTP-bus addressing
functionality, right? It's a fixed value that has to be set the same
way on all transactions using that PCC channel.
Just to walk it back a bit, we have two possible interpretations here:
1) that the channel indexes *do* form something like a physical
addressing mechanism; when packets are sent over a channel to a
remote endpoint, we need to add a specific channel identifier.
2) that the channel indices are more of an internal detail of the
transport mechanism: they're identifying channels, not MCTP
endpoints (kinda like setting the PCIe target address when
transmitting a network packet, perhaps?)
If we adopt the (1) approach, we'd want a hardware address to represent
the mechanism for addressing an MCTP endpoint, not an interface
instance. That would end up with something along the lines of:
- MCTP-over-PCC hardware addresses would be a single byte (to contain a
channel ID)
- the interface would have a hardware address of just the inbox ID:
incoming packets are received via the inbox to the local interface,
and so are "addressed" to that inbox ID
- remote endpoints would be represented by a hardware address of just
the outbox ID: outgoing packets are sent via the outbox to the remote
endpoint, so are "addressed" to that outbox ID
... but that doesn't seem to be the approach you want to take here, as
it doesn't match your requirements for an interface lladdr (also,
implementing the neighbour-handling infrastructure for that would seem
to be overkill for a strictly peer-to-peer link type).
So a couple of queries to get us to a decision:
Your goal with exposing the channel numbers is more to choose the
correct *local* interface to use on a system, right? Can you elaborate
on your objections for using something like sysfs attributes for that?
Can you outline the intended usage of the spec updates that would add
the address format you mentioned? Is there a use-case we need to
consider for those?
Cheers,
Jeremy
On 11/11/24 20:00, Jeremy Kerr wrote: > [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.] > > > Hi Adam, >> "The PCC signature. The signature of a subspace is computed by a >> bitwise-or of the value 0x50434300 with the subspace ID. For example, >> subspace 3 has the signature 0x50434303." >> >> This could be used, but the inclusion of the "PCC" is unnecessary, as >> it is on every packet. Thus only the subspace ID is relevant. This >> is the index of the entry in the PCCT, and is what I have been >> calling the outbox ID. Thus it is half of the address that I am >> proposing. > But the signature value isn't implementing any MCTP-bus addressing > functionality, right? It's a fixed value that has to be set the same > way on all transactions using that PCC channel. > > Just to walk it back a bit, we have two possible interpretations here: > > 1) that the channel indexes *do* form something like a physical > addressing mechanism; when packets are sent over a channel to a > remote endpoint, we need to add a specific channel identifier. > > 2) that the channel indices are more of an internal detail of the > transport mechanism: they're identifying channels, not MCTP > endpoints (kinda like setting the PCIe target address when > transmitting a network packet, perhaps?) > > If we adopt the (1) approach, we'd want a hardware address to represent > the mechanism for addressing an MCTP endpoint, not an interface > instance. That would end up with something along the lines of: > > - MCTP-over-PCC hardware addresses would be a single byte (to contain a > channel ID) > > - the interface would have a hardware address of just the inbox ID: > incoming packets are received via the inbox to the local interface, > and so are "addressed" to that inbox ID > > - remote endpoints would be represented by a hardware address of just > the outbox ID: outgoing packets are sent via the outbox to the remote > endpoint, so are "addressed" to that outbox ID > > ... but that doesn't seem to be the approach you want to take here, as > it doesn't match your requirements for an interface lladdr (also, > implementing the neighbour-handling infrastructure for that would seem > to be overkill for a strictly peer-to-peer link type). > > So a couple of queries to get us to a decision: > > Your goal with exposing the channel numbers is more to choose the > correct *local* interface to use on a system, right? Can you elaborate > on your objections for using something like sysfs attributes for that? > > Can you outline the intended usage of the spec updates that would add > the address format you mentioned? Is there a use-case we need to > consider for those? On consideration that the spec is still closed, and may change between now and when it is published, I am going to pull out the hardware address from this patch. Once we have a public spec, we can implement it without fear of breaking userland. > > Cheers, > > > Jeremy
© 2016 - 2024 Red Hat, Inc.