[PATCH v5 0/3] MCTP over PCC

[PATCH v5 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 V5
- Removed Owener 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.

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 |   5 +-
 drivers/mailbox/pcc.c        |  32 +++-
 drivers/net/mctp/Kconfig     |  13 ++
 drivers/net/mctp/Makefile    |   1 +
 drivers/net/mctp/mctp-pcc.c  | 325 +++++++++++++++++++++++++++++++++++
 include/acpi/pcc.h           |   8 +
 6 files changed, 374 insertions(+), 10 deletions(-)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

-- 
2.34.1

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

[admiyo]

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.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 94885e411085..4a588f1b6ec2 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,24 @@ 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
+	 * and ring doorbell after processing message.
+	 *
+	 * 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);
+}
+
 /**
  * pcc_mbox_irq - PCC mailbox interrupt handler
  * @irq:	interrupt number
@@ -306,14 +326,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 +365,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

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

[admiyo]

From: Adam Young <admiyo@os.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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c
index fff48001d7ef..9f8cfdc51637 100644
--- a/drivers/acpi/acpica/rsaddr.c
+++ b/drivers/acpi/acpica/rsaddr.c
@@ -282,9 +282,10 @@ 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);
-	}
 
 	/* Get the Resource Type and General Flags */
 
-- 
2.34.1

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

[admiyo]

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

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

diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index ce9d2d2ccf3b..9958b162af65 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..055d6408e1d7
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,325 @@
+// 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 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 {
+	/* 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 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 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->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->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);
+}
+
+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)
+{
+	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->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 = netdev_priv(ndev);
+	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;
+	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 mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
+
+	pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
+	pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
+	mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
+}
+
+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,
+	},
+};
+
+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 v5 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-rc7 next-20240712]
[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/20240712-104202
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240712023626.1010559-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v5 3/3] mctp pcc: Implement MCTP over PCC Transport
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240713/202407131538.hqt58AQS-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240713/202407131538.hqt58AQS-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/202407131538.hqt58AQS-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/net/mctp/mctp-pcc.c:17:
   include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration
      72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
         |                                           ^~~~~~~~~~~~~
   In file included from include/linux/printk.h:570,
                    from include/asm-generic/bug.h:22,
                    from arch/sh/include/asm/bug.h:112,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/sh/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:7:
   drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add':
   drivers/net/mctp/mctp-pcc.c:212:26: error: invalid use of undefined type 'struct acpi_device'
     212 |         dev_dbg(&acpi_dev->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__)
         |         ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:212:9: note: in expansion of macro 'dev_dbg'
     212 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:213:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Wimplicit-function-declaration]
     213 |                 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:212:9: note: in expansion of macro 'dev_dbg'
     212 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:214:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Wimplicit-function-declaration]
     214 |         dev_handle = acpi_device_handle(acpi_dev);
         |                      ^~~~~~~~~~~~~~~~~~
         |                      acpi_device_dep
>> drivers/net/mctp/mctp-pcc.c:214:20: error: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     214 |         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:218:34: error: invalid use of undefined type 'struct acpi_device'
     218 |                 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:218:17: note: in expansion of macro 'dev_err'
     218 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                 ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:223:24: error: invalid use of undefined type 'struct acpi_device'
     223 |         dev = &acpi_dev->dev;
         |                        ^~
   drivers/net/mctp/mctp-pcc.c:268:17: error: invalid use of undefined type 'struct acpi_device'
     268 |         acpi_dev->driver_data = mctp_pcc_dev;
         |                 ^~
   drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_remove':
   drivers/net/mctp/mctp-pcc.c:297:47: error: implicit declaration of function 'acpi_driver_data'; did you mean 'acpi_get_data'? [-Wimplicit-function-declaration]
     297 |         struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
         |                                               ^~~~~~~~~~~~~~~~
         |                                               acpi_get_data
>> drivers/net/mctp/mctp-pcc.c:297:47: error: initialization of 'struct mctp_pcc_ndev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
   drivers/net/mctp/mctp-pcc.c: At top level:
   drivers/net/mctp/mctp-pcc.c:309:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
     309 | static struct acpi_driver mctp_pcc_driver = {
         |               ^~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:310:10: error: 'struct acpi_driver' has no member named 'name'
     310 |         .name = "mctp_pcc",
         |          ^~~~
   drivers/net/mctp/mctp-pcc.c:310:17: warning: excess elements in struct initializer
     310 |         .name = "mctp_pcc",
         |                 ^~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:310:17: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:311:10: error: 'struct acpi_driver' has no member named 'class'
     311 |         .class = "Unknown",
         |          ^~~~~
   drivers/net/mctp/mctp-pcc.c:311:18: warning: excess elements in struct initializer
     311 |         .class = "Unknown",
         |                  ^~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:311:18: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:312:10: error: 'struct acpi_driver' has no member named 'ids'
     312 |         .ids = mctp_pcc_device_ids,
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:312:16: warning: excess elements in struct initializer
     312 |         .ids = mctp_pcc_device_ids,
         |                ^~~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:312:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:313:10: error: 'struct acpi_driver' has no member named 'ops'
     313 |         .ops = {
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:313:16: error: extra brace group at end of initializer
     313 |         .ops = {
         |                ^
   drivers/net/mctp/mctp-pcc.c:313:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:313:16: warning: excess elements in struct initializer
   drivers/net/mctp/mctp-pcc.c:313:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:319:1: warning: data definition has no type or storage class
     319 | module_acpi_driver(mctp_pcc_driver);
         | ^~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:319:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Wimplicit-int]
>> drivers/net/mctp/mctp-pcc.c:319:1: error: parameter names (without types) in function declaration [-Wdeclaration-missing-parameter-type]
   drivers/net/mctp/mctp-pcc.c:309:27: error: storage size of 'mctp_pcc_driver' isn't known
     309 | static struct acpi_driver mctp_pcc_driver = {
         |                           ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:309:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]


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

   197	
   198	static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
   199	{
   200		struct lookup_context context = {0, 0, 0};
   201		struct mctp_pcc_ndev *mctp_pcc_dev;
   202		struct net_device *ndev;
   203		acpi_handle dev_handle;
   204		acpi_status status;
   205		struct device *dev;
   206		int mctp_pcc_mtu;
   207		int outbox_index;
   208		int inbox_index;
   209		char name[32];
   210		int rc;
   211	
   212		dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
   213			acpi_device_hid(acpi_dev));
 > 214		dev_handle = acpi_device_handle(acpi_dev);
   215		status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
   216					     &context);
   217		if (!ACPI_SUCCESS(status)) {
   218			dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
   219			return -EINVAL;
   220		}
   221		inbox_index = context.inbox_index;
   222		outbox_index = context.outbox_index;
   223		dev = &acpi_dev->dev;
   224	
   225		snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
   226		ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
   227				    mctp_pcc_setup);
   228		if (!ndev)
   229			return -ENOMEM;
   230		mctp_pcc_dev = netdev_priv(ndev);
   231		spin_lock_init(&mctp_pcc_dev->lock);
   232	
   233		mctp_pcc_dev->hw_addr.inbox_index = inbox_index;
   234		mctp_pcc_dev->hw_addr.outbox_index = outbox_index;
   235		mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback;
   236		mctp_pcc_dev->out_chan =
   237			pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client,
   238						 outbox_index);
   239		if (IS_ERR(mctp_pcc_dev->out_chan)) {
   240			rc = PTR_ERR(mctp_pcc_dev->out_chan);
   241			goto free_netdev;
   242		}
   243		mctp_pcc_dev->in_chan =
   244			pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client,
   245						 inbox_index);
   246		if (IS_ERR(mctp_pcc_dev->in_chan)) {
   247			rc = PTR_ERR(mctp_pcc_dev->in_chan);
   248			goto cleanup_out_channel;
   249		}
   250		mctp_pcc_dev->pcc_comm_inbox_addr =
   251			devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
   252				     mctp_pcc_dev->in_chan->shmem_size);
   253		if (!mctp_pcc_dev->pcc_comm_inbox_addr) {
   254			rc = -EINVAL;
   255			goto cleanup_in_channel;
   256		}
   257		mctp_pcc_dev->pcc_comm_outbox_addr =
   258			devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
   259				     mctp_pcc_dev->out_chan->shmem_size);
   260		if (!mctp_pcc_dev->pcc_comm_outbox_addr) {
   261			rc = -EINVAL;
   262			goto cleanup_in_channel;
   263		}
   264		mctp_pcc_dev->acpi_device = acpi_dev;
   265		mctp_pcc_dev->inbox_client.dev = dev;
   266		mctp_pcc_dev->outbox_client.dev = dev;
   267		mctp_pcc_dev->mdev.dev = ndev;
   268		acpi_dev->driver_data = mctp_pcc_dev;
   269	
   270		/* There is no clean way to pass the MTU
   271		 * to the callback function used for registration,
   272		 * so set the values ahead of time.
   273		 */
   274		mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size -
   275			sizeof(struct mctp_pcc_hdr);
   276		ndev->mtu = MCTP_MIN_MTU;
   277		ndev->max_mtu = mctp_pcc_mtu;
   278		ndev->min_mtu = MCTP_MIN_MTU;
   279	
   280		rc = register_netdev(ndev);
   281		if (rc)
   282			goto cleanup_in_channel;
   283		return 0;
   284	
   285	cleanup_in_channel:
   286		pcc_mbox_free_channel(mctp_pcc_dev->in_chan);
   287	cleanup_out_channel:
   288		pcc_mbox_free_channel(mctp_pcc_dev->out_chan);
   289	free_netdev:
   290		unregister_netdev(ndev);
   291		free_netdev(ndev);
   292		return rc;
   293	}
   294	
   295	static void mctp_pcc_driver_remove(struct acpi_device *adev)
   296	{
 > 297		struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
   298	
   299		pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
   300		pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
   301		mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
   302	}
   303	
   304	static const struct acpi_device_id mctp_pcc_device_ids[] = {
   305		{ "DMT0001", 0},
   306		{ "", 0},
   307	};
   308	
   309	static struct acpi_driver mctp_pcc_driver = {
   310		.name = "mctp_pcc",
   311		.class = "Unknown",
   312		.ids = mctp_pcc_device_ids,
   313		.ops = {
   314			.add = mctp_pcc_driver_add,
   315			.remove = mctp_pcc_driver_remove,
   316		},
   317	};
   318	
 > 319	module_acpi_driver(mctp_pcc_driver);
   320	

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

