* [PATCH v6 0/2] MCTP Over PCC Transport
@ 2024-10-29 16:54 admiyo
2024-10-29 16:54 ` [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK admiyo
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
0 siblings, 2 replies; 23+ messages in thread
From: admiyo @ 2024-10-29 16:54 UTC (permalink / raw)
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Sudeep Holla, Jonathan Cameron, Huisong Li
From: Adam Young <admiyo@os.amperecomputing.com>
This series adds support for the Management Control Transport Protocol (MCTP)
over the Platform Communication Channel (PCC) mechanism.
DMTF DSP:0292
https://www.dmtf.org/sites/default/files/standards/documents/DSP0292_1.0.0WIP50.pdf
MCTP defines a communication model intended to
facilitate communication between Management controllers
and other management controllers, and between Management
controllers and management devices
PCC is a mechanism for communication between components within
the Platform. It is a composed of shared memory regions,
interrupt registers, and status registers.
The MCTP over PCC driver makes use of two PCC channels. For
sending messages, it uses a Type 3 channel, and for receiving
messages it uses the paired Type 4 channel. The device
and corresponding channels are specified via ACPI.
The first patch in the series implements a mechanism to allow the driver
to indicate whether an ACK should be sent back to the caller
after processing the interrupt. This is an optional feature in
the PCC code, but has been made explicitly required in another driver.
The implementation here maintains the backwards compatibility of that
driver.
MCTP is a general purpose protocol so it would be impossible to enumerate
all the use cases, but some of the ones that are most topical are attestation
and RAS support. There are a handful of protocols built on top of MCTP, to
include PLDM and SPDM, both specified by the DMTF.
https://www.dmtf.org/sites/default/files/standards/documents/DSP0240_1.0.0.pdf
https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.3.0.pd
SPDM entails various usages, including device identity collection, device
authentication, measurement collection, and device secure session establishment.
PLDM is more likely to be used for hardware support: temperature, voltage, or
fan sensor control.
At least two companies have devices that can make use of the mechanism. One is
Ampere Computing, my employer.
The mechanism it uses is called Platform Communication Channels is part of the
ACPI spec: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html
Since it is a socket interface, the system administrator also has the ability
to ignore an MCTP link that they do not want to enable. This link would be visible
to the end user, but would not be usable.
If MCTP support is disabled in the Kernel, this driver would also be disabled.
PCC is based on a shared buffer and a set of I/O mapped memory locations that the
Spec calls registers. This mechanism exists regardless of the existence of the
driver. Thus, if the user has the ability to map these physical location to
virtual locations, they have the ability to drive the hardware. Thus, there
is a security aspect to this mechanism that extends beyond the responsibilities
of the operating system.
If the hardware does not expose the PCC in the ACPI table, this device will never
be enabled. Thus it is only an issue on hard that does support PCC. In that case,
it is up to the remote controller to sanitize communication; MCTP will be exposed
as a socket interface, and userland can send any crafted packet it wants. It would
thus also be incumbent on the hardware manufacturer to allow the end user to disable
MCTP over PCC communication if they did not want to expose it.
Previous Version:
https://lore.kernel.org/all/20240712023626.1010559-1-admiyo@os.amperecomputing.com/
Changes in V6:
- Removed patch for ACPICA code that has merged
- Includes the hardware address in the network device
- Converted all device resources to devm resources
- Removed mctp_pcc_driver_remove function
- uses acpi_driver_module for initialization
- created helper stucture for in and out mailboxes
- Consolidated code for initializing mailboxes in the add_device function
- Added specification references
- Removed duplicate constant PCC_ACK_FLAG_MASK
- Use the MCTP_SIGNATURE_LENGTH define
- made naming of header structs consistent
- use sizeof local variables for offset calculations
- prefix structure name to avoid potential clash
- removed unneccessary null initialization from acpi_device_id
Changes in V5
- Removed Owner field from ACPI module declaration
- removed unused next field from struct mctp_pcc_ndev
- Corrected logic reading RX ACK flag.
- Added comment for struct pcc_chan_info field shmem_base_addr
- check against current mtu instead of max mtu for packet length\
- removed unnecessary lookups of pnd->mdev.dev
Changes in V4
- Read flags out of shared buffer to trigger ACK for Type 4 RX
- Remove list of netdevs and cleanup from devices only
- tag PCCT protocol headers as little endian
- Remove unused constants
Changes in V3
- removed unused header
- removed spurious space
- removed spurious semis after functiomns
- removed null assignment for init
- remove redundant set of device on skb
- tabify constant declarations
- added rtnl_link_stats64 function
- set MTU to minimum to start
- clean up logic on driver removal
- remove cast on void * assignment
- call cleanup function directly
- check received length before allocating skb
- introduce symbolic constatn for ACK FLAG MASK
- symbolic constant for PCC header flag.
- Add namespace ID to PCC magic
- replaced readls with copy from io of PCC header
- replaced custom modules init and cleanup with ACPI version
Changes in V2
- All Variable Declarations are in reverse Xmass Tree Format
- All Checkpatch Warnings Are Fixed
- Removed Dead code
- Added packet tx/rx stats
- Removed network physical address. This is still in
disucssion in the spec, and will be added once there
is consensus. The protocol can be used with out it.
This also lead to the removal of the Big Endian
conversions.
- Avoided using non volatile pointers in copy to and from io space
- Reorderd the patches to put the ACK check for the PCC Mailbox
as a pre-requisite. The corresponding change for the MCTP
driver has been inlined in the main patch.
- Replaced magic numbers with constants, fixed typos, and other
minor changes from code review.
Adam Young (2):
mctp pcc: Check before sending MCTP PCC response ACK
mctp pcc: Implement MCTP over PCC Transport
drivers/mailbox/pcc.c | 35 +++-
drivers/net/mctp/Kconfig | 13 ++
drivers/net/mctp/Makefile | 1 +
drivers/net/mctp/mctp-pcc.c | 332 ++++++++++++++++++++++++++++++++++++
include/acpi/pcc.h | 8 +
5 files changed, 381 insertions(+), 8 deletions(-)
create mode 100644 drivers/net/mctp/mctp-pcc.c
--
2.34.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK
2024-10-29 16:54 [PATCH v6 0/2] MCTP Over PCC Transport admiyo
@ 2024-10-29 16:54 ` admiyo
2024-10-30 9:45 ` lihuisong (C)
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
1 sibling, 1 reply; 23+ messages in thread
From: admiyo @ 2024-10-29 16:54 UTC (permalink / raw)
To: Sudeep Holla, Jassi Brar, Robert Moore, Rafael J. Wysocki,
Len Brown
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Cameron, Huisong Li
From: Adam Young <admiyo@os.amperecomputing.com>
Type 4 PCC channels have an option to send back a response
to the platform when they are done processing the request.
The flag to indicate whether or not to respond is inside
the message body, and thus is not available to the pcc
mailbox.
In order to read the flag, this patch maps the shared
buffer to virtual memory.
If the flag is not set, still set command completion
bit after processing message.
Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
drivers/mailbox/pcc.c | 35 +++++++++++++++++++++++++++--------
include/acpi/pcc.h | 8 ++++++++
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 94885e411085..b2a66e8a6cd6 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -90,6 +90,7 @@ struct pcc_chan_reg {
* @cmd_complete: PCC register bundle for the command complete check register
* @cmd_update: PCC register bundle for the command complete update register
* @error: PCC register bundle for the error status register
+ * @shmem_base_addr: the virtual memory address of the shared buffer
* @plat_irq: platform interrupt
* @type: PCC subspace type
* @plat_irq_flags: platform interrupt flags
@@ -107,6 +108,7 @@ struct pcc_chan_info {
struct pcc_chan_reg cmd_complete;
struct pcc_chan_reg cmd_update;
struct pcc_chan_reg error;
+ void __iomem *shmem_base_addr;
int plat_irq;
u8 type;
unsigned int plat_irq_flags;
@@ -269,6 +271,27 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
return !!val;
}
+static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
+{
+ struct pcc_extended_type_hdr pcc_hdr;
+
+ if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ return;
+ memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
+ sizeof(struct pcc_extended_type_hdr));
+ /*
+ * The PCC slave subspace channel needs to set the command complete bit
+ * after processing message. If the PCC_ACK_FLAG is set, it should also
+ * ring the doorbell.
+ *
+ * The PCC master subspace channel clears chan_in_use to free channel.
+ */
+ if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
+ pcc_send_data(chan, NULL);
+ else
+ pcc_chan_reg_read_modify_write(&pchan->cmd_update);
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -306,14 +329,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
mbox_chan_received_data(chan, NULL);
- /*
- * The PCC slave subspace channel needs to set the command complete bit
- * and ring doorbell after processing message.
- *
- * The PCC master subspace channel clears chan_in_use to free channel.
- */
- if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
- pcc_send_data(chan, NULL);
+ check_and_ack(pchan, chan);
pchan->chan_in_use = false;
return IRQ_HANDLED;
@@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
if (rc)
return ERR_PTR(rc);
+ pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
+ pchan->chan.shmem_base_addr,
+ pchan->chan.shmem_size);
return &pchan->chan;
}
EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 9b373d172a77..0bcb86dc4de7 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -18,6 +18,13 @@ struct pcc_mbox_chan {
u16 min_turnaround_time;
};
+struct pcc_extended_type_hdr {
+ __le32 signature;
+ __le32 flags;
+ __le32 length;
+ char command[4];
+};
+
/* Generic Communications Channel Shared Memory Region */
#define PCC_SIGNATURE 0x50434300
/* Generic Communications Channel Command Field */
@@ -31,6 +38,7 @@ struct pcc_mbox_chan {
#define PCC_CMD_COMPLETION_NOTIFY BIT(0)
#define MAX_PCC_SUBSPACES 256
+#define PCC_ACK_FLAG_MASK 0x1
#ifdef CONFIG_PCC
extern struct pcc_mbox_chan *
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-29 16:54 [PATCH v6 0/2] MCTP Over PCC Transport admiyo
2024-10-29 16:54 ` [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK admiyo
@ 2024-10-29 16:54 ` admiyo
2024-10-31 1:28 ` Jeremy Kerr
` (4 more replies)
1 sibling, 5 replies; 23+ messages in thread
From: admiyo @ 2024-10-29 16:54 UTC (permalink / raw)
To: Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK
2024-10-29 16:54 ` [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK admiyo
@ 2024-10-30 9:45 ` lihuisong (C)
2024-11-01 0:16 ` Adam Young
0 siblings, 1 reply; 23+ messages in thread
From: lihuisong (C) @ 2024-10-30 9:45 UTC (permalink / raw)
To: admiyo
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Len Brown, Rafael J. Wysocki,
Robert Moore, Jakub Kicinski, Paolo Abeni, Jonathan Cameron,
Jassi Brar, Sudeep Holla
Hi Adam,
在 2024/10/30 0:54, admiyo@os.amperecomputing.com 写道:
> From: Adam Young <admiyo@os.amperecomputing.com>
>
> Type 4 PCC channels have an option to send back a response
> to the platform when they are done processing the request.
> The flag to indicate whether or not to respond is inside
> the message body, and thus is not available to the pcc
> mailbox.
>
> In order to read the flag, this patch maps the shared
> buffer to virtual memory.
>
> If the flag is not set, still set command completion
> bit after processing message.
>
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
> drivers/mailbox/pcc.c | 35 +++++++++++++++++++++++++++--------
> include/acpi/pcc.h | 8 ++++++++
> 2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 94885e411085..b2a66e8a6cd6 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -90,6 +90,7 @@ struct pcc_chan_reg {
> * @cmd_complete: PCC register bundle for the command complete check register
> * @cmd_update: PCC register bundle for the command complete update register
> * @error: PCC register bundle for the error status register
> + * @shmem_base_addr: the virtual memory address of the shared buffer
> * @plat_irq: platform interrupt
> * @type: PCC subspace type
> * @plat_irq_flags: platform interrupt flags
> @@ -107,6 +108,7 @@ struct pcc_chan_info {
> struct pcc_chan_reg cmd_complete;
> struct pcc_chan_reg cmd_update;
> struct pcc_chan_reg error;
> + void __iomem *shmem_base_addr;
> int plat_irq;
> u8 type;
> unsigned int plat_irq_flags;
> @@ -269,6 +271,27 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> return !!val;
> }
>
> +static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> +{
> + struct pcc_extended_type_hdr pcc_hdr;
> +
> + if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> + return;
> + memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
> + sizeof(struct pcc_extended_type_hdr));
> + /*
> + * The PCC slave subspace channel needs to set the command complete bit
> + * after processing message. If the PCC_ACK_FLAG is set, it should also
> + * ring the doorbell.
> + *
> + * The PCC master subspace channel clears chan_in_use to free channel.
> + */
> + if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
> + pcc_send_data(chan, NULL);
> + else
> + pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> +}
> +
> /**
> * pcc_mbox_irq - PCC mailbox interrupt handler
> * @irq: interrupt number
> @@ -306,14 +329,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>
> mbox_chan_received_data(chan, NULL);
>
> - /*
> - * The PCC slave subspace channel needs to set the command complete bit
> - * and ring doorbell after processing message.
> - *
> - * The PCC master subspace channel clears chan_in_use to free channel.
> - */
> - if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> - pcc_send_data(chan, NULL);
> + check_and_ack(pchan, chan);
> pchan->chan_in_use = false;
>
> return IRQ_HANDLED;
> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
> if (rc)
> return ERR_PTR(rc);
>
> + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
> + pchan->chan.shmem_base_addr,
> + pchan->chan.shmem_size);
Currently, the PCC mbox client does ioremap after requesting PCC channel.
Thus all current clients will ioremap twice. This is not good to me.
How about add a new interface and give the type4 client the right to
decide whether to reply in rx_callback?
> return &pchan->chan;
> }
> EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 9b373d172a77..0bcb86dc4de7 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -18,6 +18,13 @@ struct pcc_mbox_chan {
> u16 min_turnaround_time;
> };
>
> +struct pcc_extended_type_hdr {
> + __le32 signature;
> + __le32 flags;
> + __le32 length;
> + char command[4];
> +};
No need to define this new structure and directly use "struct
acpi_pcct_ext_pcc_shared_memory".
> +
> /* Generic Communications Channel Shared Memory Region */
> #define PCC_SIGNATURE 0x50434300
> /* Generic Communications Channel Command Field */
> @@ -31,6 +38,7 @@ struct pcc_mbox_chan {
> #define PCC_CMD_COMPLETION_NOTIFY BIT(0)
>
> #define MAX_PCC_SUBSPACES 256
> +#define PCC_ACK_FLAG_MASK 0x1
directly use the macro PCC_CMD_COMPLETION_NOTIF
>
> #ifdef CONFIG_PCC
> extern struct pcc_mbox_chan *
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
@ 2024-10-31 1:28 ` Jeremy Kerr
2024-10-31 15:50 ` Adam Young
2024-10-31 11:26 ` kernel test robot
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Jeremy Kerr @ 2024-10-31 1:28 UTC (permalink / raw)
To: admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
2024-10-31 1:28 ` Jeremy Kerr
@ 2024-10-31 11:26 ` kernel test robot
2024-10-31 12:07 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2024-10-31 11:26 UTC (permalink / raw)
To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: oe-kbuild-all, netdev, linux-kernel, Sudeep Holla,
Jonathan Cameron, Huisong Li
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: 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
2024-10-31 1:28 ` Jeremy Kerr
2024-10-31 11:26 ` kernel test robot
@ 2024-10-31 12:07 ` kernel test robot
2024-10-31 12:38 ` kernel test robot
2024-10-31 13:09 ` kernel test robot
4 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2024-10-31 12:07 UTC (permalink / raw)
To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: oe-kbuild-all, netdev, linux-kernel, Sudeep Holla,
Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
` (2 preceding siblings ...)
2024-10-31 12:07 ` kernel test robot
@ 2024-10-31 12:38 ` kernel test robot
2024-10-31 13:09 ` kernel test robot
4 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2024-10-31 12:38 UTC (permalink / raw)
To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Paul Gazzillo, Necip Fazil Yildiran, oe-kbuild-all, netdev,
linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
` (3 preceding siblings ...)
2024-10-31 12:38 ` kernel test robot
@ 2024-10-31 13:09 ` kernel test robot
4 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2024-10-31 13:09 UTC (permalink / raw)
To: admiyo, Jeremy Kerr, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: llvm, oe-kbuild-all, netdev, linux-kernel, Sudeep Holla,
Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-31 1:28 ` Jeremy Kerr
@ 2024-10-31 15:50 ` Adam Young
2024-11-01 8:55 ` Jeremy Kerr
0 siblings, 1 reply; 23+ messages in thread
From: Adam Young @ 2024-10-31 15:50 UTC (permalink / raw)
To: Jeremy Kerr, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK
2024-10-30 9:45 ` lihuisong (C)
@ 2024-11-01 0:16 ` Adam Young
2024-11-01 1:30 ` lihuisong (C)
0 siblings, 1 reply; 23+ messages in thread
From: Adam Young @ 2024-11-01 0:16 UTC (permalink / raw)
To: lihuisong (C), admiyo
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Len Brown, Rafael J. Wysocki,
Robert Moore, Jakub Kicinski, Paolo Abeni, Jonathan Cameron,
Jassi Brar, Sudeep Holla
On 10/30/24 05:45, lihuisong (C) wrote:
>> + check_and_ack(pchan, chan);
>> pchan->chan_in_use = false;
>> return IRQ_HANDLED;
>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client *cl,
>> int subspace_id)
>> if (rc)
>> return ERR_PTR(rc);
>> + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>> + pchan->chan.shmem_base_addr,
>> + pchan->chan.shmem_size);
> Currently, the PCC mbox client does ioremap after requesting PCC channel.
> Thus all current clients will ioremap twice. This is not good to me.
> How about add a new interface and give the type4 client the right to
> decide whether to reply in rx_callback?
I do agree that is a cleaner implementation, but I don't have a way of
testing the other drivers, and did not want to break them. I think your
driver is the only that makes use of it, so we can certainly come up
with a common approach.
The mailbox interface does not allow a return code from
mbox_chan_received_data, which is what I originally wanted. If that
could return multiple status codes, one of them could indicate the need
to send the interrupt back. Otherwise, we need to query the driver to
read the shared memory again.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK
2024-11-01 0:16 ` Adam Young
@ 2024-11-01 1:30 ` lihuisong (C)
2024-11-02 15:34 ` Adam Young
0 siblings, 1 reply; 23+ messages in thread
From: lihuisong (C) @ 2024-11-01 1:30 UTC (permalink / raw)
To: Adam Young, admiyo
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Len Brown, Rafael J. Wysocki,
Robert Moore, Jakub Kicinski, Paolo Abeni, Jonathan Cameron,
Jassi Brar, Sudeep Holla
Hi Adam,
All modifications in the patch is done for pcc instead of mctp.
Suggest that use the prefix "mailbox: pcc: xxxx".
Please find my following reply.
在 2024/11/1 8:16, Adam Young 写道:
>
> On 10/30/24 05:45, lihuisong (C) wrote:
>>> + check_and_ack(pchan, chan);
>>> pchan->chan_in_use = false;
>>> return IRQ_HANDLED;
>>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client *cl,
>>> int subspace_id)
>>> if (rc)
>>> return ERR_PTR(rc);
>>> + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>>> + pchan->chan.shmem_base_addr,
>>> + pchan->chan.shmem_size);
>> Currently, the PCC mbox client does ioremap after requesting PCC
>> channel.
>> Thus all current clients will ioremap twice. This is not good to me.
>> How about add a new interface and give the type4 client the right to
>> decide whether to reply in rx_callback?
>
>
> I do agree that is a cleaner implementation, but I don't have a way of
> testing the other drivers, and did not want to break them. I think
> your driver is the only that makes use of it, so we can certainly come
> up with a common approach.
I understand what you are concerned about.
But this duplicate ioremap also works for all PCC clients no matter
which type they used. It has very wide influence.
My driver just uses type3 instead of type4. What's more, AFAICS, it
doesn't seem there is type4 client driver in linux.
Therefore, determining whether type4 client driver needs to reply to
platform has the minimum or even no impact in their rx_callback.
>
> The mailbox interface does not allow a return code from
> mbox_chan_received_data, which is what I originally wanted. If that
> could return multiple status codes, one of them could indicate the
> need to send the interrupt back. Otherwise, we need to query the
> driver to read the shared memory again.
yes
>
> .
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-10-31 15:50 ` Adam Young
@ 2024-11-01 8:55 ` Jeremy Kerr
2024-11-01 21:19 ` Adam Young
2024-11-05 20:16 ` Adam Young
0 siblings, 2 replies; 23+ messages in thread
From: Jeremy Kerr @ 2024-11-01 8:55 UTC (permalink / raw)
To: Adam Young, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-11-01 8:55 ` Jeremy Kerr
@ 2024-11-01 21:19 ` Adam Young
2024-11-05 14:09 ` Jeremy Kerr
2024-11-05 20:16 ` Adam Young
1 sibling, 1 reply; 23+ messages in thread
From: Adam Young @ 2024-11-01 21:19 UTC (permalink / raw)
To: Jeremy Kerr, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK
2024-11-01 1:30 ` lihuisong (C)
@ 2024-11-02 15:34 ` Adam Young
2024-11-04 3:17 ` lihuisong (C)
0 siblings, 1 reply; 23+ messages in thread
From: Adam Young @ 2024-11-02 15:34 UTC (permalink / raw)
To: lihuisong (C), admiyo
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Len Brown, Rafael J. Wysocki,
Robert Moore, Jakub Kicinski, Paolo Abeni, Jonathan Cameron,
Jassi Brar, Sudeep Holla
On 10/31/24 21:30, lihuisong (C) wrote:
>>
>> On 10/30/24 05:45, lihuisong (C) wrote:
>>>> + check_and_ack(pchan, chan);
>>>> pchan->chan_in_use = false;
>>>> return IRQ_HANDLED;
>>>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client
>>>> *cl, int subspace_id)
>>>> if (rc)
>>>> return ERR_PTR(rc);
>>>> + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>>>> + pchan->chan.shmem_base_addr,
>>>> + pchan->chan.shmem_size);
>>> Currently, the PCC mbox client does ioremap after requesting PCC
>>> channel.
>>> Thus all current clients will ioremap twice. This is not good to me.
>>> How about add a new interface and give the type4 client the right to
>>> decide whether to reply in rx_callback?
>>
>>
>> I do agree that is a cleaner implementation, but I don't have a way
>> of testing the other drivers, and did not want to break them. I think
>> your driver is the only that makes use of it, so we can certainly
>> come up with a common approach.
> I understand what you are concerned about.
> But this duplicate ioremap also works for all PCC clients no matter
> which type they used. It has very wide influence.
> My driver just uses type3 instead of type4. What's more, AFAICS, it
> doesn't seem there is type4 client driver in linux.
> Therefore, determining whether type4 client driver needs to reply to
> platform has the minimum or even no impact in their rx_callback.
I can move the place where we hold on to the shmem from struct
pcc_chan_info in pcc.c, where it is local to the file, to struct
pcc_mbox_chan in include/acpi/pcc.h where it will be visible from both
files. With that change, we only need ioremap once for the segment.
I don't like adding the callback decision in the driver: it is part of
the protocol, and should be enforced by the pcc layer. If we do it in
the driver, the logic will be duplicated by each driver.
I could make a further change and allow the driver to request the
remapped memory segment from the pcc layer, and couple to the
memory-remap to the client/channel. It seems like that code, too,
should be in the common layer. However most drivers would not know to
use this function yet, so the mechanism would have to be optional, and
only clean up if called this way.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK
2024-11-02 15:34 ` Adam Young
@ 2024-11-04 3:17 ` lihuisong (C)
2024-11-04 18:44 ` Adam Young
0 siblings, 1 reply; 23+ messages in thread
From: lihuisong (C) @ 2024-11-04 3:17 UTC (permalink / raw)
To: Adam Young, admiyo
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Len Brown, Rafael J. Wysocki,
Robert Moore, Jakub Kicinski, Paolo Abeni, Jonathan Cameron,
Jassi Brar, Sudeep Holla
在 2024/11/2 23:34, Adam Young 写道:
>
> On 10/31/24 21:30, lihuisong (C) wrote:
>>>
>>> On 10/30/24 05:45, lihuisong (C) wrote:
>>>>> + check_and_ack(pchan, chan);
>>>>> pchan->chan_in_use = false;
>>>>> return IRQ_HANDLED;
>>>>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client
>>>>> *cl, int subspace_id)
>>>>> if (rc)
>>>>> return ERR_PTR(rc);
>>>>> + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>>>>> + pchan->chan.shmem_base_addr,
>>>>> + pchan->chan.shmem_size);
>>>> Currently, the PCC mbox client does ioremap after requesting PCC
>>>> channel.
>>>> Thus all current clients will ioremap twice. This is not good to me.
>>>> How about add a new interface and give the type4 client the right
>>>> to decide whether to reply in rx_callback?
>>>
>>>
>>> I do agree that is a cleaner implementation, but I don't have a way
>>> of testing the other drivers, and did not want to break them. I
>>> think your driver is the only that makes use of it, so we can
>>> certainly come up with a common approach.
>> I understand what you are concerned about.
>> But this duplicate ioremap also works for all PCC clients no matter
>> which type they used. It has very wide influence.
>> My driver just uses type3 instead of type4. What's more, AFAICS, it
>> doesn't seem there is type4 client driver in linux.
>> Therefore, determining whether type4 client driver needs to reply to
>> platform has the minimum or even no impact in their rx_callback.
>
> I can move the place where we hold on to the shmem from struct
> pcc_chan_info in pcc.c, where it is local to the file, to struct
> pcc_mbox_chan in include/acpi/pcc.h where it will be visible from
> both files. With that change, we only need ioremap once for the segment.
>
> I don't like adding the callback decision in the driver: it is part
> of the protocol, and should be enforced by the pcc layer. If we do it
> in the driver, the logic will be duplicated by each driver.
Yes
>
> I could make a further change and allow the driver to request the
> remapped memory segment from the pcc layer, and couple to the
> memory-remap to the client/channel. It seems like that code, too,
> should be in the common layer. However most drivers would not know to
> use this function yet, so the mechanism would have to be optional,
> and only clean up if called this way.
I agree this method.
Don't remap twice for one shared memory.
This remaping is reasonable in PCC layer. We can let PCC client to
decide if PCC layer does remap and then they use it directly.
For new driver like the driver you are uploading, driver can give PCC
one flag to tell PCC layer remap when request channel.
For old PCC client driver, do not send this flag, PPC layer do not
remap. So no any impact on them.
>
>
>
>
>
>
> .
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK
2024-11-04 3:17 ` lihuisong (C)
@ 2024-11-04 18:44 ` Adam Young
0 siblings, 0 replies; 23+ messages in thread
From: Adam Young @ 2024-11-04 18:44 UTC (permalink / raw)
To: lihuisong (C), admiyo
Cc: netdev, linux-kernel, Jeremy Kerr, Matt Johnston,
David S . Miller, Eric Dumazet, Len Brown, Rafael J. Wysocki,
Robert Moore, Jakub Kicinski, Paolo Abeni, Jonathan Cameron,
Jassi Brar, Sudeep Holla
On 11/3/24 22:17, lihuisong (C) wrote:
>>
>> I could make a further change and allow the driver to request the
>> remapped memory segment from the pcc layer, and couple to the
>> memory-remap to the client/channel. It seems like that code, too,
>> should be in the common layer. However most drivers would not know
>> to use this function yet, so the mechanism would have to be
>> optional, and only clean up if called this way.
> I agree this method.
> Don't remap twice for one shared memory.
> This remaping is reasonable in PCC layer. We can let PCC client to
> decide if PCC layer does remap and then they use it directly.
> For new driver like the driver you are uploading, driver can give PCC
> one flag to tell PCC layer remap when request channel.
> For old PCC client driver, do not send this flag, PPC layer do not
> remap. So no any impact on them.
I think we are actually in agreement here. No double mapping, but the
driver MAY request the mapping happen in the PCC layer. No impact on
existing drivers.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-11-01 21:19 ` Adam Young
@ 2024-11-05 14:09 ` Jeremy Kerr
2024-11-06 15:59 ` Adam Young
0 siblings, 1 reply; 23+ messages in thread
From: Jeremy Kerr @ 2024-11-05 14:09 UTC (permalink / raw)
To: Adam Young, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-11-01 8:55 ` Jeremy Kerr
2024-11-01 21:19 ` Adam Young
@ 2024-11-05 20:16 ` Adam Young
2024-11-06 10:47 ` Jeremy Kerr
1 sibling, 1 reply; 23+ messages in thread
From: Adam Young @ 2024-11-05 20:16 UTC (permalink / raw)
To: Jeremy Kerr, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-11-05 20:16 ` Adam Young
@ 2024-11-06 10:47 ` Jeremy Kerr
0 siblings, 0 replies; 23+ messages in thread
From: Jeremy Kerr @ 2024-11-06 10:47 UTC (permalink / raw)
To: Adam Young, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-11-05 14:09 ` Jeremy Kerr
@ 2024-11-06 15:59 ` Adam Young
2024-11-12 1:00 ` Jeremy Kerr
0 siblings, 1 reply; 23+ messages in thread
From: Adam Young @ 2024-11-06 15:59 UTC (permalink / raw)
To: Jeremy Kerr, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-11-06 15:59 ` Adam Young
@ 2024-11-12 1:00 ` Jeremy Kerr
2024-11-13 18:41 ` Adam Young
0 siblings, 1 reply; 23+ messages in thread
From: Jeremy Kerr @ 2024-11-12 1:00 UTC (permalink / raw)
To: Adam Young, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport
2024-11-12 1:00 ` Jeremy Kerr
@ 2024-11-13 18:41 ` Adam Young
0 siblings, 0 replies; 23+ messages in thread
From: Adam Young @ 2024-11-13 18:41 UTC (permalink / raw)
To: Jeremy Kerr, admiyo, Matt Johnston, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Sudeep Holla, Jonathan Cameron, Huisong Li
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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2024-11-13 18:41 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-29 16:54 [PATCH v6 0/2] MCTP Over PCC Transport admiyo
2024-10-29 16:54 ` [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response ACK admiyo
2024-10-30 9:45 ` lihuisong (C)
2024-11-01 0:16 ` Adam Young
2024-11-01 1:30 ` lihuisong (C)
2024-11-02 15:34 ` Adam Young
2024-11-04 3:17 ` lihuisong (C)
2024-11-04 18:44 ` Adam Young
2024-10-29 16:54 ` [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport admiyo
2024-10-31 1:28 ` Jeremy Kerr
2024-10-31 15:50 ` Adam Young
2024-11-01 8:55 ` Jeremy Kerr
2024-11-01 21:19 ` Adam Young
2024-11-05 14:09 ` Jeremy Kerr
2024-11-06 15:59 ` Adam Young
2024-11-12 1:00 ` Jeremy Kerr
2024-11-13 18:41 ` Adam Young
2024-11-05 20:16 ` Adam Young
2024-11-06 10:47 ` Jeremy Kerr
2024-10-31 11:26 ` kernel test robot
2024-10-31 12:07 ` kernel test robot
2024-10-31 12:38 ` kernel test robot
2024-10-31 13:09 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox