[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250715140805.GA721198@horms.kernel.org>
Date: Tue, 15 Jul 2025 15:08:05 +0100
From: Simon Horman <horms@...nel.org>
To: admiyo@...amperecomputing.com
Cc: Jeremy Kerr <jk@...econstruct.com.au>,
Matt Johnston <matt@...econstruct.com.au>,
Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Sudeep Holla <sudeep.holla@....com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH v23 2/2] mctp pcc: Implement MCTP over PCC Transport
On Mon, Jul 14, 2025 at 08:10:08PM -0400, admiyo@...amperecomputing.com wrote:
> From: Adam Young <admiyo@...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@...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
Powered by blists - more mailing lists