drivers/acpi/acpica/rsaddr.c | 2 +- drivers/mailbox/pcc.c | 5 +- drivers/net/mctp/Kconfig | 12 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 372 +++++++++++++++++++++++++++++++++++ include/acpi/pcc.h | 1 + 6 files changed, 391 insertions(+), 2 deletions(-) create mode 100644 drivers/net/mctp/mctp-pcc.c
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 its corresponding channels are specified via ACPI. Adam Young (3): mctp pcc: Implement MCTP over PCC Transport mctp pcc: Allow PCC Data Type in MCTP resource. mctp pcc: RFC Check before sending MCTP PCC response ACK drivers/acpi/acpica/rsaddr.c | 2 +- drivers/mailbox/pcc.c | 5 +- drivers/net/mctp/Kconfig | 12 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 372 +++++++++++++++++++++++++++++++++++ include/acpi/pcc.h | 1 + 6 files changed, 391 insertions(+), 2 deletions(-) create mode 100644 drivers/net/mctp/mctp-pcc.c -- 2.34.1
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. 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 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 | 5 +- drivers/net/mctp/Kconfig | 13 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 373 +++++++++++++++++++++++++++++++++++ include/acpi/pcc.h | 1 + 6 files changed, 393 insertions(+), 2 deletions(-) create mode 100644 drivers/net/mctp/mctp-pcc.c -- 2.34.1
Apologies. I just saw that there was more feedback on the previous patchset. I am addressing that now. On 6/19/24 16:05, admiyo@os.amperecomputing.com wrote: > 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. > > 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 > > 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 | 5 +- > drivers/net/mctp/Kconfig | 13 ++ > drivers/net/mctp/Makefile | 1 + > drivers/net/mctp/mctp-pcc.c | 373 +++++++++++++++++++++++++++++++++++ > include/acpi/pcc.h | 1 + > 6 files changed, 393 insertions(+), 2 deletions(-) > create mode 100644 drivers/net/mctp/mctp-pcc.c >
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 | 5 ++++-
include/acpi/pcc.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 94885e411085..774727b89693 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,7 @@ 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.1From: 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.1From: Adam Young <admiyo@amperecomputing.com>
Implementation of DMTF DSP:0292
Management Control Transport Protocol(MCTP) over
Platform Communication Channel(PCC) network driver.
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 | 373 ++++++++++++++++++++++++++++++++++++
3 files changed, 387 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..1ce9c58099cb
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,373 @@
+// 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;
+
+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;
+ int rc;
+
+ 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;
+}
+
+static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
+ struct device *dev, int inbox_index,
+ int outbox_index)
+{
+ struct mctp_pcc_ndev *mctp_pcc_dev;
+ struct net_device *ndev;
+ int mctp_pcc_mtu;
+ char name[32];
+ int rc;
+
+ 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;
+}
+
+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 *adev)
+{
+ int outbox_index;
+ int inbox_index;
+ acpi_handle dev_handle;
+ acpi_status status;
+ struct lookup_context context = {0, 0, 0};
+
+ dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n",
+ acpi_device_hid(adev));
+ dev_handle = acpi_device_handle(adev);
+ status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
+ &context);
+ if (!ACPI_SUCCESS(status)) {
+ dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS");
+ return -EINVAL;
+ }
+ inbox_index = context.inbox_index;
+ outbox_index = context.outbox_index;
+ return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index,
+ outbox_index);
+}
+
+/* pass in adev=NULL to remove all devices
+ */
+static void mctp_pcc_driver_remove(struct acpi_device *adev)
+{
+ struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
+ struct list_head *ptr;
+ struct list_head *tmp;
+
+ list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
+ struct net_device *ndev;
+
+ mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
+ if (adev && 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);
+ if (adev)
+ 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,
+};
+
+static int __init mctp_pcc_mod_init(void)
+{
+ int rc;
+
+ pr_debug("Initializing MCTP over PCC transport driver\n");
+ INIT_LIST_HEAD(&mctp_pcc_ndevs);
+ rc = acpi_bus_register_driver(&mctp_pcc_driver);
+ if (rc < 0)
+ ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
+ return rc;
+}
+
+static __exit void mctp_pcc_mod_exit(void)
+{
+ pr_debug("Removing MCTP over PCC transport driver\n");
+ mctp_pcc_driver_remove(NULL);
+ acpi_bus_unregister_driver(&mctp_pcc_driver);
+}
+
+module_init(mctp_pcc_mod_init);
+module_exit(mctp_pcc_mod_exit);
+
+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.1Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.10-rc4 next-20240619]
[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/20240620-040816
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240619200552.119080-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
config: arm64-kismet-CONFIG_ACPI-CONFIG_MCTP_TRANSPORT_PCC-0-0 (https://download.01.org/0day-ci/archive/20240620/202406202206.9e7oGmfh-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20240620/202406202206.9e7oGmfh-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/202406202206.9e7oGmfh-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for ACPI when selected by MCTP_TRANSPORT_PCC
WARNING: unmet direct dependencies detected for ACPI
Depends on [n]: ARCH_SUPPORTS_ACPI [=n]
Selected by [y]:
- MCTP_TRANSPORT_PCC [=y] && NETDEVICES [=y] && MCTP [=y]
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.10-rc4 next-20240619]
[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/20240620-040816
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240619200552.119080-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20240620/202406201832.4oSZmptU-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240620/202406201832.4oSZmptU-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/202406201832.4oSZmptU-lkp@intel.com/
All warnings (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);
| ^~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_tx':
>> drivers/net/mctp/mctp-pcc.c:116:13: warning: unused variable 'rc' [-Wunused-variable]
116 | int rc;
| ^~
drivers/net/mctp/mctp-pcc.c: In function 'create_mctp_pcc_netdev':
drivers/net/mctp/mctp-pcc.c:223:17: error: invalid use of undefined type 'struct acpi_device'
223 | acpi_dev->driver_data = mctp_pcc_dev;
| ^~
In file included from include/linux/printk.h:573,
from include/asm-generic/bug.h:22,
from arch/arc/include/asm/bug.h:30,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/arc/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:291:22: error: invalid use of undefined type 'struct acpi_device'
291 | dev_dbg(&adev->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:291:9: note: in expansion of macro 'dev_dbg'
291 | dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:292:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
292 | acpi_device_hid(adev));
| ^~~~~~~~~~~~~~~
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:291:9: note: in expansion of macro 'dev_dbg'
291 | dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:293:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
293 | dev_handle = acpi_device_handle(adev);
| ^~~~~~~~~~~~~~~~~~
| acpi_device_dep
drivers/net/mctp/mctp-pcc.c:293:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
293 | dev_handle = acpi_device_handle(adev);
| ^
In file included from include/linux/device.h:15,
from include/linux/acpi.h:14:
drivers/net/mctp/mctp-pcc.c:297:30: error: invalid use of undefined type 'struct acpi_device'
297 | dev_err(&adev->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:297:17: note: in expansion of macro 'dev_err'
297 | dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS");
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:302:50: error: invalid use of undefined type 'struct acpi_device'
302 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index,
| ^~
drivers/net/mctp/mctp-pcc.c: At top level:
drivers/net/mctp/mctp-pcc.c:336:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
336 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:337:10: error: 'struct acpi_driver' has no member named 'name'
337 | .name = "mctp_pcc",
| ^~~~
drivers/net/mctp/mctp-pcc.c:337:17: warning: excess elements in struct initializer
337 | .name = "mctp_pcc",
| ^~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:337:17: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:338:10: error: 'struct acpi_driver' has no member named 'class'
338 | .class = "Unknown",
| ^~~~~
drivers/net/mctp/mctp-pcc.c:338:18: warning: excess elements in struct initializer
338 | .class = "Unknown",
| ^~~~~~~~~
drivers/net/mctp/mctp-pcc.c:338:18: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:339:10: error: 'struct acpi_driver' has no member named 'ids'
339 | .ids = mctp_pcc_device_ids,
| ^~~
vim +/rc +116 drivers/net/mctp/mctp-pcc.c
109
110 static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
111 {
112 struct mctp_pcc_hdr pcc_header;
113 struct mctp_pcc_ndev *mpnd;
114 void __iomem *buffer;
115 unsigned long flags;
> 116 int rc;
117
118 ndev->stats.tx_bytes += skb->len;
119 ndev->stats.tx_packets++;
120 mpnd = netdev_priv(ndev);
121
122 spin_lock_irqsave(&mpnd->lock, flags);
123 buffer = mpnd->pcc_comm_outbox_addr;
124 pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index;
125 pcc_header.flags = PCC_HEADER_FLAGS;
126 memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
127 pcc_header.length = skb->len + SIGNATURE_LENGTH;
128 memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
129 memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
130 mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
131 NULL);
132 spin_unlock_irqrestore(&mpnd->lock, flags);
133
134 dev_consume_skb_any(skb);
135 return NETDEV_TX_OK;
136 }
137
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, 19 Jun 2024 16:05:52 -0400 admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@amperecomputing.com>
>
> Implementation of DMTF DSP:0292
> Management Control Transport Protocol(MCTP) over
> Platform Communication Channel(PCC) network driver.
>
> 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.
This patch breaks allmodconfig build:
drivers/net/mctp/mctp-pcc.c:116:6: warning: unused variable 'rc' [-Wunused-variable]
116 | int rc;
| ^~
drivers/net/mctp/mctp-pcc.c:344:3: error: field designator 'owner' does not refer to any field in type 'struct acpi_driver'
344 | .owner = THIS_MODULE,
| ~^~~~~~~~~~~~~~~~~~~
In addition, please make sure you don't add new checkpatch warnings,
use:
./scripts/checkpatch.pl --strict --max-line-length=80 $patch
Please wait with the repost until next week, unless you get a review
from Jeremy before that. When reposting start a new thread, don't
repost in reply to previous posting. Instead add a lore link to the
previous version, like this:
https://lore.kernel.org/20240619200552.119080-1-admiyo@os.amperecomputing.com/
See also:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#changes-requested
--
pw-bot: cr
On 6/19/24 19:26, Jakub Kicinski wrote:
> On Wed, 19 Jun 2024 16:05:52 -0400 admiyo@os.amperecomputing.com wrote:
>> From: Adam Young <admiyo@amperecomputing.com>
>>
>> Implementation of DMTF DSP:0292
>> Management Control Transport Protocol(MCTP) over
>> Platform Communication Channel(PCC) network driver.
>>
>> 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.
> This patch breaks allmodconfig build:
>
> drivers/net/mctp/mctp-pcc.c:116:6: warning: unused variable 'rc' [-Wunused-variable]
> 116 | int rc;
> | ^~
Funny, I see that only when building on x86_64, not on arm64. I will fix.
> drivers/net/mctp/mctp-pcc.c:344:3: error: field designator 'owner' does not refer to any field in type 'struct acpi_driver'
> 344 | .owner = THIS_MODULE,
> | ~^~~~~~~~~~~~~~~~~~~
Not sure how you are getting that last error. I do not, and the v6.9.3
code base has this in include/acpi/acpi_bus.h at line 166
struct acpi_driver {
char name[80];
char class[80];
const struct acpi_device_id *ids; /* Supported Hardware IDs */
unsigned int flags;
struct acpi_device_ops ops;
struct device_driver drv;
struct module *owner;
};
> In addition, please make sure you don't add new checkpatch warnings,
> use:
>
> ./scripts/checkpatch.pl --strict --max-line-length=80 $patch
That runs clean.
>
> Please wait with the repost until next week, unless you get a review
> from Jeremy before that. When reposting start a new thread, don't
> repost in reply to previous posting. Instead add a lore link to the
> previous version, like this:
>
> https://lore.kernel.org/20240619200552.119080-1-admiyo@os.amperecomputing.com/
>
> See also:
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#changes-requested
On Wed, 19 Jun 2024 23:24:41 -0400 Adam Young wrote: > > drivers/net/mctp/mctp-pcc.c:344:3: error: field designator 'owner' does not refer to any field in type 'struct acpi_driver' > > 344 | .owner = THIS_MODULE, > > | ~^~~~~~~~~~~~~~~~~~~ > > Not sure how you are getting that last error. I do not, and the v6.9.3 > code base has this in include/acpi/acpi_bus.h at line 166 v6.9.3 is not a development kernel. Your patches will be merged to net-next. Read more of the process doc I linked in the previous message. > That runs clean. It doesn't on patches 1 and 2, but you're right, I think it's intentional / because of the "historical" ACPI coding style :(
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. 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 | 5 +- drivers/net/mctp/Kconfig | 13 ++ drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-pcc.c | 361 +++++++++++++++++++++++++++++++++++ include/acpi/pcc.h | 1 + 6 files changed, 381 insertions(+), 2 deletions(-) create mode 100644 drivers/net/mctp/mctp-pcc.c -- 2.34.1
On 5/29/2024 3:18 AM, admiyo@os.amperecomputing.com wrote: > 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. > > 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 | 5 +- > drivers/net/mctp/Kconfig | 13 ++ > drivers/net/mctp/Makefile | 1 + > drivers/net/mctp/mctp-pcc.c | 361 +++++++++++++++++++++++++++++++++++ > include/acpi/pcc.h | 1 + > 6 files changed, 381 insertions(+), 2 deletions(-) > create mode 100644 drivers/net/mctp/mctp-pcc.c > Tested OK on Arm FVP. Tested-by: John Chung <john.chung@arm.com>
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 | 5 ++++-
include/acpi/pcc.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 94885e411085..774727b89693 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,7 @@ 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.1On Tue, May 28, 2024 at 03:18:21PM -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.
>
I don't have all the 3 patches. Is this sent by error or am I expected
to just review this patch while other 2 are not mailbox related ?
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
> drivers/mailbox/pcc.c | 5 ++++-
> include/acpi/pcc.h | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 94885e411085..774727b89693 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
We need to remove this and detect when it can be true if the default expected
is false.
--
Regards,
Sudeep
On 2024-05-29 at 00:48:21, admiyo@os.amperecomputing.com (admiyo@os.amperecomputing.com) wrote:
> From: Adam Young <admiyo@amperecomputing.com>
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 94885e411085..774727b89693 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;
Reverse xmas tree.
> u64 val;
> int ret;
>
>
From: Adam Young <admiyo@amperecomputing.com>
Note that this patch sfor code that will be merged
in via ACPICA changes. The corresponding patch in ACPCA
has already merged.
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..6bd9704f17b0 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 != 10)) {
return (FALSE);
}
--
2.34.1On 2024-05-29 at 00:48:22, admiyo@os.amperecomputing.com (admiyo@os.amperecomputing.com) wrote:
> From: Adam Young <admiyo@amperecomputing.com>
> --- 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 != 10)) {
use macros or enums instead of hard coded numbers. That will improve code readability.
> return (FALSE);
> }
>
> --
> 2.34.1
>
On 5/28/24 23:25, Ratheesh Kannoth wrote:
> On 2024-05-29 at 00:48:22, admiyo@os.amperecomputing.com (admiyo@os.amperecomputing.com) wrote:
>> From: Adam Young <admiyo@amperecomputing.com>
>> --- 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 != 10)) {
> use macros or enums instead of hard coded numbers. That will improve code readability.
In general I agree, but this code is generated from ACPICA and will not
be directly commited. The corresponding patch to acpica has already
merged. What you see here is what the code will look like post-process
from ACPICA conversion.
>
>> return (FALSE);
>> }
>>
>> --
>> 2.34.1
>>
From: Adam Young <admiyo@amperecomputing.com>
Implementation of DMTF DSP:0292
Management Control Transport Protocol(MCTP) over
Platform Communication Channel(PCC)
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.
---
drivers/net/mctp/Kconfig | 13 ++
drivers/net/mctp/Makefile | 1 +
drivers/net/mctp/mctp-pcc.c | 361 ++++++++++++++++++++++++++++++++++++
3 files changed, 375 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..d97f40789fd8
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,361 @@
+// 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>
+#include <net/pkt_sched.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_DWORD_TYPE 0x0c
+
+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;
+ void (*cleanup_channel)(struct pcc_mbox_chan *in_chan);
+};
+
+static struct list_head 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_skb_cb *cb;
+ struct sk_buff *skb;
+ u32 length_offset;
+ u32 flags_offset;
+ void *skb_buf;
+ u32 data_len;
+ u32 flags;
+
+ mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
+ length_offset = offsetof(struct mctp_pcc_hdr, length);
+ data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
+ MCTP_HEADER_LENGTH;
+
+ 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;
+ skb->dev = mctp_pcc_dev->mdev.dev;
+ netif_rx(skb);
+
+ flags_offset = offsetof(struct mctp_pcc_hdr, flags);
+ flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
+ mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 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;
+ int rc;
+
+ netif_stop_queue(ndev);
+ ndev->stats.tx_bytes += skb->len;
+ ndev->stats.tx_packets++;
+ mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
+
+ spin_lock_irqsave(&mpnd->lock, flags);
+ buffer = mpnd->pcc_comm_outbox_addr;
+ pcc_header.signature = PCC_MAGIC;
+ pcc_header.flags = 0x1;
+ 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);
+ rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
+ NULL);
+ spin_unlock_irqrestore(&mpnd->lock, flags);
+
+ dev_consume_skb_any(skb);
+ netif_start_queue(ndev);
+ if (!rc)
+ return NETDEV_TX_OK;
+ return NETDEV_TX_BUSY;
+}
+
+static const struct net_device_ops mctp_pcc_netdev_ops = {
+ .ndo_start_xmit = mctp_pcc_tx,
+ .ndo_uninit = NULL
+};
+
+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 = DEFAULT_TX_QUEUE_LEN;
+ ndev->flags = IFF_NOARP;
+ ndev->netdev_ops = &mctp_pcc_netdev_ops;
+ ndev->needs_free_netdev = true;
+}
+
+static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
+ struct device *dev, int inbox_index,
+ int outbox_index)
+{
+ struct mctp_pcc_ndev *mctp_pcc_dev;
+ struct net_device *ndev;
+ int mctp_pcc_mtu;
+ char name[32];
+ int rc;
+
+ 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->cleanup_channel = pcc_mbox_free_channel;
+ 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;
+
+/* 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_pcc_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:
+ mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
+cleanup_out_channel:
+ mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
+free_netdev:
+ unregister_netdev(ndev);
+ free_netdev(ndev);
+ return rc;
+}
+
+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 *adev)
+{
+ int outbox_index;
+ int inbox_index;
+ acpi_handle dev_handle;
+ acpi_status status;
+ struct lookup_context context = {0, 0, 0};
+
+ dev_dbg(&adev->dev, "Adding mctp_pcc device for HID %s\n",
+ acpi_device_hid(adev));
+ dev_handle = acpi_device_handle(adev);
+ status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
+ &context);
+ if (!ACPI_SUCCESS(status)) {
+ dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS");
+ return -EINVAL;
+ }
+ inbox_index = context.inbox_index;
+ outbox_index = context.outbox_index;
+ return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index,
+ outbox_index);
+};
+
+/* pass in adev=NULL to remove all devices
+ */
+static void mctp_pcc_driver_remove(struct acpi_device *adev)
+{
+ struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
+ struct list_head *ptr;
+ struct list_head *tmp;
+
+ list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
+ struct net_device *ndev;
+
+ mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
+ if (adev && mctp_pcc_dev->acpi_device == adev)
+ continue;
+
+ mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
+ mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
+ ndev = mctp_pcc_dev->mdev.dev;
+ if (ndev)
+ mctp_unregister_netdev(ndev);
+ list_del(ptr);
+ if (adev)
+ 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,
+ .notify = NULL,
+ },
+ .owner = THIS_MODULE,
+
+};
+
+static int __init mctp_pcc_mod_init(void)
+{
+ int rc;
+
+ pr_debug("Initializing MCTP over PCC transport driver\n");
+ INIT_LIST_HEAD(&mctp_pcc_ndevs);
+ rc = acpi_bus_register_driver(&mctp_pcc_driver);
+ if (rc < 0)
+ ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
+ return rc;
+}
+
+static __exit void mctp_pcc_mod_exit(void)
+{
+ pr_debug("Removing MCTP over PCC transport driver\n");
+ mctp_pcc_driver_remove(NULL);
+ acpi_bus_unregister_driver(&mctp_pcc_driver);
+}
+
+module_init(mctp_pcc_mod_init);
+module_exit(mctp_pcc_mod_exit);
+
+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.1Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc1 next-20240529]
[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/20240529-072116
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240528191823.17775-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240529/202405292134.LNr25Q9s-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405292134.LNr25Q9s-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/202405292134.LNr25Q9s-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:173:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
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:8:
In file included from include/linux/if_arp.h:22:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
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:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
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:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
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:193:3: error: call to undeclared function 'devm_ioremap'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
193 | devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
| ^
>> drivers/net/mctp/mctp-pcc.c:192:36: error: incompatible integer to pointer conversion assigning to 'void *' from 'int' [-Wint-conversion]
192 | mctp_pcc_dev->pcc_comm_inbox_addr =
| ^
193 | devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
194 | mctp_pcc_dev->in_chan->shmem_size);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:199:37: error: incompatible integer to pointer conversion assigning to 'void *' from 'int' [-Wint-conversion]
199 | mctp_pcc_dev->pcc_comm_outbox_addr =
| ^
200 | devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
201 | mctp_pcc_dev->out_chan->shmem_size);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:276:15: error: incomplete definition of type 'struct acpi_device'
276 | dev_dbg(&adev->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:794:8: note: forward declaration of 'struct acpi_device'
794 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:277:3: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
277 | acpi_device_hid(adev));
| ^
drivers/net/mctp/mctp-pcc.c:277: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:278:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
278 | dev_handle = acpi_device_handle(adev);
| ^
drivers/net/mctp/mctp-pcc.c:278:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion]
278 | dev_handle = acpi_device_handle(adev);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:282:16: error: incomplete definition of type 'struct acpi_device'
282 | dev_err(&adev->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:794:8: note: forward declaration of 'struct acpi_device'
794 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:287:43: error: incomplete definition of type 'struct acpi_device'
287 | return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index,
| ~~~~^
include/linux/acpi.h:794:8: note: forward declaration of 'struct acpi_device'
794 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:322:27: error: variable has incomplete type 'struct acpi_driver'
322 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:322:15: note: forward declaration of 'struct acpi_driver'
322 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:341:7: error: call to undeclared function 'acpi_bus_register_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
341 | rc = acpi_bus_register_driver(&mctp_pcc_driver);
| ^
drivers/net/mctp/mctp-pcc.c:351:2: error: call to undeclared function 'acpi_bus_unregister_driver'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
351 | acpi_bus_unregister_driver(&mctp_pcc_driver);
| ^
18 warnings and 12 errors generated.
vim +/devm_ioremap +193 drivers/net/mctp/mctp-pcc.c
154
155 static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
156 struct device *dev, int inbox_index,
157 int outbox_index)
158 {
159 struct mctp_pcc_ndev *mctp_pcc_dev;
160 struct net_device *ndev;
161 int mctp_pcc_mtu;
162 char name[32];
163 int rc;
164
165 snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
166 ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
167 mctp_pcc_setup);
168 if (!ndev)
169 return -ENOMEM;
170 mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev);
171 INIT_LIST_HEAD(&mctp_pcc_dev->next);
172 spin_lock_init(&mctp_pcc_dev->lock);
173
174 mctp_pcc_dev->hw_addr.inbox_index = inbox_index;
175 mctp_pcc_dev->hw_addr.outbox_index = outbox_index;
176 mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback;
177 mctp_pcc_dev->cleanup_channel = pcc_mbox_free_channel;
178 mctp_pcc_dev->out_chan =
179 pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client,
180 outbox_index);
181 if (IS_ERR(mctp_pcc_dev->out_chan)) {
182 rc = PTR_ERR(mctp_pcc_dev->out_chan);
183 goto free_netdev;
184 }
185 mctp_pcc_dev->in_chan =
186 pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client,
187 inbox_index);
188 if (IS_ERR(mctp_pcc_dev->in_chan)) {
189 rc = PTR_ERR(mctp_pcc_dev->in_chan);
190 goto cleanup_out_channel;
191 }
> 192 mctp_pcc_dev->pcc_comm_inbox_addr =
> 193 devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
194 mctp_pcc_dev->in_chan->shmem_size);
195 if (!mctp_pcc_dev->pcc_comm_inbox_addr) {
196 rc = -EINVAL;
197 goto cleanup_in_channel;
198 }
199 mctp_pcc_dev->pcc_comm_outbox_addr =
200 devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
201 mctp_pcc_dev->out_chan->shmem_size);
202 if (!mctp_pcc_dev->pcc_comm_outbox_addr) {
203 rc = -EINVAL;
204 goto cleanup_in_channel;
205 }
206 mctp_pcc_dev->acpi_device = acpi_dev;
207 mctp_pcc_dev->inbox_client.dev = dev;
208 mctp_pcc_dev->outbox_client.dev = dev;
209 mctp_pcc_dev->mdev.dev = ndev;
210
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc1 next-20240529]
[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/20240529-072116
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240528191823.17775-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240529/202405292136.ZyuCa1Fc-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405292136.ZyuCa1Fc-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/202405292136.ZyuCa1Fc-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/net/mctp/mctp-pcc.c:331:10: error: 'struct acpi_driver' has no member named 'owner'
331 | .owner = THIS_MODULE,
| ^~~~~
In file included from include/linux/printk.h:6,
from include/asm-generic/bug.h:22,
from arch/loongarch/include/asm/bug.h:60,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/loongarch/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:
>> include/linux/init.h:180:21: error: initialization of 'const char *' from incompatible pointer type 'struct module *' [-Werror=incompatible-pointer-types]
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:331:18: note: in expansion of macro 'THIS_MODULE'
331 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
include/linux/init.h:180:21: note: (near initialization for 'mctp_pcc_driver.drv.name')
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:331:18: note: in expansion of macro 'THIS_MODULE'
331 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:322:45: warning: missing braces around initializer [-Wmissing-braces]
322 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_mod_init':
drivers/net/mctp/mctp-pcc.c:343:80: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
343 | ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
| ^
cc1: some warnings being treated as errors
vim +180 include/linux/init.h
f2511774863487e Arjan van de Ven 2009-12-13 177
5b20755b7780464 Masahiro Yamada 2023-11-26 178 #ifdef MODULE
5b20755b7780464 Masahiro Yamada 2023-11-26 179 extern struct module __this_module;
5b20755b7780464 Masahiro Yamada 2023-11-26 @180 #define THIS_MODULE (&__this_module)
5b20755b7780464 Masahiro Yamada 2023-11-26 181 #else
5b20755b7780464 Masahiro Yamada 2023-11-26 182 #define THIS_MODULE ((struct module *)0)
5b20755b7780464 Masahiro Yamada 2023-11-26 183 #endif
5b20755b7780464 Masahiro Yamada 2023-11-26 184
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Adam,
Thanks for the v2! Progress looks good, this seems simpler now too. Some
comments inline.
> From: Adam Young <admiyo@amperecomputing.com>
>
> Implementation of DMTF DSP:0292
> Management Control Transport Protocol(MCTP) over
> Platform Communication Channel(PCC)
>
> 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?
And can you include a brief summary of changes since the prior version
you have sent? (see
https://lore.kernel.org/netdev/20240220081053.1439104-1-jk@codeconstruct.com.au/
for an example, the marker lines means that the changes don't get
included in the commit log; Jakub may also have other preferences around
this...)
> 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"
Super minor: you have two spaces between "MCTP" and "PCC"
> + 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..d97f40789fd8
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,361 @@
> +// 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>
> +#include <net/pkt_sched.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_DWORD_TYPE 0x0c
> +
> +struct mctp_pcc_hdr {
> + u32 signature;
> + u32 flags;
> + u32 length;
> + char mctp_signature[4];
> +};
The usage of this struct isn't really consistent; you'll at least want
endian annotations on these members. More on that below though.
> +
> +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;
> + void (*cleanup_channel)(struct pcc_mbox_chan *in_chan);
Why this as a callback? There's only one possible function it can be.
> +};
> +
> +static struct list_head mctp_pcc_ndevs;
I'm not clear on what this list is doing; it seems to be for freeing
devices on module unload (or device remove).
However, the module will be refcounted while there are devices bound, so
module unload shouldn't be possible in that state. So the only time
you'll be iterating this list to free everything will be when it's
empty.
You could replace this with the mctp_pcc_driver_remove() just removing the
device passed in the argument, rather than doing any list iteration.
... unless I've missed something?
> +
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev;
> + struct mctp_skb_cb *cb;
> + struct sk_buff *skb;
> + u32 length_offset;
> + u32 flags_offset;
> + void *skb_buf;
> + u32 data_len;
> + u32 flags;
> +
> + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> + length_offset = offsetof(struct mctp_pcc_hdr, length);
> + data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
> + MCTP_HEADER_LENGTH;
Doing this using offsetof with separate readl()s is a bit clunky. Can
you memcpy_fromio the whole header, and use the appropriate endian
accessors?
(this would match the behaviour in the tx path)
Also, maybe check that data_len is sensible before allocating.
> +
> + 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));
Any benefit in including the pcc_hdr in the skb?
(not necessarily an issue, just asking...)
> + skb_reset_network_header(skb);
> + cb = __mctp_cb(skb);
> + cb->halen = 0;
> + skb->dev = mctp_pcc_dev->mdev.dev;
> + netif_rx(skb);
> +
> + flags_offset = offsetof(struct mctp_pcc_hdr, flags);
> + flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
> + mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 0;
Might be best to define what the flags bits mean, rather than
magic-numbering this.
Does anything need to tell the mailbox driver to do that ack after
setting ack_rx?
> +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;
> + int rc;
> +
> + netif_stop_queue(ndev);
Do you need to stop and restart the queue? Your handling is atomic.
> + ndev->stats.tx_bytes += skb->len;
> + ndev->stats.tx_packets++;
> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
no need for this cast, netdev_priv() returns void *
> +
> + spin_lock_irqsave(&mpnd->lock, flags);
> + buffer = mpnd->pcc_comm_outbox_addr;
> + pcc_header.signature = PCC_MAGIC;
> + pcc_header.flags = 0x1;
Magic numbers for flags here too
> + 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);
> + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
> + NULL);
> + spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> + dev_consume_skb_any(skb);
> + netif_start_queue(ndev);
> + if (!rc)
> + return NETDEV_TX_OK;
> + return NETDEV_TX_BUSY;
I think you want to return NETDEV_TX_OK unconditionally here, or at
least you need to change the queue handling; see the comment for the
ndo_start_xmit callback.
> +}
> +
> +static const struct net_device_ops mctp_pcc_netdev_ops = {
> + .ndo_start_xmit = mctp_pcc_tx,
> + .ndo_uninit = NULL
No need for this assignment.
> +};
> +
> +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 = DEFAULT_TX_QUEUE_LEN;
> + ndev->flags = IFF_NOARP;
> + ndev->netdev_ops = &mctp_pcc_netdev_ops;
> + ndev->needs_free_netdev = true;
> +}
> +
> +static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
> + struct device *dev, int inbox_index,
> + int outbox_index)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev;
> + struct net_device *ndev;
> + int mctp_pcc_mtu;
> + char name[32];
> + int rc;
> +
> + 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->cleanup_channel = pcc_mbox_free_channel;
> + 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;
You probably want the link back too:
acpi_dev->driver_data = mctp_pcc_dev;
> + mctp_pcc_dev->inbox_client.dev = dev;
> + mctp_pcc_dev->outbox_client.dev = dev;
> + mctp_pcc_dev->mdev.dev = ndev;
> +
> +/* There is no clean way to pass the MTU to the callback function
> + * used for registration, so set the values ahead of time.
> + */
Super minor, but keep this aligned with the code indent.
> + mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size -
> + sizeof(struct mctp_pcc_hdr);
> + ndev->mtu = mctp_pcc_mtu;
> + ndev->max_mtu = mctp_pcc_mtu;
> + ndev->min_mtu = MCTP_MIN_MTU;
Same as last review: I'd recommend setting the MTU to the minimum, and
leaving it up to userspace to do a `mctp link mctppcc0 mtu <whatever>`.
This means you don't break things if/when you ever encounter remote
endpoints that don't support the max mtu.
If the driver could reliably detect the remote max MTU, then what you
have is fine (and the driver would then set the appropriate MTU
here).
However, if that probing *cannot* be done at the driver level, or needs
any userspace interaction to do so (say, a higher level protocol), then
you've set a MTU that could be unusable in this initial state. In that
case, setting it low to start with means that the link is usable by
default.
Userspace can then hard-code a higher MTU if you like, and that can be
adjusted later as appropriate.
(this is a lesson learnt from the i2c transport...)
> +/* pass in adev=NULL to remove all devices
> + */
> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
> + struct list_head *ptr;
> + struct list_head *tmp;
> +
> + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
> + struct net_device *ndev;
> +
> + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> + if (adev && mctp_pcc_dev->acpi_device == adev)
> + continue;
> +
> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
> + ndev = mctp_pcc_dev->mdev.dev;
> + if (ndev)
> + mctp_unregister_netdev(ndev);
> + list_del(ptr);
> + if (adev)
> + break;
> + }
> +};
Assuming we don't need the free-everything case, how about something
like:
static void mctp_pcc_driver_remove(struct acpi_device *adev)
{
struct mctp_pcc_ndev *mctp_pcc_dev = acpi_driver_data(adev);
mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
mctp_unregister_netdev(mctp_pcc_dev->mdev.dev);
}
If you do need the list iteration: you're currently doing the cleanup on
every adev *except* the one you want:
> + if (adev && mctp_pcc_dev->acpi_device == adev)
> + continue;
I think you meant '!=' instead of '=='?
> +
> +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,
> + .notify = NULL,
Minor: don't need the zero assignment here.
> + },
> + .owner = THIS_MODULE,
> +
> +};
> +
[...]
> +static int __init mctp_pcc_mod_init(void)
> +{
> + int rc;
> +
> + pr_debug("Initializing MCTP over PCC transport driver\n");
> + INIT_LIST_HEAD(&mctp_pcc_ndevs);
> + rc = acpi_bus_register_driver(&mctp_pcc_driver);
> + if (rc < 0)
> + ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
> + return rc;
> +}
> +
> +static __exit void mctp_pcc_mod_exit(void)
> +{
> + pr_debug("Removing MCTP over PCC transport driver\n");
> + mctp_pcc_driver_remove(NULL);
> + acpi_bus_unregister_driver(&mctp_pcc_driver);
> +}
> +
> +module_init(mctp_pcc_mod_init);
> +module_exit(mctp_pcc_mod_exit);
If you end up removing the mctp_pcc_ndevs list, these can all be
replaced with module_acpi_driver(mctp_pcc_driver);
Cheers,
Jeremy
Pretty sure I have all these corrections in the next one. Some inline
comments.
On 5/28/24 23:02, Jeremy Kerr wrote:
> Hi Adam,
>
> Thanks for the v2! Progress looks good, this seems simpler now too. Some
> comments inline.
>
>> From: Adam Young <admiyo@amperecomputing.com>
>>
>> Implementation of DMTF DSP:0292
>> Management Control Transport Protocol(MCTP) over
>> Platform Communication Channel(PCC)
>>
>> 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?
>
> And can you include a brief summary of changes since the prior version
> you have sent? (see
> https://lore.kernel.org/netdev/20240220081053.1439104-1-jk@codeconstruct.com.au/
> for an example, the marker lines means that the changes don't get
> included in the commit log; Jakub may also have other preferences around
> this...)
They are all in the header patch.
>
>> 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"
> Super minor: you have two spaces between "MCTP" and "PCC"
>
>> + 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..d97f40789fd8
>> --- /dev/null
>> +++ b/drivers/net/mctp/mctp-pcc.c
>> @@ -0,0 +1,361 @@
>> +// 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>
>> +#include <net/pkt_sched.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_DWORD_TYPE 0x0c
>> +
>> +struct mctp_pcc_hdr {
>> + u32 signature;
>> + u32 flags;
>> + u32 length;
>> + char mctp_signature[4];
>> +};
> The usage of this struct isn't really consistent; you'll at least want
> endian annotations on these members. More on that below though.
>
>> +
>> +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;
>> + void (*cleanup_channel)(struct pcc_mbox_chan *in_chan);
> Why this as a callback? There's only one possible function it can be.
Vestige of code designed to work with multiple mailbox implementations.
Will remove.
>
>> +};
>> +
>> +static struct list_head mctp_pcc_ndevs;
> I'm not clear on what this list is doing; it seems to be for freeing
> devices on module unload (or device remove).
>
> However, the module will be refcounted while there are devices bound, so
> module unload shouldn't be possible in that state. So the only time
> you'll be iterating this list to free everything will be when it's
> empty.
>
> You could replace this with the mctp_pcc_driver_remove() just removing the
> device passed in the argument, rather than doing any list iteration.
>
> ... unless I've missed something?
There is no requirement that all the devices be unloaded in order for
the module to get unloaded.
It someone wants to disable the MCTP devices, they can unload the
module, and it gets cleaned up.
With ACPI, the devices never go away, they are defined in a table read
at start up and stay there. So without this change there is no way to
unload the module. Maybe it is just a convenience for development, but
I think most modules behave this way.
>
>
>> +
>> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
>> +{
>> + struct mctp_pcc_ndev *mctp_pcc_dev;
>> + struct mctp_skb_cb *cb;
>> + struct sk_buff *skb;
>> + u32 length_offset;
>> + u32 flags_offset;
>> + void *skb_buf;
>> + u32 data_len;
>> + u32 flags;
>> +
>> + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
>> + length_offset = offsetof(struct mctp_pcc_hdr, length);
>> + data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
>> + MCTP_HEADER_LENGTH;
> Doing this using offsetof with separate readl()s is a bit clunky. Can
> you memcpy_fromio the whole header, and use the appropriate endian
> accessors?
I was overthinking this. You are right.
>
> (this would match the behaviour in the tx path)
>
> Also, maybe check that data_len is sensible before allocating.
Good idea
>
>> +
>> + 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));
> Any benefit in including the pcc_hdr in the skb?
>
> (not necessarily an issue, just asking...)
It shows up in tracing of the packet. Useful for debugging.
>
>> + skb_reset_network_header(skb);
>> + cb = __mctp_cb(skb);
>> + cb->halen = 0;
>> + skb->dev = mctp_pcc_dev->mdev.dev;
>> + netif_rx(skb);
>> +
>> + flags_offset = offsetof(struct mctp_pcc_hdr, flags);
>> + flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
>> + mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 0;
> Might be best to define what the flags bits mean, rather than
> magic-numbering this.
I'll make a constant for the mask.
>
> Does anything need to tell the mailbox driver to do that ack after
> setting ack_rx?
Yes. It is in the previous patch, in the pcc_mailbox code. I
originally had it as a follow on, but reordered to make it a pre-req.
That allows me to inline this logic, making the driver easier to review
(I hope).
>
>> +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;
>> + int rc;
>> +
>> + netif_stop_queue(ndev);
> Do you need to stop and restart the queue? Your handling is atomic.
I guess not. This was just from following the examples of others. Will
remove.
>
>> + ndev->stats.tx_bytes += skb->len;
>> + ndev->stats.tx_packets++;
>> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
> no need for this cast, netdev_priv() returns void *
>
>> +
>> + spin_lock_irqsave(&mpnd->lock, flags);
>> + buffer = mpnd->pcc_comm_outbox_addr;
>> + pcc_header.signature = PCC_MAGIC;
>> + pcc_header.flags = 0x1;
> Magic numbers for flags here too
>
>> + 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);
>> + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
>> + NULL);
>> + spin_unlock_irqrestore(&mpnd->lock, flags);
>> +
>> + dev_consume_skb_any(skb);
>> + netif_start_queue(ndev);
>> + if (!rc)
>> + return NETDEV_TX_OK;
>> + return NETDEV_TX_BUSY;
> I think you want to return NETDEV_TX_OK unconditionally here, or at
> least you need to change the queue handling; see the comment for the
> ndo_start_xmit callback.
Will Do. Thanks for pointing out that comment, would never have seen it.
>
>> +}
>> +
>> +static const struct net_device_ops mctp_pcc_netdev_ops = {
>> + .ndo_start_xmit = mctp_pcc_tx,
>> + .ndo_uninit = NULL
> No need for this assignment.
>
>> +};
>> +
>> +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 = DEFAULT_TX_QUEUE_LEN;
>> + ndev->flags = IFF_NOARP;
>> + ndev->netdev_ops = &mctp_pcc_netdev_ops;
>> + ndev->needs_free_netdev = true;
>> +}
>> +
>> +static int create_mctp_pcc_netdev(struct acpi_device *acpi_dev,
>> + struct device *dev, int inbox_index,
>> + int outbox_index)
>> +{
>> + struct mctp_pcc_ndev *mctp_pcc_dev;
>> + struct net_device *ndev;
>> + int mctp_pcc_mtu;
>> + char name[32];
>> + int rc;
>> +
>> + 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->cleanup_channel = pcc_mbox_free_channel;
>> + 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;
> You probably want the link back too:
>
> acpi_dev->driver_data = mctp_pcc_dev;
>
>
>> + mctp_pcc_dev->inbox_client.dev = dev;
>> + mctp_pcc_dev->outbox_client.dev = dev;
>> + mctp_pcc_dev->mdev.dev = ndev;
>> +
>> +/* There is no clean way to pass the MTU to the callback function
>> + * used for registration, so set the values ahead of time.
>> + */
> Super minor, but keep this aligned with the code indent.
>
>> + mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size -
>> + sizeof(struct mctp_pcc_hdr);
>> + ndev->mtu = mctp_pcc_mtu;
>> + ndev->max_mtu = mctp_pcc_mtu;
>> + ndev->min_mtu = MCTP_MIN_MTU;
> Same as last review: I'd recommend setting the MTU to the minimum, and
> leaving it up to userspace to do a `mctp link mctppcc0 mtu <whatever>`.
> This means you don't break things if/when you ever encounter remote
> endpoints that don't support the max mtu.
>
> If the driver could reliably detect the remote max MTU, then what you
> have is fine (and the driver would then set the appropriate MTU
> here).
>
> However, if that probing *cannot* be done at the driver level, or needs
> any userspace interaction to do so (say, a higher level protocol), then
> you've set a MTU that could be unusable in this initial state. In that
> case, setting it low to start with means that the link is usable by
> default.
>
> Userspace can then hard-code a higher MTU if you like, and that can be
> adjusted later as appropriate.
>
> (this is a lesson learnt from the i2c transport...)
Ah...makes sense. I was not sure if our impl,enetation could handle
that yet, but it appears to,
If it can't, it is a bug we have to fix.
>
>> +/* pass in adev=NULL to remove all devices
>> + */
>> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
>> +{
>> + struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
>> + struct list_head *ptr;
>> + struct list_head *tmp;
>> +
>> + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
>> + struct net_device *ndev;
>> +
>> + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
>> + if (adev && mctp_pcc_dev->acpi_device == adev)
>> + continue;
>> +
>> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
>> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
>> + ndev = mctp_pcc_dev->mdev.dev;
>> + if (ndev)
>> + mctp_unregister_netdev(ndev);
>> + list_del(ptr);
>> + if (adev)
>> + break;
>> + }
>> +};
> Assuming we don't need the free-everything case, how about something
> like:
>
> static void mctp_pcc_driver_remove(struct acpi_device *adev)
> {
> struct mctp_pcc_ndev *mctp_pcc_dev = acpi_driver_data(adev);
>
> mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
> mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
> mctp_unregister_netdev(mctp_pcc_dev->mdev.dev);
> }
>
> If you do need the list iteration: you're currently doing the cleanup on
> every adev *except* the one you want:
>
>> + if (adev && mctp_pcc_dev->acpi_device == adev)
>> + continue;
> I think you meant '!=' instead of '=='?
Yes. Yes I did. This is code that has to be there for completeness,
but I don't really have a way to test, except for the "delete all" case.
>
>> +
>> +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,
>> + .notify = NULL,
> Minor: don't need the zero assignment here.
>
>> + },
>> + .owner = THIS_MODULE,
>> +
>> +};
>> +
> [...]
>
>> +static int __init mctp_pcc_mod_init(void)
>> +{
>> + int rc;
>> +
>> + pr_debug("Initializing MCTP over PCC transport driver\n");
>> + INIT_LIST_HEAD(&mctp_pcc_ndevs);
>> + rc = acpi_bus_register_driver(&mctp_pcc_driver);
>> + if (rc < 0)
>> + ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
>> + return rc;
>> +}
>> +
>> +static __exit void mctp_pcc_mod_exit(void)
>> +{
>> + pr_debug("Removing MCTP over PCC transport driver\n");
>> + mctp_pcc_driver_remove(NULL);
>> + acpi_bus_unregister_driver(&mctp_pcc_driver);
>> +}
>> +
>> +module_init(mctp_pcc_mod_init);
>> +module_exit(mctp_pcc_mod_exit);
> If you end up removing the mctp_pcc_ndevs list, these can all be
> replaced with module_acpi_driver(mctp_pcc_driver);
Yeah, I can't get away with that. The ACPI devices may still be there
when some one calls rmmod, and so we need to clean up the ndevs. It is
the only case where the module will be removed, as the ACPI devices
never go away.
>
> Cheers,
>
>
> Jeremy
Hi Adam,
> > And can you include a brief summary of changes since the prior version
> > you have sent?
> >
> They are all in the header patch.
Ah, neat. Can you include reviewers on CC for that 0/n patch then?
> > > +static struct list_head mctp_pcc_ndevs;
> > I'm not clear on what this list is doing; it seems to be for freeing
> > devices on module unload (or device remove).
> >
> > However, the module will be refcounted while there are devices bound, so
> > module unload shouldn't be possible in that state. So the only time
> > you'll be iterating this list to free everything will be when it's
> > empty.
> >
> > You could replace this with the mctp_pcc_driver_remove() just removing the
> > device passed in the argument, rather than doing any list iteration.
> >
> > ... unless I've missed something?
>
> There is no requirement that all the devices be unloaded in order for
> the module to get unloaded.
... aside from the driver refcounting. You're essentially replicating
the driver core's own facility for device-to-driver mappings here.
> It someone wants to disable the MCTP devices, they can unload the
> module, and it gets cleaned up.
>
> With ACPI, the devices never go away, they are defined in a table read
> at start up and stay there.
Sure, the ACPI bus devices may always be present, but you can still
unbind the driver from one device:
echo '<device-id>' > /sys/bus/acpi/drivers/mctp_pcc/unbind
- where device-id is one of the links to a device in that mctp_pcc dir.
then:
> So without this change there is no way to unload the module.
... with no devices bound, you can safely unload the module (but the
unload path will also perform that unbind anyway, more on that below).
> Maybe it is just a convenience for development, but I think most
> modules behave this way.
If you can avoid holding internal references to devices, you have a
whole class of bugs you can avoid.
> > Any benefit in including the pcc_hdr in the skb?
> >
> > (not necessarily an issue, just asking...)
> It shows up in tracing of the packet. Useful for debugging.
Sounds good!
> > Does anything need to tell the mailbox driver to do that ack after
> > setting ack_rx?
>
> Yes. It is in the previous patch, in the pcc_mailbox code. I
> originally had it as a follow on, but reordered to make it a pre-req.
> That allows me to inline this logic, making the driver easier to review
> (I hope).
OK. As far as I can tell here this is just setting a member of the
mailbox interface, but not calling back into the mailbox code. If this
is okay, then all good.
> > > + netif_stop_queue(ndev);
> > Do you need to stop and restart the queue? Your handling is atomic.
> I guess not. This was just from following the examples of others. Will
> remove.
Those examples (at least, in the MCTP drivers) will not have been able
to complete transmission until way later - say, after a separate
completion, or after a separate thread has processed the outgoing skb.
While that is happening, we may have stopped the queue.
In your case, you complete transmission entirely within the start_xmit
operation (*and* that path is atomic), so the queue is fine to remain
enabled.
> > > + if (adev && mctp_pcc_dev->acpi_device == adev)
> > > + continue;
> > I think you meant '!=' instead of '=='?
> Yes. Yes I did. This is code that has to be there for completeness,
> but I don't really have a way to test, except for the "delete all" case.
The 'unbind' example above will test this.
> > > +static int __init mctp_pcc_mod_init(void)
> > > +{
> > > + int rc;
> > > +
> > > + pr_debug("Initializing MCTP over PCC transport driver\n");
> > > + INIT_LIST_HEAD(&mctp_pcc_ndevs);
> > > + rc = acpi_bus_register_driver(&mctp_pcc_driver);
> > > + if (rc < 0)
> > > + ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n"));
> > > + return rc;
> > > +}
> > > +
> > > +static __exit void mctp_pcc_mod_exit(void)
> > > +{
> > > + pr_debug("Removing MCTP over PCC transport driver\n");
> > > + mctp_pcc_driver_remove(NULL);
> > > + acpi_bus_unregister_driver(&mctp_pcc_driver);
> > > +}
> > > +
> > > +module_init(mctp_pcc_mod_init);
> > > +module_exit(mctp_pcc_mod_exit);
> > If you end up removing the mctp_pcc_ndevs list, these can all be
> > replaced with module_acpi_driver(mctp_pcc_driver);
>
> Yeah, I can't get away with that. The ACPI devices may still be there
> when some one calls rmmod, and so we need to clean up the ndevs.
The core driver unregister path should unbind all devices before the
module is removed, so your mctp_pcc_driver_remove() should get invoked
on each individual device during that process.
(with the above != vs. == bug fixed, you'll probably find that
"delete-all" case will always hit an empty list)
... this is unless something is different about the ACPI bus type, but
it all looks standard from a brief look!
Cheers,
Jeremy
On Tue, 28 May 2024 15:18:23 -0400 admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@amperecomputing.com>
>
> Implementation of DMTF DSP:0292
> Management Control Transport Protocol(MCTP) over
> Platform Communication Channel(PCC)
>
> 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.
Missing your SoB, but please wait for more feedback before reposting.
> +#include <net/pkt_sched.h>
Hm, what do you need this include for?
> +#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_DWORD_TYPE 0x0c
Could you align the values using tabs?
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev;
> + struct mctp_skb_cb *cb;
> + struct sk_buff *skb;
> + u32 length_offset;
> + u32 flags_offset;
> + void *skb_buf;
> + u32 data_len;
> + u32 flags;
> +
> + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> + length_offset = offsetof(struct mctp_pcc_hdr, length);
> + data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
> + MCTP_HEADER_LENGTH;
> +
> + 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;
Please implement ndo_get_stats64, use of the core dev stats in drivers
is deprecated:
* @stats: Statistics struct, which was left as a legacy, use
* rtnl_link_stats64 instead
> + 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;
> + skb->dev = mctp_pcc_dev->mdev.dev;
netdev_alloc_skb() already sets dev
> + netif_rx(skb);
> +
> + flags_offset = offsetof(struct mctp_pcc_hdr, flags);
> + flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
> + mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 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;
> + int rc;
> +
> + netif_stop_queue(ndev);
Why?
> + ndev->stats.tx_bytes += skb->len;
> + ndev->stats.tx_packets++;
> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
> +
> + spin_lock_irqsave(&mpnd->lock, flags);
> + buffer = mpnd->pcc_comm_outbox_addr;
> + pcc_header.signature = PCC_MAGIC;
> + pcc_header.flags = 0x1;
> + 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);
> + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
> + NULL);
> + spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> + dev_consume_skb_any(skb);
> + netif_start_queue(ndev);
> + if (!rc)
> + return NETDEV_TX_OK;
> + return NETDEV_TX_BUSY;
> +}
> +
> +static const struct net_device_ops mctp_pcc_netdev_ops = {
> + .ndo_start_xmit = mctp_pcc_tx,
> + .ndo_uninit = NULL
No need to init things to NULL
> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
> + struct list_head *ptr;
> + struct list_head *tmp;
> +
> + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
> + struct net_device *ndev;
> +
> + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> + if (adev && mctp_pcc_dev->acpi_device == adev)
> + continue;
> +
> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
> + ndev = mctp_pcc_dev->mdev.dev;
> + if (ndev)
> + mctp_unregister_netdev(ndev);
> + list_del(ptr);
> + if (adev)
> + break;
> + }
> +};
spurious ;
> + .owner = THIS_MODULE,
> +
suprious new line
> +};
> +
--
pw-bot: cr
On 5/28/24 22:45, Jakub Kicinski wrote:
> On Tue, 28 May 2024 15:18:23 -0400 admiyo@os.amperecomputing.com wrote:
>> From: Adam Young <admiyo@amperecomputing.com>
>>
>> Implementation of DMTF DSP:0292
>> Management Control Transport Protocol(MCTP) over
>> Platform Communication Channel(PCC)
>>
>> 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.
> Missing your SoB, but please wait for more feedback before reposting.
Sorry, must have dropped it when redoing the last patch, It was on the
original and will be on the next version.
>
>> +#include <net/pkt_sched.h>
> Hm, what do you need this include for?
Gets the symbolic constant
>
>> +#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_DWORD_TYPE 0x0c
> Could you align the values using tabs?
Will do
>
>> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
>> +{
>> + struct mctp_pcc_ndev *mctp_pcc_dev;
>> + struct mctp_skb_cb *cb;
>> + struct sk_buff *skb;
>> + u32 length_offset;
>> + u32 flags_offset;
>> + void *skb_buf;
>> + u32 data_len;
>> + u32 flags;
>> +
>> + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
>> + length_offset = offsetof(struct mctp_pcc_hdr, length);
>> + data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
>> + MCTP_HEADER_LENGTH;
>> +
>> + 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;
> Please implement ndo_get_stats64, use of the core dev stats in drivers
> is deprecated:
>
> * @stats: Statistics struct, which was left as a legacy, use
> * rtnl_link_stats64 instead
Thanks. Was not aware. The other MCTP drivers need this as well.
>
>> + 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;
>> + skb->dev = mctp_pcc_dev->mdev.dev;
> netdev_alloc_skb() already sets dev
>
>> + netif_rx(skb);
>> +
>> + flags_offset = offsetof(struct mctp_pcc_hdr, flags);
>> + flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
>> + mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 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;
>> + int rc;
>> +
>> + netif_stop_queue(ndev);
> Why?
I saw this in other network drivers, both Ethernet and MCTP. As I
understand it, without stopping the queue, we could take another packet
before this one is sent and we only have one buffer; that said, it
should be protected by the spin lock.
I am tempted to leave this in here, but move all of the statistics into
the spin lock. Is there a reason to not stop the queue?
>
>> + ndev->stats.tx_bytes += skb->len;
>> + ndev->stats.tx_packets++;
>> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
>> +
>> + spin_lock_irqsave(&mpnd->lock, flags);
>> + buffer = mpnd->pcc_comm_outbox_addr;
>> + pcc_header.signature = PCC_MAGIC;
>> + pcc_header.flags = 0x1;
>> + 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);
>> + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
>> + NULL);
>> + spin_unlock_irqrestore(&mpnd->lock, flags);
>> +
>> + dev_consume_skb_any(skb);
>> + netif_start_queue(ndev);
>> + if (!rc)
>> + return NETDEV_TX_OK;
>> + return NETDEV_TX_BUSY;
>> +}
>> +
>> +static const struct net_device_ops mctp_pcc_netdev_ops = {
>> + .ndo_start_xmit = mctp_pcc_tx,
>> + .ndo_uninit = NULL
> No need to init things to NULL
>
>> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
>> +{
>> + struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
>> + struct list_head *ptr;
>> + struct list_head *tmp;
>> +
>> + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
>> + struct net_device *ndev;
>> +
>> + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
>> + if (adev && mctp_pcc_dev->acpi_device == adev)
>> + continue;
>> +
>> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
>> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
>> + ndev = mctp_pcc_dev->mdev.dev;
>> + if (ndev)
>> + mctp_unregister_netdev(ndev);
>> + list_del(ptr);
>> + if (adev)
>> + break;
>> + }
>> +};
> spurious ;
>
>
>> + .owner = THIS_MODULE,
>> +
> suprious new line
>
>> +};
>> +
Hi Jakub, > > +#include <net/pkt_sched.h> > > Hm, what do you need this include for? I've used this in the past for DEFAULT_TX_QUEUE_LEN in existing mctp drivers, but looks like setting tx_queue_len = 0 will do the right thing... Cheers, Jeremy
© 2016 - 2026 Red Hat, Inc.