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

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Adam,

All good with the hw addressing changes, but there are still things from
my previous review that have either been ignored or discarded. In case
of the latter, that still may be fine, but at least a note that you have
done so would be helpful.

Those inline again, and one new one ("Implementation [...]").

> +config MCTP_TRANSPORT_PCC
> +       tristate "MCTP PCC transport"
> +       select ACPI
> +       help
> +         Provides a driver to access MCTP devices over PCC transport,
> +         A MCTP protocol network device is created via ACPI for each
> +         entry in the DST/SDST that matches the identifier. The Platform
> +         commuinucation channels are selected from the corresponding

typo: communication

> +         entries in the PCCT.
> +
> +         Say y here if you need to connect to MCTP endpoints over PCC. To
> +         compile as a module, use m; the module will be called mctp-pcc.
> +
>  endmenu
>  
>  endif
> diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
> index e1cb99ced54a..492a9e47638f 100644
> --- a/drivers/net/mctp/Makefile
> +++ b/drivers/net/mctp/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o
>  obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
>  obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
>  obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
> new file mode 100644
> index 000000000000..489f42849a24
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mctp-pcc.c - Driver for MCTP over PCC.
> + * Copyright (c) 2024, Ampere Computing LLC
> + */
> +
> +/* Implelmentation of MCTP over PCC DMTF Specification 256

"Implementation"

(also, might be better to use the full spec ID ("DSP0256"), as it's
easier to search)

> +struct mctp_pcc_hdr {
> +       u32 signature;
> +       u32 flags;
> +       u32 length;
> +       char mctp_signature[MCTP_SIGNATURE_LENGTH];
> +};
>

These signature/flags/length still don't have the endian annotations
(nor conversions on access). This was raised on v2, but looks like that
got lost?

> +static void
> +mctp_pcc_net_stats(struct net_device *net_dev,
> +                  struct rtnl_link_stats64 *stats)
> +{
> +       stats->rx_errors = 0;
> +       stats->rx_packets = net_dev->stats.rx_packets;
> +       stats->tx_packets = net_dev->stats.tx_packets;
> +       stats->rx_dropped = 0;
> +       stats->tx_bytes = net_dev->stats.tx_bytes;
> +       stats->rx_bytes = net_dev->stats.rx_bytes;
> +}

Is this missing the rx_dropped stat (which you're updating in
_rx_callback)?

If you like, there are some new tstats helpers available, meaning you
wouldn't need the ndo_get_stats64 op at all. Let me know if you're
interested in using those, and would like a hand doing so.

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

Super minor: double space before the %s here.

> +       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");

+ trailing newline (on the error message).

Other than that, all good!

Cheers,


Jeremy