[PATCH v3 0/3] MCTP over PCC

[PATCH v3 0/3] MCTP over PCC

[admiyo]

From: Adam Young <admiyo@os.amperecomputing.com>

This series adds support for the Management Control Transport Protocol (MCTP)
over the Platform Communication Channel (PCC) mechanism.

MCTP defines a communication model intended to
facilitate communication between Management controllers
and other management controllers, and between Management
controllers and management devices

PCC is a mechanism for communication between components within
the  Platform.  It is a composed of shared memory regions,
interrupt registers, and status registers.

The MCTP over PCC driver makes use of two PCC channels. For
sending messages, it uses a Type 3 channel, and for receiving
messages it uses the paired Type 4 channel.  The device
and corresponding channels are specified via ACPI.

The first patch in the series implements a mechanism to allow the driver
to indicate whether an ACK should be sent back to the caller
after processing the interrupt.  This is an optional feature in
the PCC code, but has been made explicitly required in another driver.
The implementation here maintains the backwards compatibility of that
driver.

The second patch in the series is the required change from ACPICA
code that will be imported into the Linux kernel when synchronized
with the ACPICA repository. It ahs already merged there and will
be merged in as is.  It is included here so that the patch series
can run and be tested prior to that merge.

Previous Version:
https://lore.kernel.org/20240619200552.119080-1-admiyo@os.amperecomputing.com/

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.

Code Review Change not made

- Did not change the module init unload function to use the
  ACPI equivalent as they do not remove all devices prior
  to unload, leading to dangling references and seg faults.

Adam Young (3):
  mctp pcc: Check before sending MCTP PCC response ACK
  mctp pcc: Allow PCC Data Type in MCTP resource.
  mctp pcc: Implement MCTP over PCC Transport

 drivers/acpi/acpica/rsaddr.c |   2 +-
 drivers/mailbox/pcc.c        |   6 +-
 drivers/net/mctp/Kconfig     |  13 ++
 drivers/net/mctp/Makefile    |   1 +
 drivers/net/mctp/mctp-pcc.c  | 343 +++++++++++++++++++++++++++++++++++
 include/acpi/pcc.h           |   1 +
 6 files changed, 364 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

-- 
2.34.1

[PATCH v3 1/3] mctp pcc: Check before sending MCTP PCC response ACK

[admiyo]

From: Adam Young <admiyo@amperecomputing.com>

Type 4 PCC channels have an option to send back a response
to the platform when they are done processing the request.
The flag to indicate whether or not to respond is inside
the message body, and thus is not available to the pcc
mailbox.  Since only one message can be processed at once per
channel, the value of this flag is checked during message processing
and passed back via the channels global structure.

Ideally, the mailbox callback function would return a value
indicating whether the message requires an ACK, but that
would be a change to the mailbox API.  That would involve
some change to all (about 12) of the mailbox based drivers,
and the majority of them would not need to know about the
ACK call.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/mailbox/pcc.c | 6 +++++-
 include/acpi/pcc.h    | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 94885e411085..5cf792700d79 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 {
 	struct pcc_chan_info *pchan;
 	struct mbox_chan *chan = p;
+	struct pcc_mbox_chan *pmchan;
 	u64 val;
 	int ret;
 
@@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
 		return IRQ_NONE;
 
+	pmchan = &pchan->chan;
+	pmchan->ack_rx = true;  //TODO default to False
 	mbox_chan_received_data(chan, NULL);
 
 	/*
@@ -312,7 +315,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	 *
 	 * The PCC master subspace channel clears chan_in_use to free channel.
 	 */
-	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE &&
+	    pmchan->ack_rx)
 		pcc_send_data(chan, NULL);
 	pchan->chan_in_use = false;
 
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 9b373d172a77..297913378c2b 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -16,6 +16,7 @@ struct pcc_mbox_chan {
 	u32 latency;
 	u32 max_access_rate;
 	u16 min_turnaround_time;
+	bool ack_rx;
 };
 
 /* Generic Communications Channel Shared Memory Region */
-- 
2.34.1

[PATCH v3 2/3] mctp pcc: Allow PCC Data Type in MCTP resource.

[admiyo]

From: Adam Young <admiyo@amperecomputing.com>

Note that this patch is for code that will be merged
in via ACPICA changes.  The corresponding patch in ACPCA
has already merged. Thus, no changes can be made to this patch.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/acpi/acpica/rsaddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c
index fff48001d7ef..7932c2a6ddde 100644
--- a/drivers/acpi/acpica/rsaddr.c
+++ b/drivers/acpi/acpica/rsaddr.c
@@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource,
 
 	/* Validate the Resource Type */
 
-	if ((address.resource_type > 2) && (address.resource_type < 0xC0)) {
+	if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 0x0A)) {
 		return (FALSE);
 	}
 
-- 
2.34.1

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

[admiyo]

From: Adam Young <admiyo@amperecomputing.com>

Implementation of network driver for
Management Control Transport Protocol(MCTP) over
Platform Communication Channel(PCC)

DMTF DSP:0292

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 | 343 ++++++++++++++++++++++++++++++++++++
 3 files changed, 357 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index ce9d2d2ccf3b..ff4effd8e99c 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C
 	  A MCTP protocol network device is created for each I3C bus
 	  having a "mctp-controller" devicetree property.
 
