Re: [PATCH v7 2/2] mctp pcc: Implement MCTP over PCC Transport
flat view
From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: 2024-11-15 07:27:56
Also in:
lkml
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 [...]").
quoted hunk ↗ jump to hunk
+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
quoted hunk ↗ jump to hunk
+ 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 endifdiff --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.oobj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.odiff --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)
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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