On Mon, Jul 14, 2025 at 08:10:08PM -0400, admiyo@os.amperecomputing.com wrote:

From: Adam Young <redacted>

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

DMTF DSP:0292
https://www.dmtf.org/sites/default/files/standards/documents/\
DSP0292_1.0.0WIP50.pdf

MCTP devices are specified via ACPI by entries
in DSDT/SDST and reference channels specified
in the PCCT.  Messages are sent on a type 3 and
received on a type 4 channel.  Communication with
other devices use the PCC based doorbell mechanism;
a shared memory segment with a corresponding
interrupt and a memory register used to trigger
remote interrupts.

This driver takes advantage of PCC mailbox buffer
management. The data section of the struct sk_buff
that contains the outgoing packet is sent to the mailbox,
already properly formatted  as a PCC message.  The driver
is also reponsible for allocating a struct sk_buff that

responsible

is then passed to the mailbox and used to record the
data in the shared buffer. It maintains a list of both
outging and incoming sk_buffs to match the data buffers
with the original sk_buffs.

When the Type 3 channel outbox receives a txdone response
interrupt, it consumes the outgoing sk_buff, allowing
it to be freed.

Signed-off-by: Adam Young <redacted>

...

...

+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
+	struct pcc_header *pcc_header;
+	int len = skb->len;
+	int rc;
+
+	rc = skb_cow_head(skb, sizeof(*pcc_header));
+	if (rc) {
+		dev_dstats_tx_dropped(ndev);
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+
+	pcc_header = skb_push(skb, sizeof(*pcc_header));
+	pcc_header->signature = cpu_to_le32(PCC_SIGNATURE | mpnd->outbox.index);
+	pcc_header->flags = cpu_to_le32(PCC_CMD_COMPLETION_NOTIFY);
+	memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
+	pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);

Hi Adam,

There seems to be an endian missmatch here: host order values
are being converted to little endian values and then
assigned to host endian variables.

Sparse says:

  .../mctp-pcc.c:146:31: warning: incorrect type in assignment (different base types)
  .../mctp-pcc.c:146:31:    expected unsigned int [usertype] signature
  .../mctp-pcc.c:146:31:    got restricted __le32 [usertype]
  .../mctp-pcc.c:147:27: warning: incorrect type in assignment (different base types)
  .../mctp-pcc.c:147:27:    expected unsigned int [usertype] flags
  .../mctp-pcc.c:147:27:    got restricted __le32 [usertype]
  .../mctp-pcc.c:149:28: warning: incorrect type in assignment (different base types)
  .../mctp-pcc.c:149:28:    expected unsigned int [usertype] length
  .../mctp-pcc.c:149:28:    got restricted __le32 [usertype]

+	skb_queue_head(&mpnd->outbox.packets, skb);
+
+	rc = mbox_send_message(mpnd->outbox.chan->mchan, skb->data);
+
+	if (rc < 0) {
+		skb_unlink(skb, &mpnd->outbox.packets);
+		return NETDEV_TX_BUSY;
+	}
+
+	dev_dstats_tx_add(ndev, len);
+	return NETDEV_TX_OK;
+}

...

+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
+{

...

+	/* ndev needs to be freed before the iomemory (mapped above) gets
+	 * unmapped,  devm resources get freed in reverse to the order they
+	 * are added.
+	 */
+	rc = mctp_register_netdev(ndev, &mctp_netdev_ops, MCTP_PHYS_BINDING_PCC);

nit: please line wrap to 80 columbs wide or less:

	rc = mctp_register_netdev(ndev, &mctp_netdev_ops,
				  MCTP_PHYS_BINDING_PCC);

+	if (rc)
+		goto free_netdev;
+
+	return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
+free_netdev:
+	free_netdev(ndev);
+	return rc;
+}

...

pw-bot: changes-requested