Re: [PATCH v5 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-rc7 next-20240712]
[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/20240712-104202
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240712023626.1010559-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v5 3/3] mctp pcc: Implement MCTP over PCC Transport
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240713/202407131507.yys3gYAw-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240713/202407131507.yys3gYAw-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/202407131507.yys3gYAw-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/net/mctp/mctp-pcc.c:17:
   include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration
      72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
         |                                           ^~~~~~~~~~~~~
   In file included from include/linux/printk.h:570,
                    from include/asm-generic/bug.h:22,
                    from arch/s390/include/asm/bug.h:69,
                    from include/linux/bug.h:5,
                    from arch/s390/include/asm/ctlreg.h:85,
                    from arch/s390/include/asm/lowcore.h:14,
                    from arch/s390/include/asm/current.h:13,
                    from arch/s390/include/asm/preempt.h:5,
                    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:7:
   drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add':
   drivers/net/mctp/mctp-pcc.c:212:26: error: invalid use of undefined type 'struct acpi_device'
     212 |         dev_dbg(&acpi_dev->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__)
         |         ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:212:9: note: in expansion of macro 'dev_dbg'
     212 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:213:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Wimplicit-function-declaration]
     213 |                 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:212:9: note: in expansion of macro 'dev_dbg'
     212 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:214:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Wimplicit-function-declaration]
     214 |         dev_handle = acpi_device_handle(acpi_dev);
         |                      ^~~~~~~~~~~~~~~~~~
         |                      acpi_device_dep
   drivers/net/mctp/mctp-pcc.c:214:20: error: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     214 |         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:218:34: error: invalid use of undefined type 'struct acpi_device'
     218 |                 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:218:17: note: in expansion of macro 'dev_err'
     218 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                 ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:223:24: error: invalid use of undefined type 'struct acpi_device'
     223 |         dev = &acpi_dev->dev;
         |                        ^~
   drivers/net/mctp/mctp-pcc.c:251:17: error: implicit declaration of function 'devm_ioremap' [-Wimplicit-function-declaration]
     251 |                 devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
         |                 ^~~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:250:43: error: assignment to 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     250 |         mctp_pcc_dev->pcc_comm_inbox_addr =
         |                                           ^
   drivers/net/mctp/mctp-pcc.c:257:44: error: assignment to 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     257 |         mctp_pcc_dev->pcc_comm_outbox_addr =
         |                                            ^
   drivers/net/mctp/mctp-pcc.c:268:17: error: invalid use of undefined type 'struct acpi_device'
     268 |         acpi_dev->driver_data = mctp_pcc_dev;
         |                 ^~
   drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_remove':
   drivers/net/mctp/mctp-pcc.c:297:47: error: implicit declaration of function 'acpi_driver_data'; did you mean 'acpi_get_data'? [-Wimplicit-function-declaration]
     297 |         struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
         |                                               ^~~~~~~~~~~~~~~~
         |                                               acpi_get_data
   drivers/net/mctp/mctp-pcc.c:297:47: error: initialization of 'struct mctp_pcc_ndev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
   drivers/net/mctp/mctp-pcc.c: At top level:
   drivers/net/mctp/mctp-pcc.c:309:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
     309 | static struct acpi_driver mctp_pcc_driver = {
         |               ^~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:310:10: error: 'struct acpi_driver' has no member named 'name'
     310 |         .name = "mctp_pcc",
         |          ^~~~
   drivers/net/mctp/mctp-pcc.c:310:17: warning: excess elements in struct initializer
     310 |         .name = "mctp_pcc",
         |                 ^~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:310:17: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:311:10: error: 'struct acpi_driver' has no member named 'class'
     311 |         .class = "Unknown",
         |          ^~~~~
   drivers/net/mctp/mctp-pcc.c:311:18: warning: excess elements in struct initializer
     311 |         .class = "Unknown",
         |                  ^~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:311:18: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:312:10: error: 'struct acpi_driver' has no member named 'ids'
     312 |         .ids = mctp_pcc_device_ids,
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:312:16: warning: excess elements in struct initializer
     312 |         .ids = mctp_pcc_device_ids,
         |                ^~~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:312:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:313:10: error: 'struct acpi_driver' has no member named 'ops'
     313 |         .ops = {
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:313:16: error: extra brace group at end of initializer
     313 |         .ops = {
         |                ^
   drivers/net/mctp/mctp-pcc.c:313:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:313:16: warning: excess elements in struct initializer
   drivers/net/mctp/mctp-pcc.c:313:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:319:1: warning: data definition has no type or storage class
     319 | module_acpi_driver(mctp_pcc_driver);
         | ^~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:319:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Wimplicit-int]
   drivers/net/mctp/mctp-pcc.c:319:1: error: parameter names (without types) in function declaration [-Wdeclaration-missing-parameter-type]
   drivers/net/mctp/mctp-pcc.c:309:27: error: storage size of 'mctp_pcc_driver' isn't known
     309 | static struct acpi_driver mctp_pcc_driver = {
         |                           ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:309:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]


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

   197	
   198	static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
   199	{
   200		struct lookup_context context = {0, 0, 0};
   201		struct mctp_pcc_ndev *mctp_pcc_dev;
   202		struct net_device *ndev;
   203		acpi_handle dev_handle;
   204		acpi_status status;
   205		struct device *dev;
   206		int mctp_pcc_mtu;
   207		int outbox_index;
   208		int inbox_index;
   209		char name[32];
   210		int rc;
   211	
   212		dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
   213			acpi_device_hid(acpi_dev));
   214		dev_handle = acpi_device_handle(acpi_dev);
   215		status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
   216					     &context);
   217		if (!ACPI_SUCCESS(status)) {
   218			dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
   219			return -EINVAL;
   220		}
   221		inbox_index = context.inbox_index;
   222		outbox_index = context.outbox_index;
   223		dev = &acpi_dev->dev;
   224	
   225		snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
   226		ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
   227				    mctp_pcc_setup);
   228		if (!ndev)
   229			return -ENOMEM;
   230		mctp_pcc_dev = netdev_priv(ndev);
   231		spin_lock_init(&mctp_pcc_dev->lock);
   232	
   233		mctp_pcc_dev->hw_addr.inbox_index = inbox_index;
   234		mctp_pcc_dev->hw_addr.outbox_index = outbox_index;
   235		mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback;
   236		mctp_pcc_dev->out_chan =
   237			pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client,
   238						 outbox_index);
   239		if (IS_ERR(mctp_pcc_dev->out_chan)) {
   240			rc = PTR_ERR(mctp_pcc_dev->out_chan);
   241			goto free_netdev;
   242		}
   243		mctp_pcc_dev->in_chan =
   244			pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client,
   245						 inbox_index);
   246		if (IS_ERR(mctp_pcc_dev->in_chan)) {
   247			rc = PTR_ERR(mctp_pcc_dev->in_chan);
   248			goto cleanup_out_channel;
   249		}
 > 250		mctp_pcc_dev->pcc_comm_inbox_addr =
   251			devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
   252				     mctp_pcc_dev->in_chan->shmem_size);
   253		if (!mctp_pcc_dev->pcc_comm_inbox_addr) {
   254			rc = -EINVAL;
   255			goto cleanup_in_channel;
   256		}
   257		mctp_pcc_dev->pcc_comm_outbox_addr =
   258			devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
   259				     mctp_pcc_dev->out_chan->shmem_size);
   260		if (!mctp_pcc_dev->pcc_comm_outbox_addr) {
   261			rc = -EINVAL;
   262			goto cleanup_in_channel;
   263		}
   264		mctp_pcc_dev->acpi_device = acpi_dev;
   265		mctp_pcc_dev->inbox_client.dev = dev;
   266		mctp_pcc_dev->outbox_client.dev = dev;
   267		mctp_pcc_dev->mdev.dev = ndev;
   268		acpi_dev->driver_data = mctp_pcc_dev;
   269	
   270		/* There is no clean way to pass the MTU
   271		 * to the callback function used for registration,
   272		 * so set the values ahead of time.
   273		 */
   274		mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size -
   275			sizeof(struct mctp_pcc_hdr);
   276		ndev->mtu = MCTP_MIN_MTU;
   277		ndev->max_mtu = mctp_pcc_mtu;
   278		ndev->min_mtu = MCTP_MIN_MTU;
   279	
   280		rc = register_netdev(ndev);
   281		if (rc)
   282			goto cleanup_in_channel;
   283		return 0;
   284	
   285	cleanup_in_channel:
   286		pcc_mbox_free_channel(mctp_pcc_dev->in_chan);
   287	cleanup_out_channel:
   288		pcc_mbox_free_channel(mctp_pcc_dev->out_chan);
   289	free_netdev:
   290		unregister_netdev(ndev);
   291		free_netdev(ndev);
   292		return rc;
   293	}
   294	

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

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

