Re: [PATCH v23 2/2] mctp pcc: Implement MCTP over PCC Transport

[Simon Horman <horms@kernel.org>]

On Mon, Jul 14, 2025 at 08:10:08PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Implementation of network driver for
> Management Control Transport Protocol(MCTP)
> over Platform Communication Channel(PCC)
> 
> DMTF DSP:0292
> https://www.dmtf.org/sites/default/files/standards/documents/\
> DSP0292_1.0.0WIP50.pdf
> 
> MCTP devices are specified 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 <admiyo@os.amperecomputing.com>

...

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c

...

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