+config MCTP_TRANSPORT_PCC
+	tristate "MCTP  PCC transport"
+	select ACPI
+	help
+	  Provides a driver to access MCTP devices over PCC transport,
+	  A MCTP protocol network device is created via ACPI for each
+	  entry in the DST/SDST that matches the identifier. The Platform
+	  commuinucation channels are selected from the corresponding
+	  entries in the PCCT.
+
+	  Say y here if you need to connect to MCTP endpoints over PCC. To
+	  compile as a module, use m; the module will be called mctp-pcc.
+
 endmenu
 
 endif
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index e1cb99ced54a..492a9e47638f 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o
 obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
 obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
 obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
new file mode 100644
index 000000000000..9ef4eefabd58
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * mctp-pcc.c - Driver for MCTP over PCC.
+ * Copyright (c) 2024, Ampere Computing LLC
+ */
+
+#include <linux/acpi.h>
+#include <linux/if_arp.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/acrestyp.h>
+#include <acpi/actbl.h>
+#include <net/mctp.h>
+#include <net/mctpdevice.h>
+#include <acpi/pcc.h>
+
+#define SPDM_VERSION_OFFSET	1
+#define SPDM_REQ_RESP_OFFSET	2
+#define MCTP_PAYLOAD_LENGTH	256
+#define MCTP_CMD_LENGTH		4
+#define MCTP_PCC_VERSION	0x1 /* DSP0253 defines a single version: 1 */
+#define MCTP_SIGNATURE		"MCTP"
+#define SIGNATURE_LENGTH	4
+#define MCTP_HEADER_LENGTH	12
+#define MCTP_MIN_MTU		68
+#define PCC_MAGIC		0x50434300
+#define PCC_HEADER_FLAG_REQ_INT	0x1
+#define PCC_HEADER_FLAGS	PCC_HEADER_FLAG_REQ_INT
+#define PCC_DWORD_TYPE		0x0c
+#define PCC_ACK_FLAG_MASK	0x1
+
+struct mctp_pcc_hdr {
+	u32 signature;
+	u32 flags;
+	u32 length;
+	char mctp_signature[4];
+};
+
+struct mctp_pcc_hw_addr {
+	u32 inbox_index;
+	u32 outbox_index;
+};
+
+/* The netdev structure. One of these per PCC adapter. */
+struct mctp_pcc_ndev {
+	struct list_head next;
+	/* spinlock to serialize access to pcc buffer and registers*/
+	spinlock_t lock;
+	struct mctp_dev mdev;
+	struct acpi_device *acpi_device;
+	struct pcc_mbox_chan *in_chan;
+	struct pcc_mbox_chan *out_chan;
+	struct mbox_client outbox_client;
+	struct mbox_client inbox_client;
+	void __iomem *pcc_comm_inbox_addr;
+	void __iomem *pcc_comm_outbox_addr;
+	struct mctp_pcc_hw_addr hw_addr;
+};
+
+static struct list_head mctp_pcc_ndevs = LIST_HEAD_INIT(mctp_pcc_ndevs);
+
+static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
+{
+	struct mctp_pcc_ndev *mctp_pcc_dev;
+	struct mctp_pcc_hdr mctp_pcc_hdr;
+	struct mctp_skb_cb *cb;
+	struct sk_buff *skb;
+	void *skb_buf;
+	u32 data_len;
+	u32 flags;
+
+	mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
+	memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr,
+		      sizeof(struct mctp_pcc_hdr));
+	data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
+
+	if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) {
+		mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
+		return;
+	}
+
+	skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len);
+	if (!skb) {
+		mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
+		return;
+	}
+	mctp_pcc_dev->mdev.dev->stats.rx_packets++;
+	mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len;
+	skb->protocol = htons(ETH_P_MCTP);
+	skb_buf = skb_put(skb, data_len);
+	memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len);
+	skb_reset_mac_header(skb);
+	skb_pull(skb, sizeof(struct mctp_pcc_hdr));
+	skb_reset_network_header(skb);
+	cb = __mctp_cb(skb);
+	cb->halen = 0;
+	netif_rx(skb);
+
+	flags = mctp_pcc_hdr.flags;
+	mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0;
+}
+
+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mctp_pcc_hdr pcc_header;
+	struct mctp_pcc_ndev *mpnd;
+	void __iomem *buffer;
+	unsigned long flags;
+
+	ndev->stats.tx_bytes += skb->len;
+	ndev->stats.tx_packets++;
+	mpnd = netdev_priv(ndev);
+
+	spin_lock_irqsave(&mpnd->lock, flags);
+	buffer = mpnd->pcc_comm_outbox_addr;
+	pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index;
+	pcc_header.flags = PCC_HEADER_FLAGS;
+	memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
+	pcc_header.length = skb->len + SIGNATURE_LENGTH;
+	memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
+	memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
+	mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
+						    NULL);
+	spin_unlock_irqrestore(&mpnd->lock, flags);
+
+	dev_consume_skb_any(skb);
+	return NETDEV_TX_OK;
+}
+
+static void
+mctp_pcc_net_stats(struct net_device *net_dev,
+		   struct rtnl_link_stats64 *stats)
+{
+	struct mctp_pcc_ndev *mpnd;
+
+	mpnd = (struct mctp_pcc_ndev *)netdev_priv(net_dev);
+	stats->rx_errors = 0;
+	stats->rx_packets = mpnd->mdev.dev->stats.rx_packets;
+	stats->tx_packets = mpnd->mdev.dev->stats.tx_packets;
+	stats->rx_dropped = 0;
+	stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes;
+	stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes;
+}
+
+static const struct net_device_ops mctp_pcc_netdev_ops = {
+	.ndo_start_xmit = mctp_pcc_tx,
+	.ndo_get_stats64 = mctp_pcc_net_stats,
+};
+
+static void  mctp_pcc_setup(struct net_device *ndev)
+{
+	ndev->type = ARPHRD_MCTP;
+	ndev->hard_header_len = 0;
+	ndev->addr_len = 0;
+	ndev->tx_queue_len = 0;
+	ndev->flags = IFF_NOARP;
+	ndev->netdev_ops = &mctp_pcc_netdev_ops;
+	ndev->needs_free_netdev = true;
+}
+
+struct lookup_context {
+	int index;
+	u32 inbox_index;
+	u32 outbox_index;
+};
+
+static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
+				       void *context)
+{
+	struct acpi_resource_address32 *addr;
+	struct lookup_context *luc = context;
+
+	switch (ares->type) {
+	case PCC_DWORD_TYPE:
+		break;
+	default:
+		return AE_OK;
+	}
+
+	addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data);
+	switch (luc->index) {
+	case 0:
+		luc->outbox_index = addr[0].address.minimum;
+		break;
+	case 1:
+		luc->inbox_index = addr[0].address.minimum;
+		break;
+	}
+	luc->index++;
+	return AE_OK;
+}
+
+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
+{
+	struct lookup_context context = {0, 0, 0};
+	struct mctp_pcc_ndev *mctp_pcc_dev;
+	struct net_device *ndev;
+	acpi_handle dev_handle;
+	acpi_status status;
+	struct device *dev;
+	int mctp_pcc_mtu;
+	int outbox_index;
+	int inbox_index;
+	char name[32];
+	int rc;
+
+	dev_dbg(&acpi_dev->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(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
+		return -EINVAL;
+	}
+	inbox_index = context.inbox_index;
+	outbox_index = context.outbox_index;
+	dev = &acpi_dev->dev;
+
+	snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
+	ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
+			    mctp_pcc_setup);
+	if (!ndev)
+		return -ENOMEM;
+	mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev);
+	INIT_LIST_HEAD(&mctp_pcc_dev->next);
+	spin_lock_init(&mctp_pcc_dev->lock);
+
+	mctp_pcc_dev->hw_addr.inbox_index = inbox_index;
+	mctp_pcc_dev->hw_addr.outbox_index = outbox_index;
+	mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback;
+	mctp_pcc_dev->out_chan =
+		pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client,
+					 outbox_index);
+	if (IS_ERR(mctp_pcc_dev->out_chan)) {
+		rc = PTR_ERR(mctp_pcc_dev->out_chan);
+		goto free_netdev;
+	}
+	mctp_pcc_dev->in_chan =
+		pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client,
+					 inbox_index);
+	if (IS_ERR(mctp_pcc_dev->in_chan)) {
+		rc = PTR_ERR(mctp_pcc_dev->in_chan);
+		goto cleanup_out_channel;
+	}
+	mctp_pcc_dev->pcc_comm_inbox_addr =
+		devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
+			     mctp_pcc_dev->in_chan->shmem_size);
+	if (!mctp_pcc_dev->pcc_comm_inbox_addr) {
+		rc = -EINVAL;
+		goto cleanup_in_channel;
+	}
+	mctp_pcc_dev->pcc_comm_outbox_addr =
+		devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
+			     mctp_pcc_dev->out_chan->shmem_size);
+	if (!mctp_pcc_dev->pcc_comm_outbox_addr) {
+		rc = -EINVAL;
+		goto cleanup_in_channel;
+	}
+	mctp_pcc_dev->acpi_device = acpi_dev;
+	mctp_pcc_dev->inbox_client.dev = dev;
+	mctp_pcc_dev->outbox_client.dev = dev;
+	mctp_pcc_dev->mdev.dev = ndev;
+	acpi_dev->driver_data = mctp_pcc_dev;
+
+	/* There is no clean way to pass the MTU
+	 * to the callback function used for registration,
+	 * so set the values ahead of time.
+	 */
+	mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size -
+		sizeof(struct mctp_pcc_hdr);
+	ndev->mtu = MCTP_MIN_MTU;
+	ndev->max_mtu = mctp_pcc_mtu;
+	ndev->min_mtu = MCTP_MIN_MTU;
+
+	rc = register_netdev(ndev);
+	if (rc)
+		goto cleanup_in_channel;
+	list_add_tail(&mctp_pcc_dev->next, &mctp_pcc_ndevs);
+	return 0;
+
+cleanup_in_channel:
+	pcc_mbox_free_channel(mctp_pcc_dev->in_chan);
+cleanup_out_channel:
+	pcc_mbox_free_channel(mctp_pcc_dev->out_chan);
+free_netdev:
+	unregister_netdev(ndev);
+	free_netdev(ndev);
+	return rc;
+}
+
+static void mctp_pcc_driver_remove(struct acpi_device *adev)
+{
+	struct list_head *ptr;
+	struct list_head *tmp;
+
+	list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
+		struct net_device *ndev;
+		struct mctp_pcc_ndev *mctp_pcc_dev;
+
+		mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
+		if (mctp_pcc_dev->acpi_device != adev)
+			continue;
+		pcc_mbox_free_channel(mctp_pcc_dev->out_chan);
+		pcc_mbox_free_channel(mctp_pcc_dev->in_chan);
+		ndev = mctp_pcc_dev->mdev.dev;
+		if (ndev)
+			mctp_unregister_netdev(ndev);
+		list_del(ptr);
+			break;
+	}
+}
+
+static const struct acpi_device_id mctp_pcc_device_ids[] = {
+	{ "DMT0001", 0},
+	{ "", 0},
+};
+
+static struct acpi_driver mctp_pcc_driver = {
+	.name = "mctp_pcc",
+	.class = "Unknown",
+	.ids = mctp_pcc_device_ids,
+	.ops = {
+		.add = mctp_pcc_driver_add,
+		.remove = mctp_pcc_driver_remove,
+	},
+	.owner = THIS_MODULE,
+};
+
+module_acpi_driver(mctp_pcc_driver);
+
+MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
+
+MODULE_DESCRIPTION("MCTP PCC device");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");
-- 
2.34.1