[Jonathan Cameron]

On Thu, 11 Jul 2024 22:36:24 -0400
admiyo@os.amperecomputing.com wrote:

> 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.
Hi Adam,

I've been meaning to look at this for a while, but finally
getting time for review catchup.

Would be good to have an explicit specification reference to
make it easy for reviewers to find the bit to compare with.

> 
> In order to read the flag, this patch maps the shared
> buffer to virtual memory.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>  drivers/mailbox/pcc.c | 32 ++++++++++++++++++++++++--------
>  include/acpi/pcc.h    |  8 ++++++++
>  2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 94885e411085..4a588f1b6ec2 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

If you are only going to map this from this pointer for the
initiator/responder shared memory region, maybe it would benefit
from a more specific name?

>   * @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,24 @@ 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;

I'd avoid name pcc_hdr, as it's only the header on a paricular type
of pcc subspace.  Either go super generic with hdr or
pcc_rsp_subspc_hdr or something along those lines.

> +
> +	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		return;

I'd put a blank line here. Next bit is unrelated to the error check
so white space will help with readability (a little bit anyway!)

> +	memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
> +		      sizeof(struct pcc_extended_type_hdr));

sizeof(pcc_hdr)

> +	/*
> +	 * 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 (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
> +		pcc_send_data(chan, NULL);
> +}
> +
>  /**
>   * pcc_mbox_irq - PCC mailbox interrupt handler
>   * @irq:	interrupt number
> @@ -306,14 +326,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 +365,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);

devm doesn't seem appropriate here given we have manual management
of other resources, so the ordering will be different in remove
vs probe.

So I'd handle release of this manually in mbox_free_channel()


>  	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 {
Spec reference would be useful for this.
Looks to be Table 14.12 in acpi 6.5.
I can't remember convention in this file for whether you need
to find earliest spec for references or not.

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

Maybe this should be something like
PCC_EXT_FLAGS_ACK_MASK
to give a hint of where it applies.
Also, can we keep name closer to the spec (even though it's
meaning is that we must ack the command when done)

PCC_EXT_FLAGS_NOTIFY_ON_COMPLETION_MASK


>  
>  #ifdef CONFIG_PCC
>  extern struct pcc_mbox_chan *

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

[Jonathan Cameron]

On Thu, 11 Jul 2024 22:36:25 -0400
admiyo@os.amperecomputing.com wrote:

> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Note that this patch is for code that will be merged
> in via ACPICA changes.  The corresponding patch in ACPCA
Typo in ACPICA

Add a link to the patch in the acpica tree as then
its easier to identify exactly what needs pulling in before
this merges.

> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c
> index fff48001d7ef..9f8cfdc51637 100644
> --- a/drivers/acpi/acpica/rsaddr.c
> +++ b/drivers/acpi/acpica/rsaddr.c
> @@ -282,9 +282,10 @@ 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);
> -	}
>  
>  	/* Get the Resource Type and General Flags */
>  

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

