| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250424105752.00007396@huawei.com>
Date: Thu, 24 Apr 2025 10:57:52 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
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>, Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH net-next v20 1/1] mctp pcc: Implement MCTP over PCC
Transport
On Wed, 23 Apr 2025 18:01:42 -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.
>
> Communication with other devices use the PCC based
> doorbell mechanism.
>
> Signed-off-by: Adam Young <admiyo@...amperecomputing.com>
Hi Adam,
Sorry it's been a while since I last looked at this.
Anyhow, some fairly general feedback inline. All minor stuff.
With that tidied up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> ---
> MAINTAINERS | 5 +
> drivers/net/mctp/Kconfig | 13 ++
> drivers/net/mctp/Makefile | 1 +
> drivers/net/mctp/mctp-pcc.c | 317 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 336 insertions(+)
> create mode 100644 drivers/net/mctp/mctp-pcc.c
> new file mode 100644
> index 000000000000..589ba4387ce6
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,317 @@
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> + struct mctp_pcc_hdr *mctp_pcc_header;
> + void __iomem *buffer;
> + unsigned long flags;
> + int len = skb->len;
> + int rc;
> +
> + rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
> + if (rc)
> + goto err_drop;
> +
> + mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
I'd use sizeof(*mctp_pcc_header) for these to avoid the reader needing to check
types. There are a bunch of these you could do the same with to slightly
improve readability.
> + mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | mpnd->outbox.index);
> + mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
> + memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
> + MCTP_SIGNATURE_LENGTH);
> + mctp_pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
> +
> + spin_lock_irqsave(&mpnd->lock, flags);
> + buffer = mpnd->outbox.chan->shmem;
> + memcpy_toio(buffer, skb->data, skb->len);
> + mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
> + NULL);
Slightly odd alignment. One extra space?
> + spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> + dev_dstats_tx_add(ndev, len);
> + dev_consume_skb_any(skb);
> + return NETDEV_TX_OK;
> +err_drop:
> + dev_dstats_tx_dropped(ndev);
> + kfree_skb(skb);
> + return NETDEV_TX_OK;
> +}
> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
> + void *context)
> +{
> + struct mctp_pcc_lookup_context *luc = context;
> + struct acpi_resource_address32 *addr;
> +
> + switch (ares->type) {
> + case PCC_DWORD_TYPE:
If unlikely we will ever care about other types could simpilfy to
if (ares->type != PCC_DWORD_TYPE)
return AE_OK;
etc
> + 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_initialize_mailbox(struct device *dev,
> + struct mctp_pcc_mailbox *box, u32 index)
> +{
> + int ret;
> +
> + box->index = index;
> + box->chan = pcc_mbox_request_channel(&box->client, index);
as below
box->client.dev = dev;
> + if (IS_ERR(box->chan))
> + return PTR_ERR(box->chan);
I'd put a blank line here...
> + ret = devm_add_action_or_reset(dev, mctp_cleanup_channel, box->chan);
> + if (ret)
> + return -EINVAL;
And here.
Why eat ret? Which incidentally is normally -ENOMEM for these.
Why not the simpler
return devm_add_action_or_reset(dev, mctp_cleanup_channel, box->chan);
> + return 0;
> +}
> +
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> + struct mctp_pcc_lookup_context context = {0, 0, 0};
I'd be tempted to use simply {} or { 0 } so that if we have
extra context in future we aren't somehow implying it should not be
intialized to zero (as it will happen anyway).
> + struct mctp_pcc_ndev *mctp_pcc_ndev;
> + struct device *dev = &acpi_dev->dev;
> + struct net_device *ndev;
> + acpi_handle dev_handle;
> + acpi_status status;
> + int mctp_pcc_mtu;
> + char name[32];
> + int rc;
> +
> + dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
> + acpi_device_hid(acpi_dev));
> + dev_handle = acpi_device_handle(acpi_dev);
> + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
> + &context);
> + if (!ACPI_SUCCESS(status)) {
> + dev_err(dev, "FAILURE to lookup PCC indexes from CRS\n");
> + return -EINVAL;
> + }
> +
> + /* inbox initialization */
> + snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
> + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
> + mctp_pcc_setup);
I'd use sizeof(*mctp_pcc_ndev) so we don't have to bother checking types...
> + if (!ndev)
> + return -ENOMEM;
> +
> + mctp_pcc_ndev = netdev_priv(ndev);
> + spin_lock_init(&mctp_pcc_ndev->lock);
> +
> + rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
> + context.inbox_index);
I think this should be responsible for all the setup of inbox. So that
includes setting inbox.client.dev as set below.
> + if (rc)
> + goto free_netdev;
> + mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
> +
> + /* outbox initialization */
> + rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
> + context.outbox_index);
Same as for inbox.
> + if (rc)
> + goto free_netdev;
> +
> + mctp_pcc_ndev->acpi_device = acpi_dev;
> + mctp_pcc_ndev->inbox.client.dev = dev;
> + mctp_pcc_ndev->outbox.client.dev = dev;
As above. I think these should be part of the initialize_mailbox calls
given they are part of the mailboxes and we are passing in dev anyway.
> + mctp_pcc_ndev->mdev.dev = ndev;
> + acpi_dev->driver_data = mctp_pcc_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_ndev->outbox.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;
> +
> + /* 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);
> + if (rc)
> + goto free_netdev;
> + return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
> +free_netdev:
> + free_netdev(ndev);
> + return rc;
> +}
> +
> +static const struct acpi_device_id mctp_pcc_device_ids[] = {
> + { "DMT0001"},
Bracket before } for consistency of spacing.
> + {}
> +};
> +
> +static struct acpi_driver mctp_pcc_driver = {
> + .name = "mctp_pcc",
> + .class = "Unknown",
> + .ids = mctp_pcc_device_ids,
> + .ops = {
> + .add = mctp_pcc_driver_add,
> + },
> +};
> +
> +module_acpi_driver(mctp_pcc_driver);
> +
> +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
> +
> +MODULE_DESCRIPTION("MCTP PCC ACPI device");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Adam Young <admiyo@...amperecomputing.com>");
Powered by blists - more mailing lists