Re: [PATCH v3 1/3] mctp pcc: Check before sending MCTP PCC response ACK

[Sudeep Holla]

On Tue, Jun 25, 2024 at 02:53:31PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@amperecomputing.com>
> 
> Type 4 PCC channels have an option to send back a response
> to the platform when they are done processing the request.
> The flag to indicate whether or not to respond is inside
> the message body, and thus is not available to the pcc
> mailbox.  Since only one message can be processed at once per
> channel, the value of this flag is checked during message processing
> and passed back via the channels global structure.
> 
> Ideally, the mailbox callback function would return a value
> indicating whether the message requires an ACK, but that
> would be a change to the mailbox API.  That would involve
> some change to all (about 12) of the mailbox based drivers,
> and the majority of them would not need to know about the
> ACK call.
>

Next time when you post new series, I prefer to be cc-ed in all the patches.
So far I ignored v1 and v2 thinking it has landed in my mbox my mistake and
deleted them. But just checked the series on lore, sorry for that.

> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>  drivers/mailbox/pcc.c | 6 +++++-
>  include/acpi/pcc.h    | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 94885e411085..5cf792700d79 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  {
>  	struct pcc_chan_info *pchan;
>  	struct mbox_chan *chan = p;
> +	struct pcc_mbox_chan *pmchan;
>  	u64 val;
>  	int ret;
>  
> @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>  		return IRQ_NONE;
>  
> +	pmchan = &pchan->chan;
> +	pmchan->ack_rx = true;  //TODO default to False

Indeed, default must be false. You need to do this conditionally at runtime
otherwise I see no need for this patch as it doesn't change anything as it
stands. It needs to be fixed to get this change merged.

Also we should set any such flag once at the boot, IRQ handler is not
the right place for sure.

-- 
Regards,
Sudeep

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

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc5 next-20240625]
[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/20240626-052432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240625185333.23211-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport
config: parisc-allmodconfig (https://download.01.org/0day-ci/archive/20240627/202406270051.3C6XAHU8-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406270051.3C6XAHU8-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/202406270051.3C6XAHU8-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

     165 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:214:9: note: in expansion of macro 'dev_dbg'
     214 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:215:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
     215 |                 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:214:9: note: in expansion of macro 'dev_dbg'
     214 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:216:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
     216 |         dev_handle = acpi_device_handle(acpi_dev);
         |                      ^~~~~~~~~~~~~~~~~~
         |                      acpi_device_dep
   drivers/net/mctp/mctp-pcc.c:216:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     216 |         dev_handle = acpi_device_handle(acpi_dev);
         |                    ^
   In file included from include/linux/device.h:15,
                    from include/linux/acpi.h:14:
   drivers/net/mctp/mctp-pcc.c:220:34: error: invalid use of undefined type 'struct acpi_device'
     220 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                                  ^~
   include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   drivers/net/mctp/mctp-pcc.c:220:17: note: in expansion of macro 'dev_err'
     220 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                 ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:225:24: error: invalid use of undefined type 'struct acpi_device'
     225 |         dev = &acpi_dev->dev;
         |                        ^~
   drivers/net/mctp/mctp-pcc.c:271:17: error: invalid use of undefined type 'struct acpi_device'
     271 |         acpi_dev->driver_data = mctp_pcc_dev;
         |                 ^~
   drivers/net/mctp/mctp-pcc.c: At top level:
   drivers/net/mctp/mctp-pcc.c:326:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
     326 | static struct acpi_driver mctp_pcc_driver = {
         |               ^~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:327:10: error: 'struct acpi_driver' has no member named 'name'
     327 |         .name = "mctp_pcc",
         |          ^~~~
   drivers/net/mctp/mctp-pcc.c:327:17: warning: excess elements in struct initializer
     327 |         .name = "mctp_pcc",
         |                 ^~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:327:17: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:328:10: error: 'struct acpi_driver' has no member named 'class'
     328 |         .class = "Unknown",
         |          ^~~~~
   drivers/net/mctp/mctp-pcc.c:328:18: warning: excess elements in struct initializer
     328 |         .class = "Unknown",
         |                  ^~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:328:18: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:329:10: error: 'struct acpi_driver' has no member named 'ids'
     329 |         .ids = mctp_pcc_device_ids,
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:329:16: warning: excess elements in struct initializer
     329 |         .ids = mctp_pcc_device_ids,
         |                ^~~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:329:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:330:10: error: 'struct acpi_driver' has no member named 'ops'
     330 |         .ops = {
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:330:16: error: extra brace group at end of initializer
     330 |         .ops = {
         |                ^
   drivers/net/mctp/mctp-pcc.c:330:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:330:16: warning: excess elements in struct initializer
   drivers/net/mctp/mctp-pcc.c:330:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:334:10: error: 'struct acpi_driver' has no member named 'owner'
     334 |         .owner = THIS_MODULE,
         |          ^~~~~
   In file included from arch/parisc/include/asm/alternative.h:18,
                    from arch/parisc/include/asm/barrier.h:5,
                    from include/linux/list.h:11,
                    from include/linux/resource_ext.h:9:
   include/linux/init.h:180:21: warning: excess elements in struct initializer
     180 | #define THIS_MODULE (&__this_module)
         |                     ^
   drivers/net/mctp/mctp-pcc.c:334:18: note: in expansion of macro 'THIS_MODULE'
     334 |         .owner = THIS_MODULE,
         |                  ^~~~~~~~~~~
   include/linux/init.h:180:21: note: (near initialization for 'mctp_pcc_driver')
     180 | #define THIS_MODULE (&__this_module)
         |                     ^
   drivers/net/mctp/mctp-pcc.c:334:18: note: in expansion of macro 'THIS_MODULE'
     334 |         .owner = THIS_MODULE,
         |                  ^~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:337:1: warning: data definition has no type or storage class
     337 | module_acpi_driver(mctp_pcc_driver);
         | ^~~~~~~~~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:337:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Werror=implicit-int]
>> drivers/net/mctp/mctp-pcc.c:337:1: warning: parameter names (without types) in function declaration
   drivers/net/mctp/mctp-pcc.c:326:27: error: storage size of 'mctp_pcc_driver' isn't known
     326 | static struct acpi_driver mctp_pcc_driver = {
         |                           ^~~~~~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:326:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]
   cc1: some warnings being treated as errors


vim +337 drivers/net/mctp/mctp-pcc.c

   325	
 > 326	static struct acpi_driver mctp_pcc_driver = {
   327		.name = "mctp_pcc",
   328		.class = "Unknown",
   329		.ids = mctp_pcc_device_ids,
   330		.ops = {
   331			.add = mctp_pcc_driver_add,
   332			.remove = mctp_pcc_driver_remove,
   333		},
   334		.owner = THIS_MODULE,
   335	};
   336	
 > 337	module_acpi_driver(mctp_pcc_driver);
   338	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/3] mctp pcc: Check before sending MCTP PCC response ACK

[Adam Young]

On 6/26/24 08:27, Sudeep Holla wrote:
> On Tue, Jun 25, 2024 at 02:53:31PM -0400, admiyo@os.amperecomputing.com wrote:
>> From: Adam Young <admiyo@amperecomputing.com>
>>
>> Type 4 PCC channels have an option to send back a response
>> to the platform when they are done processing the request.
>> The flag to indicate whether or not to respond is inside
>> the message body, and thus is not available to the pcc
>> mailbox.  Since only one message can be processed at once per
>> channel, the value of this flag is checked during message processing
>> and passed back via the channels global structure.
>>
>> Ideally, the mailbox callback function would return a value
>> indicating whether the message requires an ACK, but that
>> would be a change to the mailbox API.  That would involve
>> some change to all (about 12) of the mailbox based drivers,
>> and the majority of them would not need to know about the
>> ACK call.
>>
> Next time when you post new series, I prefer to be cc-ed in all the patches.

I was using the list of maintainers for each patch as pulled via the 
Kernel scripts in git send-email for the first two versions.

I have started hard coding the list of CCers as a superset of all the 
maintainers, as this patch series crosses a few boundaries.


> So far I ignored v1 and v2 thinking it has landed in my mbox my mistake and
> deleted them. But just checked the series on lore, sorry for that.
>
>> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>> ---
>>   drivers/mailbox/pcc.c | 6 +++++-
>>   include/acpi/pcc.h    | 1 +
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 94885e411085..5cf792700d79 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   {
>>   	struct pcc_chan_info *pchan;
>>   	struct mbox_chan *chan = p;
>> +	struct pcc_mbox_chan *pmchan;
>>   	u64 val;
>>   	int ret;
>>   
>> @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>>   		return IRQ_NONE;
>>   
>> +	pmchan = &pchan->chan;
>> +	pmchan->ack_rx = true;  //TODO default to False
> Indeed, default must be false. You need to do this conditionally at runtime
> otherwise I see no need for this patch as it doesn't change anything as it
> stands. It needs to be fixed to get this change merged.
>
> Also we should set any such flag once at the boot, IRQ handler is not
> the right place for sure.

Ringing the doorbell to signify that the message is received is optional 
in the ACPI/PCC spec but was hard coded to always get set.

There is currently no way to pass the value from the rx function to here 
where the code is actually triggering the response.  The Mailbox receive 
callback returns void.  Changing that would require changing every 
module that uses the mailbox code.

You can see the spec here:

https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#platform-notification-for-slave-pcc-subspaces-type-4

note in step 2 of the  send process, the part that says "The platform 
can request the OSPM rings the doorbell once it has completed processing 
the notification command by setting the Generate Signal bit in the flags."

The change that made the response mandatory was 60c40b06fa686 and merged 
in the 6.9 timeframe.

The actual check is done at run time, and you can see how it is done in 
the third patch, at the end of the  function mctp_pcc_client_rx_callback

+       flags = mctp_pcc_hdr.flags;
+       mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0;

While we don't anticipate our backend requiring the ACK, we did encode 
the possibility into the driver.

I am willing to rework this if we have a viable alternative.










>

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

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc5 next-20240625]
[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/20240626-052432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240625185333.23211-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20240627/202406270625.2MuBj55z-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project ad79a14c9e5ec4a369eed4adf567c22cc029863f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406270625.2MuBj55z-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/202406270625.2MuBj55z-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/net/mctp/mctp-pcc.c:8:
   In file included from include/linux/if_arp.h:22:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/powerpc/include/asm/cacheflush.h:7:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     500 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     501 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     507 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     508 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     519 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     520 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     528 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     529 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/net/mctp/mctp-pcc.c:17:
   include/acpi/acpi_drivers.h:72:43: warning: declaration of 'struct acpi_pci_root' will not be visible outside of this function [-Wvisibility]
      72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
         |                                           ^
   drivers/net/mctp/mctp-pcc.c:214:19: error: incomplete definition of type 'struct acpi_device'
     214 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |                  ~~~~~~~~^
   include/linux/dev_printk.h:165:18: note: expanded from macro 'dev_dbg'
     165 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                         ^~~
   include/linux/dynamic_debug.h:274:7: note: expanded from macro 'dynamic_dev_dbg'
     274 |                            dev, fmt, ##__VA_ARGS__)
         |                            ^~~
   include/linux/dynamic_debug.h:250:59: note: expanded from macro '_dynamic_func_call'
     250 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
         |                                                                  ^~~~~~~~~~~
   include/linux/dynamic_debug.h:248:65: note: expanded from macro '_dynamic_func_call_cls'
     248 |         __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
         |                                                                        ^~~~~~~~~~~
   include/linux/dynamic_debug.h:224:15: note: expanded from macro '__dynamic_func_call_cls'
     224 |                 func(&id, ##__VA_ARGS__);                       \
         |                             ^~~~~~~~~~~
   include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
     792 | struct acpi_device;
         |        ^
   drivers/net/mctp/mctp-pcc.c:215:3: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     215 |                 acpi_device_hid(acpi_dev));
         |                 ^
   drivers/net/mctp/mctp-pcc.c:215: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:216:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     216 |         dev_handle = acpi_device_handle(acpi_dev);
         |                      ^
   drivers/net/mctp/mctp-pcc.c:216:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion]
     216 |         dev_handle = acpi_device_handle(acpi_dev);
         |                    ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:220:20: error: incomplete definition of type 'struct acpi_device'
     220 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                          ~~~~~~~~^
   include/linux/dev_printk.h:154:44: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                   ^~~
   include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
     792 | struct acpi_device;
         |        ^
   drivers/net/mctp/mctp-pcc.c:225:17: error: incomplete definition of type 'struct acpi_device'
     225 |         dev = &acpi_dev->dev;
         |                ~~~~~~~~^
   include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
     792 | struct acpi_device;
         |        ^
   drivers/net/mctp/mctp-pcc.c:271:10: error: incomplete definition of type 'struct acpi_device'
     271 |         acpi_dev->driver_data = mctp_pcc_dev;
         |         ~~~~~~~~^
   include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
     792 | struct acpi_device;
         |        ^
   drivers/net/mctp/mctp-pcc.c:326:27: error: variable has incomplete type 'struct acpi_driver'
     326 | static struct acpi_driver mctp_pcc_driver = {
         |                           ^
   drivers/net/mctp/mctp-pcc.c:326:15: note: forward declaration of 'struct acpi_driver'
     326 | static struct acpi_driver mctp_pcc_driver = {
         |               ^
>> drivers/net/mctp/mctp-pcc.c:337:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
     337 | module_acpi_driver(mctp_pcc_driver);
         | ^
         | int
>> drivers/net/mctp/mctp-pcc.c:337:20: error: a parameter list without types is only allowed in a function definition
     337 | module_acpi_driver(mctp_pcc_driver);
         |                    ^
   6 warnings and 10 errors generated.


vim +/int +337 drivers/net/mctp/mctp-pcc.c

   325	
 > 326	static struct acpi_driver mctp_pcc_driver = {
   327		.name = "mctp_pcc",
   328		.class = "Unknown",
   329		.ids = mctp_pcc_device_ids,
   330		.ops = {
   331			.add = mctp_pcc_driver_add,
   332			.remove = mctp_pcc_driver_remove,
   333		},
   334		.owner = THIS_MODULE,
   335	};
   336	
 > 337	module_acpi_driver(mctp_pcc_driver);
   338	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Jeremy Kerr]

Hi Adam,

Looking good, just a couple of minor things inline, and a query on the
new _remove() implementation.

> +#define SPDM_VERSION_OFFSET    1
> +#define SPDM_REQ_RESP_OFFSET   2

These seem unrelated, and are unused?

> +#define MCTP_PAYLOAD_LENGTH    256
> +#define MCTP_CMD_LENGTH                4
> +#define MCTP_PCC_VERSION       0x1 /* DSP0253 defines a single version: 1 */
> +#define MCTP_SIGNATURE         "MCTP"
> +#define SIGNATURE_LENGTH       4
> +#define MCTP_HEADER_LENGTH     12
> +#define MCTP_MIN_MTU           68
> +#define PCC_MAGIC              0x50434300
> +#define PCC_HEADER_FLAG_REQ_INT        0x1
> +#define PCC_HEADER_FLAGS       PCC_HEADER_FLAG_REQ_INT
> +#define PCC_DWORD_TYPE         0x0c
> +#define PCC_ACK_FLAG_MASK      0x1
> +
> +struct mctp_pcc_hdr {
> +       u32 signature;
> +       u32 flags;
> +       u32 length;
> +       char mctp_signature[4];
> +};

I'd recommend the endian-annotated types for signature, flags & length
here, and converting on access.

(yes, maybe this will only ever be intended for little-endian, but
there's almost always cases where code gets moved around, copied into
new drivers, etc)

> +struct mctp_pcc_hw_addr {
> +       u32 inbox_index;
> +       u32 outbox_index;
> +};
> +
> +/* The netdev structure. One of these per PCC adapter. */
> +struct mctp_pcc_ndev {
> +       struct list_head next;
> +       /* spinlock to serialize access to pcc buffer and registers*/
> +       spinlock_t lock;
> +       struct mctp_dev mdev;
> +       struct acpi_device *acpi_device;
> +       struct pcc_mbox_chan *in_chan;
> +       struct pcc_mbox_chan *out_chan;
> +       struct mbox_client outbox_client;
> +       struct mbox_client inbox_client;
> +       void __iomem *pcc_comm_inbox_addr;
> +       void __iomem *pcc_comm_outbox_addr;
> +       struct mctp_pcc_hw_addr hw_addr;
> +};
> +
> +static struct list_head mctp_pcc_ndevs = LIST_HEAD_INIT(mctp_pcc_ndevs);

Saw your changelog entry about needing this, makes sense if that's what
the acpi core requires.

In which case, you can make this a little more brief with:

static LIST_HEAD(mctp_pcc_ndevs);

(but more on that below)

> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_dev;
> +       struct mctp_pcc_hdr mctp_pcc_hdr;
> +       struct mctp_skb_cb *cb;
> +       struct sk_buff *skb;
> +       void *skb_buf;
> +       u32 data_len;
> +       u32 flags;
> +
> +       mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> +       memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr,
> +                     sizeof(struct mctp_pcc_hdr));
> +       data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
> +
> +       if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) {
> +               mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> +               return;
> +       }
> +
> +       skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len);
> +       if (!skb) {
> +               mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> +               return;
> +       }
> +       mctp_pcc_dev->mdev.dev->stats.rx_packets++;
> +       mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len;
> +       skb->protocol = htons(ETH_P_MCTP);
> +       skb_buf = skb_put(skb, data_len);
> +       memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len);
> +       skb_reset_mac_header(skb);
> +       skb_pull(skb, sizeof(struct mctp_pcc_hdr));
> +       skb_reset_network_header(skb);
> +       cb = __mctp_cb(skb);
> +       cb->halen = 0;
> +       netif_rx(skb);
> +
> +       flags = mctp_pcc_hdr.flags;
> +       mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0;