[Jonathan Cameron]

On Thu, 11 Jul 2024 22:36:26 -0400
admiyo@os.amperecomputing.com wrote:

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

This will be fairly superficial because it's a while
since I last looked an mctp driver..

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

Oddly short line wrapping.  Aim for 75 chars limit.

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

Various general comments inline.

Jonathan

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
> new file mode 100644
> index 000000000000..055d6408e1d7
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,325 @@

...

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

Spec references good for this.

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

This is defined in patch 1.

> +
> +struct mctp_pcc_hdr {
> +	u32 signature;
> +	u32 flags;
> +	u32 length;
> +	char mctp_signature[4];

Use the MCTP_SIGNATURE_LENGTH define to ensure these are kept in sync.

> +};

...

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

Use consistent naming. Not sure why it is mctp_pcc_hdr here and pcc_header
in the tx function.

> +	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->pcc_comm_inbox_addr,
> +		      sizeof(struct mctp_pcc_hdr));

sizeof(mctp_pcc_hdr)

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

set at declaration.

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

sizeof(pcc_header)

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

prefix with mctp_pcct or similar.
Very high chance of a name clash in future on something called
simply lookup_context.


> +	int index;
> +	u32 inbox_index;
> +	u32 outbox_index;
> +};
> +
> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
prefix the function name as well.

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

