| 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: <Zz-AvBwUgNzMJb7-@LQ3V64L9R2>
Date: Thu, 21 Nov 2024 10:49:32 -0800
From: Joe Damato <jdamato@...tly.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>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH v8 2/2] mctp pcc: Implement MCTP over PCC Transport
FWIW, net-next is currently closed so this would have to be resent
once it has reopened:
https://lore.kernel.org/netdev/20241118071654.695bb1a2@kernel.org/
I don't know much about MCTP, so my apologies that my review is
mostly little nits and a question/comment about stats below.
I don't think any of these are worth holding this back, but since
net-next is closed this needs to be resent, maybe worth considering?
On Wed, Nov 20, 2024 at 02:02:15PM -0500, 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 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>
> ---
> drivers/net/mctp/Kconfig | 13 ++
> drivers/net/mctp/Makefile | 1 +
> drivers/net/mctp/mctp-pcc.c | 321 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 335 insertions(+)
> create mode 100644 drivers/net/mctp/mctp-pcc.c
[...]
It seems like below there are a few places where unnecessary double
spaces are included. Not necessarily a reason to hold this back, but
since net-next is closed and you need to resend anyway...
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,321 @@
[...]
> +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;
> +
> + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox.client);
> + memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->inbox.chan->shmem,
> + sizeof(struct mctp_pcc_hdr));
> + data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
> +
> + if (data_len > mctp_pcc_dev->mdev.dev->mtu) {
> + mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
I'm not an expert on rtnl stats, but maybe this should be
accounted for as rx_length_errors ?
And when rx_dropped is accounted in the stats callback it can add
rx_length_errors in as well as setting rtnl_link_stats64's
rx_length_errors?
You've probably read this already, but just in case:
https://docs.kernel.org/networking/statistics.html#struct-rtnl-link-stats64
> + 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->inbox.chan->shmem, 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);
> +}
> +
> +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;
Extra space after mctp_pcc_hdr ?
[...]
> +
> +static void mctp_pcc_setup(struct net_device *ndev)
Extra space after void?
[...]
> +
> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
> + void *context)
> +{
> + struct mctp_pcc_lookup_context *luc = context;
extra space after struct ?
[...]
> +
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> + struct mctp_pcc_lookup_context context = {0, 0, 0};
> + 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
I'm not sure but in net/ the general comment style seems to be /*
*/, I grepped around a bit and didn't notice many comments of this
style (except SPDX lines).
Maybe this should be a /* */ ?
> + snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
> + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
> + mctp_pcc_setup);
> + if (!ndev)
> + return -ENOMEM;
> +
> + mctp_pcc_ndev = netdev_priv(ndev);
> + rc = devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
extra space after = ?
> + if (rc)
> + goto cleanup_netdev;
> + spin_lock_init(&mctp_pcc_ndev->lock);
> +
> + rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
> + context.inbox_index);
> + if (rc)
> + goto cleanup_netdev;
> + mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
> +
> + //outbox initialization
Same as above on comment style
Powered by blists - more mailing lists