The '>' comparison to a bit field is a bit odd, you could just:

       mctp_pcc_dev->in_chan->ack_rx = !!(flags & PCC_ACK_FLAG_MASK);

> +static void
> +mctp_pcc_net_stats(struct net_device *net_dev,
> +                  struct rtnl_link_stats64 *stats)
> +{
> +       struct mctp_pcc_ndev *mpnd;
> +
> +       mpnd = (struct mctp_pcc_ndev *)netdev_priv(net_dev);

No need to cast from void *.

> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> +       struct lookup_context context = {0, 0, 0};
> +       struct mctp_pcc_ndev *mctp_pcc_dev;
> +       struct net_device *ndev;
> +       acpi_handle dev_handle;
> +       acpi_status status;
> +       struct device *dev;
> +       int mctp_pcc_mtu;
> +       int outbox_index;
> +       int inbox_index;
> +       char name[32];
> +       int rc;
> +
> +       dev_dbg(&acpi_dev->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(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
> +               return -EINVAL;
> +       }
> +       inbox_index = context.inbox_index;
> +       outbox_index = context.outbox_index;
> +       dev = &acpi_dev->dev;
> +
> +       snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
> +       ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
> +                           mctp_pcc_setup);
> +       if (!ndev)
> +               return -ENOMEM;
> +       mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev);