I would take all registration device managed. It makes error handling
simpler and drops need for remove() function.

> +
> +	dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",

Use local dev variable (init that earlier)

> +		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");
With dev being set earlier as suggested below, use just dev here.

> +		return -EINVAL;
> +	}
> +	inbox_index = context.inbox_index;
> +	outbox_index = context.outbox_index;
> +	dev = &acpi_dev->dev;

Do this at declaration above.

> +
> +	snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
> +	ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
> +			    mctp_pcc_setup);
There are only devm versions of the more complex alloc_netdev (ethernet etc)
so best bet here probably to use
devm_add_action_or_reset() and a local bit of cleanup.

> +	if (!ndev)
> +		return -ENOMEM;
> +	mctp_pcc_dev = netdev_priv(ndev);
> +	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);

with devm throughout, use
		return dev_err_probe() here and in 
other failure paths in probe() and functions only called from probe()

> +		goto free_netdev;
> +	}
Use a devm_add_action_or_reset() here as well


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

And one here,

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

That will avoid ordering issues with the devm calls as we will know
that tear down will occur in opposite order to setup.
As it stands your netdev is registered long after you've ripped out
the stuff it uses.

> +	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;
I'd set all the simpler parts of this (ones without
allocations) in one block rather than some before and
some after hte allocations.