No need for the cast from void *.

> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> +       struct list_head *ptr;
> +       struct list_head *tmp;
> +
> +       list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {

You can save the list_entry() below by using list_for_each_entry_safe()
here.

> +               struct net_device *ndev;
> +               struct mctp_pcc_ndev *mctp_pcc_dev;
> +
> +               mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> +               if (mctp_pcc_dev->acpi_device != adev)
> +                       continue;

Wait, you removed the case where you clean up the whole list if adev is
NULL?

Now this contradicts your section of the changelog:

- Did not change the module init unload function to use the
  ACPI equivalent as they do not remove all devices prior
  to unload, leading to dangling references and seg faults.

Does this mean you no longer need to free all instances on unload? In
which case, that sounds great! that probably also means:

 - all the list is doing is allowing you to find the mctp_pcc_dev from
   the acpi_device

 - but: you can turn an acpi_device into a mctp_pcc_dev from
   adev->driver_data (which you're already setting), so you don't need
   the list

 - then, _remove just becomes something like:

    static void mctp_pcc_driver_remove(struct acpi_device *adev)
    {
            struct mctp_pcc_dev *dev = acpi_driver_data(adev);

            pcc_mbox_free_channel(dev->out_chan);
            pcc_mbox_free_channel(dev->in_chan);
            if (dev->mdev.dev)
                    mctp_unregister_netdev(dev->mdev.dev);
    }

    (but I can't see how dev->mdev.dev can be null, so you may be able
    to remove the condition too)

> +               pcc_mbox_free_channel(mctp_pcc_dev->out_chan);
> +               pcc_mbox_free_channel(mctp_pcc_dev->in_chan);
> +               ndev = mctp_pcc_dev->mdev.dev;
> +               if (ndev)
> +                       mctp_unregister_netdev(ndev);
> +               list_del(ptr);
> +                       break;

Unusual indent here.

Cheers,


Jeremy

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

[Adam Young]

Yep, you are right, and as I continue to mull this over, I realize I can 
make it much simpler.

Sorry about missing the changelog entry, as I had written that prior to 
realizing that all of this cleanup was a workaround for the error you 
found in the first iteration:  flipping the null meant I was leaving 
behind devices when I should have been cleaning them up.  So, yeah, I 
think the list can go now.



On 6/28/24 04:41, Jeremy Kerr wrote:
> You can save the list_entry() below by using list_for_each_entry_safe()
> here.
>
>> +               struct net_device *ndev;
>> +               struct mctp_pcc_ndev *mctp_pcc_dev;
>> +
>> +               mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
>> +               if (mctp_pcc_dev->acpi_device != adev)
>> +                       continue;
> Wait, you removed the case where you clean up the whole list if adev is
> NULL?
>
> Now this contradicts your section of the changelog:
>
> - Did not change the module init unload function to use the
>    ACPI equivalent as they do not remove all devices prior
>    to unload, leading to dangling references and seg faults.
>
> Does this mean you no longer need to free all instances on unload? In
> which case, that sounds great! that probably also means:
>
>   - all the list is doing is allowing you to find the mctp_pcc_dev from
>     the acpi_device
>
>   - but: you can turn an acpi_device into a mctp_pcc_dev from
>     adev->driver_data (which you're already setting), so you don't need
>     the list
>
>   - then, _remove just becomes something like:
>
>      static void mctp_pcc_driver_remove(struct acpi_device *adev)
>      {
>              struct mctp_pcc_dev *dev = acpi_driver_data(adev);
>
>              pcc_mbox_free_channel(dev->out_chan);
>              pcc_mbox_free_channel(dev->in_chan);
>              if (dev->mdev.dev)
>                      mctp_unregister_netdev(dev->mdev.dev);
>      }
>
>      (but I can't see how dev->mdev.dev can be null, so you may be able
>      to remove the condition too)

Re: [PATCH v3 1/3] mctp pcc: Check before sending MCTP PCC response ACK

[Adam Young]

  Huisong Li you wrote the original patch that I am working around.

What would I break if I disabled the IRQ ACK?  It should not be the 
default behavior as it is an optional feature.

There needs to be a mechanism for the driver to trigger the ACK, but it 
needs to be based on the content of the message buffer.

I was thinking it should be in the return code  of the callback, but 
that breaks all of the other mailbox implementations.

I suspect a better approach would be to provide a function pointer to 
the  driver and let the driver decide whether or not to trigger the ACK.



On 6/26/24 08:27, Sudeep Holla wrote:
> On Tue, Jun 25, 2024 at 02:53:31PM -0400, admiyo@os.amperecomputing.com wrote:
>> From: Adam Young <admiyo@amperecomputing.com>
>>
>> Type 4 PCC channels have an option to send back a response
>> to the platform when they are done processing the request.
>> The flag to indicate whether or not to respond is inside
>> the message body, and thus is not available to the pcc
>> mailbox.  Since only one message can be processed at once per
>> channel, the value of this flag is checked during message processing
>> and passed back via the channels global structure.
>>
>> Ideally, the mailbox callback function would return a value
>> indicating whether the message requires an ACK, but that
>> would be a change to the mailbox API.  That would involve
>> some change to all (about 12) of the mailbox based drivers,
>> and the majority of them would not need to know about the
>> ACK call.
>>
> Next time when you post new series, I prefer to be cc-ed in all the patches.
> So far I ignored v1 and v2 thinking it has landed in my mbox my mistake and
> deleted them. But just checked the series on lore, sorry for that.
>
>> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>> ---
>>   drivers/mailbox/pcc.c | 6 +++++-
>>   include/acpi/pcc.h    | 1 +
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 94885e411085..5cf792700d79 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   {
>>   	struct pcc_chan_info *pchan;
>>   	struct mbox_chan *chan = p;
>> +	struct pcc_mbox_chan *pmchan;
>>   	u64 val;
>>   	int ret;
>>   
>> @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>>   		return IRQ_NONE;
>>   
>> +	pmchan = &pchan->chan;
>> +	pmchan->ack_rx = true;  //TODO default to False
> Indeed, default must be false. You need to do this conditionally at runtime
> otherwise I see no need for this patch as it doesn't change anything as it
> stands. It needs to be fixed to get this change merged.
>
> Also we should set any such flag once at the boot, IRQ handler is not
> the right place for sure.
>