> +
> +	/* There is no clean way to pass the MTU
> +	 * to the callback function used for registration,
Wrap closer to 80 chars.

> +	 * 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);
devm_register_netdev()
> +	if (rc)
> +		goto cleanup_in_channel;
> +	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);

mctp_unregster_netdev()
but all this and remove() below will go away if you follow
suggestion on devm throughout.
> +	free_netdev(ndev);
> +	return rc;
> +}
> +
> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
> +
> +	pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
> +	pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
> +	mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
No free_netdev()?  Anyhow, devm will handle all this so you won't
have a remove function at all when you are done with that conversion.

> +}
> +
> +static const struct acpi_device_id mctp_pcc_device_ids[] = {
> +	{ "DMT0001", 0},
Can skip setting the zero as C will do that for you anyway
an you don't care about the value.
Space before }

> +	{ "", 0},

{} should be sufficient and would be more common way of
terminating such a list.  Note no comma as we don't want to
ever add anything after it.



> +};
> +
> +static struct acpi_driver mctp_pcc_driver = {
> +	.name = "mctp_pcc",
> +	.class = "Unknown",

Perhaps one for Rafael, what should this be?

> +	.ids = mctp_pcc_device_ids,
> +	.ops = {
> +		.add = mctp_pcc_driver_add,
> +		.remove = mctp_pcc_driver_remove,
> +	},
> +};
> +
> +module_acpi_driver(mctp_pcc_driver);
> +
> +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
> +
> +MODULE_DESCRIPTION("MCTP PCC device");

Stick ACPI in the description. 


> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");

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

[Adam Young]

>>+ * @shmem_base_addr: the virtual memory address of the shared buffer

>If you are only going to map this from this pointer for the
>initiator/responder shared memory region, maybe it would benefit
>from a more specific name?


I am not certain what would be more correct.


On 8/1/24 07:41, Jonathan Cameron wrote:

>> +	pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>> +					      pchan->chan.shmem_base_addr,
>> +					      pchan->chan.shmem_size);
> devm doesn't seem appropriate here given we have manual management
> of other resources, so the ordering will be different in remove
> vs probe.
>
> So I'd handle release of this manually in mbox_free_channel()


How fixed are you on this?  mbox_free_channel is the parent code, and 
knows nothing about this resource.  It does no specific resource cleanup.

The only place we could release it is in the pcc_mbox_free, but that is 
essentially a call to the parent function.

All other comments should be addressed in the next version.

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

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:21:06 -0400
Adam Young <admiyo@amperemail.onmicrosoft.com> wrote:

> >>+ * @shmem_base_addr: the virtual memory address of the shared buffer  
> 
> >If you are only going to map this from this pointer for the
> >initiator/responder shared memory region, maybe it would benefit
> >from a more specific name?  
> 
> 
> I am not certain what would be more correct.
> 
> 
> On 8/1/24 07:41, Jonathan Cameron wrote:
> 
> >> +	pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
> >> +					      pchan->chan.shmem_base_addr,
> >> +					      pchan->chan.shmem_size);  
> > devm doesn't seem appropriate here given we have manual management
> > of other resources, so the ordering will be different in remove
> > vs probe.
> >
> > So I'd handle release of this manually in mbox_free_channel()  
> 
> 
> How fixed are you on this?  mbox_free_channel is the parent code, and 
> knows nothing about this resource.  It does no specific resource cleanup.


I've lost context on this unfortunately and don't have time to look
back at it this week. Maybe right answer is a cleanup callback?

> 
> The only place we could release it is in the pcc_mbox_free, but that is 
> essentially a call to the parent function.
> 
> All other comments should be addressed in the next version.
> 
>