Message ID | 20230119113140.20208-2-alejandro.lucero-palau@amd.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | sfc: devlink support for ef100 | expand |
Thu, Jan 19, 2023 at 12:31:34PM CET, alejandro.lucero-palau@amd.com wrote: >From: Alejandro Lucero <alejandro.lucero-palau@amd.com> > >Basic support for devlink info command. > >Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >--- > drivers/net/ethernet/sfc/Kconfig | 1 + > drivers/net/ethernet/sfc/Makefile | 3 +- > drivers/net/ethernet/sfc/ef100_nic.c | 6 + > drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ > drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ > drivers/net/ethernet/sfc/mcdi.c | 72 +++++ > drivers/net/ethernet/sfc/mcdi.h | 3 + > drivers/net/ethernet/sfc/net_driver.h | 2 + > 8 files changed, 533 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c > create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h Could you please split? At least the devlink introduction part and info implementation. > >diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig >index 0950e6b0508f..4af36ba8906b 100644 >--- a/drivers/net/ethernet/sfc/Kconfig >+++ b/drivers/net/ethernet/sfc/Kconfig >@@ -22,6 +22,7 @@ config SFC > depends on PTP_1588_CLOCK_OPTIONAL > select MDIO > select CRC32 >+ select NET_DEVLINK > help > This driver supports 10/40-gigabit Ethernet cards based on > the Solarflare SFC9100-family controllers. >diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >index 712a48d00069..55b9c73cd8ef 100644 >--- a/drivers/net/ethernet/sfc/Makefile >+++ b/drivers/net/ethernet/sfc/Makefile >@@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ > mcdi.o mcdi_port.o mcdi_port_common.o \ > mcdi_functions.o mcdi_filters.o mcdi_mon.o \ > ef100.o ef100_nic.o ef100_netdev.o \ >- ef100_ethtool.o ef100_rx.o ef100_tx.o >+ ef100_ethtool.o ef100_rx.o ef100_tx.o \ >+ efx_devlink.o > sfc-$(CONFIG_SFC_MTD) += mtd.o > sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ > mae.o tc.o tc_bindings.o tc_counters.o >diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >index ad686c671ab8..14af8f314b83 100644 >--- a/drivers/net/ethernet/sfc/ef100_nic.c >+++ b/drivers/net/ethernet/sfc/ef100_nic.c >@@ -27,6 +27,7 @@ > #include "tc.h" > #include "mae.h" > #include "rx_common.h" >+#include "efx_devlink.h" > > #define EF100_MAX_VIS 4096 > #define EF100_NUM_MCDI_BUFFERS 1 >@@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) > netif_warn(efx, probe, net_dev, > "Failed to probe base mport rc %d; representors will not function\n", > rc); >+ } else { >+ if (efx_probe_devlink(efx)) I don't understand why the devlink register call is here. 1) You can registers it for PF and VF. I don't follow why you do it only for PF. 2) It should be done as the first thing in the probe flow. Then devlink port register, only after that netdev. It makes sense from the perspective of object hierarchy. It is also usual to have the devlink priv as the "main" driver structure storage, see how that is done in mlx5 of mlxsw for example. >+ netif_warn(efx, probe, net_dev, >+ "Failed to register devlink\n"); > } > > rc = efx_init_tc(efx); >@@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) > { > struct ef100_nic_data *nic_data = efx->nic_data; > >+ efx_fini_devlink(efx); > efx_mcdi_detach(efx); > efx_mcdi_fini(efx); > if (nic_data) >diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >new file mode 100644 >index 000000000000..c506f8f35d25 >--- /dev/null >+++ b/drivers/net/ethernet/sfc/efx_devlink.c >@@ -0,0 +1,427 @@ >+// SPDX-License-Identifier: GPL-2.0-only >+/**************************************************************************** >+ * Driver for AMD network controllers and boards >+ * Copyright (C) 2023, Advanced Micro Devices, Inc. >+ * >+ * This program is free software; you can redistribute it and/or modify it >+ * under the terms of the GNU General Public License version 2 as published >+ * by the Free Software Foundation, incorporated herein by reference. >+ */ >+ >+#include <linux/rtc.h> >+#include "net_driver.h" >+#include "ef100_nic.h" >+#include "efx_devlink.h" >+#include "nic.h" >+#include "mcdi.h" >+#include "mcdi_functions.h" >+#include "mcdi_pcol.h" >+ >+/* Custom devlink-info version object names for details that do not map to the >+ * generic standardized names. >+ */ >+#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >+#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >+#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >+#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >+#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >+#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >+#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >+#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >+#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >+#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >+#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" >+ >+#define EFX_MAX_VERSION_INFO_LEN 64 >+ >+static int efx_devlink_info_nvram_partition(struct efx_nic *efx, >+ struct devlink_info_req *req, >+ unsigned int partition_type, >+ const char *version_name) >+{ >+ char buf[EFX_MAX_VERSION_INFO_LEN]; >+ u16 version[4]; >+ int rc; >+ >+ rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, >+ 0); >+ if (rc) >+ return rc; >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], >+ version[1], version[2], version[3]); >+ devlink_info_version_stored_put(req, version_name, buf); >+ >+ return 0; >+} >+ >+static void efx_devlink_info_stored_versions(struct efx_nic *efx, >+ struct devlink_info_req *req) >+{ >+ efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, >+ DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); >+ efx_devlink_info_nvram_partition(efx, req, >+ NVRAM_PARTITION_TYPE_MC_FIRMWARE, >+ DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); >+ efx_devlink_info_nvram_partition(efx, req, >+ NVRAM_PARTITION_TYPE_SUC_FIRMWARE, >+ EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); >+ efx_devlink_info_nvram_partition(efx, req, >+ NVRAM_PARTITION_TYPE_EXPANSION_ROM, >+ EFX_DEVLINK_INFO_VERSION_FW_EXPROM); >+ efx_devlink_info_nvram_partition(efx, req, >+ NVRAM_PARTITION_TYPE_EXPANSION_UEFI, >+ EFX_DEVLINK_INFO_VERSION_FW_UEFI); >+} >+ >+#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) >+ >+static void efx_devlink_info_board_cfg(struct efx_nic *efx, >+ struct devlink_info_req *req) >+{ >+ char sn[EFX_MAX_SERIALNUM_LEN]; >+ u8 mac_address[ETH_ALEN]; >+ int rc; >+ >+ rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); >+ if (!rc) { >+ snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); >+ devlink_info_serial_number_put(req, sn); >+ } >+} >+ >+#define EFX_VER_FLAG(_f) \ >+ (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) >+ >+static void efx_devlink_info_running_versions(struct efx_nic *efx, >+ struct devlink_info_req *req) >+{ >+ MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); >+ MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); >+ char buf[EFX_MAX_VERSION_INFO_LEN]; >+ unsigned int flags, build_id; >+ union { >+ const __le32 *dwords; >+ const __le16 *words; >+ const char *str; >+ } ver; >+ struct rtc_time build_date; >+ size_t outlength, offset; >+ u64 tstamp; >+ int rc; >+ >+ rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), >+ outbuf, sizeof(outbuf), &outlength); >+ if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) >+ return; >+ >+ /* Handle previous output */ >+ if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { >+ ver.words = (__le16 *)MCDI_PTR(outbuf, >+ GET_VERSION_EXT_OUT_VERSION); >+ offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le16_to_cpu(ver.words[0]), >+ le16_to_cpu(ver.words[1]), >+ le16_to_cpu(ver.words[2]), >+ le16_to_cpu(ver.words[3])); >+ >+ devlink_info_version_running_put(req, >+ DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >+ buf); >+ return; >+ } >+ >+ /* Handle V2 additions */ >+ flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); >+ if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", >+ MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); >+ devlink_info_version_fixed_put(req, >+ DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, >+ buf); >+ >+ /* Favour full board version if present (in V5 or later) */ >+ if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", >+ MCDI_DWORD(outbuf, >+ GET_VERSION_V2_OUT_BOARD_REVISION)); >+ devlink_info_version_fixed_put(req, >+ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >+ buf); >+ } >+ >+ ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); >+ if (ver.str[0]) >+ devlink_info_board_serial_number_put(req, ver.str); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V2_OUT_FPGA_VERSION); >+ offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", >+ le32_to_cpu(ver.dwords[0]), >+ 'A' + le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2])); >+ >+ ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); >+ if (ver.str[0]) >+ snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >+ " (%s)", ver.str); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_FPGA_REV, >+ buf); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V2_OUT_CMCFW_VERSION); >+ offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), >+ le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ tstamp = MCDI_QWORD(outbuf, >+ GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); >+ if (tstamp) { >+ rtc_time64_to_tm(tstamp, &build_date); >+ snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >+ " (%ptRd)", &build_date); >+ } >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, >+ buf); >+ } >+ >+ ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); >+ offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), >+ le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); >+ if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { >+ build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); >+ snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >+ " (%x) %s", build_id, >+ MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); >+ } >+ devlink_info_version_running_put(req, >+ DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >+ buf); >+ >+ if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V2_OUT_SUCFW_VERSION); >+ tstamp = MCDI_QWORD(outbuf, >+ GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); >+ rtc_time64_to_tm(tstamp, &build_date); >+ build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, >+ "%u.%u.%u.%u type %x (%ptRd)", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), >+ build_id, &build_date); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >+ buf); >+ } >+ >+ if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) >+ return; >+ >+ /* Handle V3 additions */ >+ if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2])); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, >+ buf); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2])); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, >+ buf); >+ } >+ >+ if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) >+ return; >+ >+ /* Handle V4 additions */ >+ if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V4_OUT_SOC_BOOT_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_SOC_BOOT, >+ buf); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, >+ buf); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_SOC_MAIN, >+ buf); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, >+ buf); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && >+ ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V4_OUT_SUCFW_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ devlink_info_version_running_put(req, >+ EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >+ buf); >+ } >+ >+ if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) >+ return; >+ >+ /* Handle V5 additions */ >+ >+ if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V5_OUT_BOARD_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ devlink_info_version_running_put(req, >+ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >+ buf); >+ } >+ >+ if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { >+ ver.dwords = (__le32 *)MCDI_PTR(outbuf, >+ GET_VERSION_V5_OUT_BUNDLE_VERSION); >+ >+ snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >+ le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >+ le32_to_cpu(ver.dwords[2]), >+ le32_to_cpu(ver.dwords[3])); >+ >+ devlink_info_version_running_put(req, >+ DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, >+ buf); >+ } >+} >+ >+#undef EFX_VER_FLAG >+ >+static void efx_devlink_info_query_all(struct efx_nic *efx, >+ struct devlink_info_req *req) >+{ >+ efx_devlink_info_board_cfg(efx, req); >+ efx_devlink_info_stored_versions(efx, req); >+ efx_devlink_info_running_versions(efx, req); >+} >+ >+struct efx_devlink { >+ struct efx_nic *efx; >+}; >+ >+static int efx_devlink_info_get(struct devlink *devlink, >+ struct devlink_info_req *req, >+ struct netlink_ext_ack *extack) >+{ >+ struct efx_devlink *devlink_private = devlink_priv(devlink); >+ struct efx_nic *efx = devlink_private->efx; >+ >+ efx_devlink_info_query_all(efx, req); I don't understand the reason for having efx_devlink_info_query_all() as a separate function in compare to inline its code here. >+ return 0; >+} >+ >+static const struct devlink_ops sfc_devlink_ops = { >+ .info_get = efx_devlink_info_get, >+}; >+ >+void efx_fini_devlink(struct efx_nic *efx) >+{ >+ if (efx->devlink) { >+ struct efx_devlink *devlink_private; >+ >+ devlink_private = devlink_priv(efx->devlink); >+ >+ devlink_unregister(efx->devlink); >+ devlink_free(efx->devlink); >+ efx->devlink = NULL; >+ } >+} >+ >+int efx_probe_devlink(struct efx_nic *efx) >+{ >+ struct efx_devlink *devlink_private; >+ >+ efx->devlink = devlink_alloc(&sfc_devlink_ops, >+ sizeof(struct efx_devlink), >+ &efx->pci_dev->dev); >+ if (!efx->devlink) >+ return -ENOMEM; >+ devlink_private = devlink_priv(efx->devlink); >+ devlink_private->efx = efx; >+ >+ devlink_register(efx->devlink); >+ >+ return 0; >+} >diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h >new file mode 100644 >index 000000000000..997f878aea93 >--- /dev/null >+++ b/drivers/net/ethernet/sfc/efx_devlink.h >@@ -0,0 +1,20 @@ >+/* SPDX-License-Identifier: GPL-2.0-only */ >+/**************************************************************************** >+ * Driver for AMD network controllers and boards >+ * Copyright (C) 2023, Advanced Micro Devices, Inc. >+ * >+ * This program is free software; you can redistribute it and/or modify it >+ * under the terms of the GNU General Public License version 2 as published >+ * by the Free Software Foundation, incorporated herein by reference. >+ */ >+ >+#ifndef _EFX_DEVLINK_H >+#define _EFX_DEVLINK_H >+ >+#include "net_driver.h" >+#include <net/devlink.h> >+ >+int efx_probe_devlink(struct efx_nic *efx); >+void efx_fini_devlink(struct efx_nic *efx); >+ >+#endif /* _EFX_DEVLINK_H */ >diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c >index af338208eae9..328cae82a7d8 100644 >--- a/drivers/net/ethernet/sfc/mcdi.c >+++ b/drivers/net/ethernet/sfc/mcdi.c >@@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) > return rc; > } > >+int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >+ u32 *subtype, u16 version[4], char *desc, >+ size_t descsize) >+{ >+ MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); >+ efx_dword_t *outbuf; >+ size_t outlen; >+ u32 flags; >+ int rc; >+ >+ outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); >+ if (!outbuf) >+ return -ENOMEM; >+ >+ MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); >+ >+ rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, >+ sizeof(inbuf), outbuf, >+ MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, >+ &outlen); >+ if (rc) >+ goto out_free; >+ if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { >+ rc = -EIO; >+ goto out_free; >+ } >+ >+ flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); >+ >+ if (desc && descsize > 0) { >+ if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { >+ if (descsize <= >+ MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { >+ rc = -E2BIG; >+ goto out_free; >+ } >+ >+ strncpy(desc, >+ MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), >+ MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); >+ desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; >+ } else { >+ desc[0] = '\0'; >+ } >+ } >+ >+ if (subtype) { >+ if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) >+ *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); >+ else >+ *subtype = 0; >+ } >+ >+ if (version) { >+ if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { >+ version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); >+ version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); >+ version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); >+ version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); >+ } else { >+ version[0] = 0; >+ version[1] = 0; >+ version[2] = 0; >+ version[3] = 0; >+ } >+ } >+ >+out_free: >+ kfree(outbuf); >+ return rc; >+} >+ > int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, > size_t len, size_t *retlen, u8 *buffer) > { >diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h >index 7e35fec9da35..63b090587f7a 100644 >--- a/drivers/net/ethernet/sfc/mcdi.h >+++ b/drivers/net/ethernet/sfc/mcdi.h >@@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, > bool *protected_out); > int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); > int efx_mcdi_nvram_test_all(struct efx_nic *efx); >+int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >+ u32 *subtype, u16 version[4], char *desc, >+ size_t descsize); > int efx_mcdi_handle_assertion(struct efx_nic *efx); > int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); > int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, >diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >index 3b49e216768b..d036641dc043 100644 >--- a/drivers/net/ethernet/sfc/net_driver.h >+++ b/drivers/net/ethernet/sfc/net_driver.h >@@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { > * xdp_rxq_info structures? > * @netdev_notifier: Netdevice notifier. > * @tc: state for TC offload (EF100). >+ * @devlink: reference to devlink structure owned by this device > * @mem_bar: The BAR that is mapped into membase. > * @reg_base: Offset from the start of the bar to the function control window. > * @monitor_work: Hardware monitor workitem >@@ -1179,6 +1180,7 @@ struct efx_nic { > struct notifier_block netdev_notifier; > struct efx_tc_state *tc; > >+ struct devlink *devlink; > unsigned int mem_bar; > u32 reg_base; > >-- >2.17.1 >
On 1/19/23 12:14, Jiri Pirko wrote: > Thu, Jan 19, 2023 at 12:31:34PM CET, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> >> Basic support for devlink info command. >> >> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> --- >> drivers/net/ethernet/sfc/Kconfig | 1 + >> drivers/net/ethernet/sfc/Makefile | 3 +- >> drivers/net/ethernet/sfc/ef100_nic.c | 6 + >> drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ >> drivers/net/ethernet/sfc/mcdi.c | 72 +++++ >> drivers/net/ethernet/sfc/mcdi.h | 3 + >> drivers/net/ethernet/sfc/net_driver.h | 2 + >> 8 files changed, 533 insertions(+), 1 deletion(-) >> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c >> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h > Could you please split? > > At least the devlink introduction part and info implementation. > Sure. >> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig >> index 0950e6b0508f..4af36ba8906b 100644 >> --- a/drivers/net/ethernet/sfc/Kconfig >> +++ b/drivers/net/ethernet/sfc/Kconfig >> @@ -22,6 +22,7 @@ config SFC >> depends on PTP_1588_CLOCK_OPTIONAL >> select MDIO >> select CRC32 >> + select NET_DEVLINK >> help >> This driver supports 10/40-gigabit Ethernet cards based on >> the Solarflare SFC9100-family controllers. >> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >> index 712a48d00069..55b9c73cd8ef 100644 >> --- a/drivers/net/ethernet/sfc/Makefile >> +++ b/drivers/net/ethernet/sfc/Makefile >> @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ >> mcdi.o mcdi_port.o mcdi_port_common.o \ >> mcdi_functions.o mcdi_filters.o mcdi_mon.o \ >> ef100.o ef100_nic.o ef100_netdev.o \ >> - ef100_ethtool.o ef100_rx.o ef100_tx.o >> + ef100_ethtool.o ef100_rx.o ef100_tx.o \ >> + efx_devlink.o >> sfc-$(CONFIG_SFC_MTD) += mtd.o >> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ >> mae.o tc.o tc_bindings.o tc_counters.o >> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >> index ad686c671ab8..14af8f314b83 100644 >> --- a/drivers/net/ethernet/sfc/ef100_nic.c >> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >> @@ -27,6 +27,7 @@ >> #include "tc.h" >> #include "mae.h" >> #include "rx_common.h" >> +#include "efx_devlink.h" >> >> #define EF100_MAX_VIS 4096 >> #define EF100_NUM_MCDI_BUFFERS 1 >> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >> netif_warn(efx, probe, net_dev, >> "Failed to probe base mport rc %d; representors will not function\n", >> rc); >> + } else { >> + if (efx_probe_devlink(efx)) > I don't understand why the devlink register call is here. > 1) You can registers it for PF and VF. I don't follow why you do it only > for PF. We discuss the possibility of creating the devlink interface for VFs, but we decided not to. Arguably, this should not be available for VFs owned by VMs/containers, or at least in our case, since the interface is being used for getting information about the whole device or for getting/setting the MAC address. We plan to support more devlink functionalities in the near future, so maybe we add such support then if we consider it is needed. > 2) It should be done as the first thing in the probe flow. Then devlink > port register, only after that netdev. It makes sense from the > perspective of object hierarchy. I can not see a problem here. It is not the first thing done in the probe function, but I can not see any dependency for being so. And it is done before the devlink port registration and before the netdev registration what seems to be what other drivers do. What am I missing here? > > It is also usual to have the devlink priv as the "main" driver > structure storage, see how that is done in mlx5 of mlxsw for example. I will check the way others drivers use the devlink priv. > > >> + netif_warn(efx, probe, net_dev, >> + "Failed to register devlink\n"); >> } >> >> rc = efx_init_tc(efx); >> @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) >> { >> struct ef100_nic_data *nic_data = efx->nic_data; >> >> + efx_fini_devlink(efx); >> efx_mcdi_detach(efx); >> efx_mcdi_fini(efx); >> if (nic_data) >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >> new file mode 100644 >> index 000000000000..c506f8f35d25 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >> @@ -0,0 +1,427 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/**************************************************************************** >> + * Driver for AMD network controllers and boards >> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation, incorporated herein by reference. >> + */ >> + >> +#include <linux/rtc.h> >> +#include "net_driver.h" >> +#include "ef100_nic.h" >> +#include "efx_devlink.h" >> +#include "nic.h" >> +#include "mcdi.h" >> +#include "mcdi_functions.h" >> +#include "mcdi_pcol.h" >> + >> +/* Custom devlink-info version object names for details that do not map to the >> + * generic standardized names. >> + */ >> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >> +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >> +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >> +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >> +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" >> + >> +#define EFX_MAX_VERSION_INFO_LEN 64 >> + >> +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, >> + struct devlink_info_req *req, >> + unsigned int partition_type, >> + const char *version_name) >> +{ >> + char buf[EFX_MAX_VERSION_INFO_LEN]; >> + u16 version[4]; >> + int rc; >> + >> + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, >> + 0); >> + if (rc) >> + return rc; >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], >> + version[1], version[2], version[3]); >> + devlink_info_version_stored_put(req, version_name, buf); >> + >> + return 0; >> +} >> + >> +static void efx_devlink_info_stored_versions(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, >> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_MC_FIRMWARE, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_EXPANSION_ROM, >> + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, >> + EFX_DEVLINK_INFO_VERSION_FW_UEFI); >> +} >> + >> +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) >> + >> +static void efx_devlink_info_board_cfg(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + char sn[EFX_MAX_SERIALNUM_LEN]; >> + u8 mac_address[ETH_ALEN]; >> + int rc; >> + >> + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); >> + if (!rc) { >> + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); >> + devlink_info_serial_number_put(req, sn); >> + } >> +} >> + >> +#define EFX_VER_FLAG(_f) \ >> + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) >> + >> +static void efx_devlink_info_running_versions(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); >> + char buf[EFX_MAX_VERSION_INFO_LEN]; >> + unsigned int flags, build_id; >> + union { >> + const __le32 *dwords; >> + const __le16 *words; >> + const char *str; >> + } ver; >> + struct rtc_time build_date; >> + size_t outlength, offset; >> + u64 tstamp; >> + int rc; >> + >> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), >> + outbuf, sizeof(outbuf), &outlength); >> + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) >> + return; >> + >> + /* Handle previous output */ >> + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { >> + ver.words = (__le16 *)MCDI_PTR(outbuf, >> + GET_VERSION_EXT_OUT_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le16_to_cpu(ver.words[0]), >> + le16_to_cpu(ver.words[1]), >> + le16_to_cpu(ver.words[2]), >> + le16_to_cpu(ver.words[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >> + buf); >> + return; >> + } >> + >> + /* Handle V2 additions */ >> + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); >> + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", >> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); >> + devlink_info_version_fixed_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, >> + buf); >> + >> + /* Favour full board version if present (in V5 or later) */ >> + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", >> + MCDI_DWORD(outbuf, >> + GET_VERSION_V2_OUT_BOARD_REVISION)); >> + devlink_info_version_fixed_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >> + buf); >> + } >> + >> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); >> + if (ver.str[0]) >> + devlink_info_board_serial_number_put(req, ver.str); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_FPGA_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", >> + le32_to_cpu(ver.dwords[0]), >> + 'A' + le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); >> + if (ver.str[0]) >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%s)", ver.str); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FPGA_REV, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_CMCFW_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), >> + le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + tstamp = MCDI_QWORD(outbuf, >> + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); >> + if (tstamp) { >> + rtc_time64_to_tm(tstamp, &build_date); >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%ptRd)", &build_date); >> + } >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, >> + buf); >> + } >> + >> + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), >> + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); >> + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { >> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%x) %s", build_id, >> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); >> + } >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >> + buf); >> + >> + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_SUCFW_VERSION); >> + tstamp = MCDI_QWORD(outbuf, >> + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); >> + rtc_time64_to_tm(tstamp, &build_date); >> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, >> + "%u.%u.%u.%u type %x (%ptRd)", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), >> + build_id, &build_date); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) >> + return; >> + >> + /* Handle V3 additions */ >> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) >> + return; >> + >> + /* Handle V4 additions */ >> + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && >> + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SUCFW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) >> + return; >> + >> + /* Handle V5 additions */ >> + >> + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V5_OUT_BOARD_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V5_OUT_BUNDLE_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, >> + buf); >> + } >> +} >> + >> +#undef EFX_VER_FLAG >> + >> +static void efx_devlink_info_query_all(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + efx_devlink_info_board_cfg(efx, req); >> + efx_devlink_info_stored_versions(efx, req); >> + efx_devlink_info_running_versions(efx, req); >> +} >> + >> +struct efx_devlink { >> + struct efx_nic *efx; >> +}; >> + >> +static int efx_devlink_info_get(struct devlink *devlink, >> + struct devlink_info_req *req, >> + struct netlink_ext_ack *extack) >> +{ >> + struct efx_devlink *devlink_private = devlink_priv(devlink); >> + struct efx_nic *efx = devlink_private->efx; >> + >> + efx_devlink_info_query_all(efx, req); > I don't understand the reason for having efx_devlink_info_query_all() as > a separate function in compare to inline its code here. > Yes, it could be do as you say. I'll do. Thanks >> + return 0; >> +} >> + >> +static const struct devlink_ops sfc_devlink_ops = { >> + .info_get = efx_devlink_info_get, >> +}; >> + >> +void efx_fini_devlink(struct efx_nic *efx) >> +{ >> + if (efx->devlink) { >> + struct efx_devlink *devlink_private; >> + >> + devlink_private = devlink_priv(efx->devlink); >> + >> + devlink_unregister(efx->devlink); >> + devlink_free(efx->devlink); >> + efx->devlink = NULL; >> + } >> +} >> + >> +int efx_probe_devlink(struct efx_nic *efx) >> +{ >> + struct efx_devlink *devlink_private; >> + >> + efx->devlink = devlink_alloc(&sfc_devlink_ops, >> + sizeof(struct efx_devlink), >> + &efx->pci_dev->dev); >> + if (!efx->devlink) >> + return -ENOMEM; >> + devlink_private = devlink_priv(efx->devlink); >> + devlink_private->efx = efx; >> + >> + devlink_register(efx->devlink); >> + >> + return 0; >> +} >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h >> new file mode 100644 >> index 000000000000..997f878aea93 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_devlink.h >> @@ -0,0 +1,20 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/**************************************************************************** >> + * Driver for AMD network controllers and boards >> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation, incorporated herein by reference. >> + */ >> + >> +#ifndef _EFX_DEVLINK_H >> +#define _EFX_DEVLINK_H >> + >> +#include "net_driver.h" >> +#include <net/devlink.h> >> + >> +int efx_probe_devlink(struct efx_nic *efx); >> +void efx_fini_devlink(struct efx_nic *efx); >> + >> +#endif /* _EFX_DEVLINK_H */ >> diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c >> index af338208eae9..328cae82a7d8 100644 >> --- a/drivers/net/ethernet/sfc/mcdi.c >> +++ b/drivers/net/ethernet/sfc/mcdi.c >> @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) >> return rc; >> } >> >> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >> + u32 *subtype, u16 version[4], char *desc, >> + size_t descsize) >> +{ >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); >> + efx_dword_t *outbuf; >> + size_t outlen; >> + u32 flags; >> + int rc; >> + >> + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); >> + if (!outbuf) >> + return -ENOMEM; >> + >> + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); >> + >> + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, >> + sizeof(inbuf), outbuf, >> + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, >> + &outlen); >> + if (rc) >> + goto out_free; >> + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { >> + rc = -EIO; >> + goto out_free; >> + } >> + >> + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); >> + >> + if (desc && descsize > 0) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { >> + if (descsize <= >> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { >> + rc = -E2BIG; >> + goto out_free; >> + } >> + >> + strncpy(desc, >> + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), >> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); >> + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; >> + } else { >> + desc[0] = '\0'; >> + } >> + } >> + >> + if (subtype) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) >> + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); >> + else >> + *subtype = 0; >> + } >> + >> + if (version) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { >> + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); >> + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); >> + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); >> + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); >> + } else { >> + version[0] = 0; >> + version[1] = 0; >> + version[2] = 0; >> + version[3] = 0; >> + } >> + } >> + >> +out_free: >> + kfree(outbuf); >> + return rc; >> +} >> + >> int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, >> size_t len, size_t *retlen, u8 *buffer) >> { >> diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h >> index 7e35fec9da35..63b090587f7a 100644 >> --- a/drivers/net/ethernet/sfc/mcdi.h >> +++ b/drivers/net/ethernet/sfc/mcdi.h >> @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, >> bool *protected_out); >> int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); >> int efx_mcdi_nvram_test_all(struct efx_nic *efx); >> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >> + u32 *subtype, u16 version[4], char *desc, >> + size_t descsize); >> int efx_mcdi_handle_assertion(struct efx_nic *efx); >> int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); >> int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, >> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >> index 3b49e216768b..d036641dc043 100644 >> --- a/drivers/net/ethernet/sfc/net_driver.h >> +++ b/drivers/net/ethernet/sfc/net_driver.h >> @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { >> * xdp_rxq_info structures? >> * @netdev_notifier: Netdevice notifier. >> * @tc: state for TC offload (EF100). >> + * @devlink: reference to devlink structure owned by this device >> * @mem_bar: The BAR that is mapped into membase. >> * @reg_base: Offset from the start of the bar to the function control window. >> * @monitor_work: Hardware monitor workitem >> @@ -1179,6 +1180,7 @@ struct efx_nic { >> struct notifier_block netdev_notifier; >> struct efx_tc_state *tc; >> >> + struct devlink *devlink; >> unsigned int mem_bar; >> u32 reg_base; >> >> -- >> 2.17.1 >>
Hi, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on net-next/master] url: https://github.com/intel-lab-lkp/linux/commits/alejandro-lucero-palau-amd-com/sfc-add-devlink-support-for-ef100/20230119-193440 patch link: https://lore.kernel.org/r/20230119113140.20208-2-alejandro.lucero-palau%40amd.com patch subject: [PATCH net-next 1/7] sfc: add devlink support for ef100 config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230119/202301192303.3zOqMgPP-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/f393f2642abb0e7536df6f9f08e0d69ecfd5b435 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review alejandro-lucero-palau-amd-com/sfc-add-devlink-support-for-ef100/20230119-193440 git checkout f393f2642abb0e7536df6f9f08e0d69ecfd5b435 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/net/ethernet/sfc/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): drivers/net/ethernet/sfc/efx_devlink.c: In function 'efx_fini_devlink': >> drivers/net/ethernet/sfc/efx_devlink.c:402:37: warning: variable 'devlink_private' set but not used [-Wunused-but-set-variable] 402 | struct efx_devlink *devlink_private; | ^~~~~~~~~~~~~~~ vim +/devlink_private +402 drivers/net/ethernet/sfc/efx_devlink.c 398 399 void efx_fini_devlink(struct efx_nic *efx) 400 { 401 if (efx->devlink) { > 402 struct efx_devlink *devlink_private; 403 404 devlink_private = devlink_priv(efx->devlink); 405 406 devlink_unregister(efx->devlink); 407 devlink_free(efx->devlink); 408 efx->devlink = NULL; 409 } 410 } 411
On 1/19/23 14:51, Lucero Palau, Alejandro wrote: > On 1/19/23 12:14, Jiri Pirko wrote: >> Thu, Jan 19, 2023 at 12:31:34PM CET, alejandro.lucero-palau@amd.com wrote: >>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>> >>> Basic support for devlink info command. >>> >>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>> --- >>> drivers/net/ethernet/sfc/Kconfig | 1 + >>> drivers/net/ethernet/sfc/Makefile | 3 +- >>> drivers/net/ethernet/sfc/ef100_nic.c | 6 + >>> drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ >>> drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ >>> drivers/net/ethernet/sfc/mcdi.c | 72 +++++ >>> drivers/net/ethernet/sfc/mcdi.h | 3 + >>> drivers/net/ethernet/sfc/net_driver.h | 2 + >>> 8 files changed, 533 insertions(+), 1 deletion(-) >>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c >>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h >> Could you please split? >> >> At least the devlink introduction part and info implementation. >> > Sure. Just for being sure, would it be fine to have a simple info implementation first along with the devlink infrastructure then a second patch adding the more complex devlink info support? I guess I need at least one operation supported initially. >>> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig >>> index 0950e6b0508f..4af36ba8906b 100644 >>> --- a/drivers/net/ethernet/sfc/Kconfig >>> +++ b/drivers/net/ethernet/sfc/Kconfig >>> @@ -22,6 +22,7 @@ config SFC >>> depends on PTP_1588_CLOCK_OPTIONAL >>> select MDIO >>> select CRC32 >>> + select NET_DEVLINK >>> help >>> This driver supports 10/40-gigabit Ethernet cards based on >>> the Solarflare SFC9100-family controllers. >>> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >>> index 712a48d00069..55b9c73cd8ef 100644 >>> --- a/drivers/net/ethernet/sfc/Makefile >>> +++ b/drivers/net/ethernet/sfc/Makefile >>> @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ >>> mcdi.o mcdi_port.o mcdi_port_common.o \ >>> mcdi_functions.o mcdi_filters.o mcdi_mon.o \ >>> ef100.o ef100_nic.o ef100_netdev.o \ >>> - ef100_ethtool.o ef100_rx.o ef100_tx.o >>> + ef100_ethtool.o ef100_rx.o ef100_tx.o \ >>> + efx_devlink.o >>> sfc-$(CONFIG_SFC_MTD) += mtd.o >>> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ >>> mae.o tc.o tc_bindings.o tc_counters.o >>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>> index ad686c671ab8..14af8f314b83 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>> @@ -27,6 +27,7 @@ >>> #include "tc.h" >>> #include "mae.h" >>> #include "rx_common.h" >>> +#include "efx_devlink.h" >>> >>> #define EF100_MAX_VIS 4096 >>> #define EF100_NUM_MCDI_BUFFERS 1 >>> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >>> netif_warn(efx, probe, net_dev, >>> "Failed to probe base mport rc %d; representors will not function\n", >>> rc); >>> + } else { >>> + if (efx_probe_devlink(efx)) >> I don't understand why the devlink register call is here. >> 1) You can registers it for PF and VF. I don't follow why you do it only >> for PF. > We discuss the possibility of creating the devlink interface for VFs, > but we decided not to. Arguably, this should not be available for VFs > owned by VMs/containers, or at least in our case, since the interface is > being used for getting information about the whole device or for > getting/setting the MAC address. We plan to support more devlink > functionalities in the near future, so maybe we add such support then if > we consider it is needed. > >> 2) It should be done as the first thing in the probe flow. Then devlink >> port register, only after that netdev. It makes sense from the >> perspective of object hierarchy. > I can not see a problem here. It is not the first thing done in the > probe function, but I can not see any dependency for being so. And it is > done before the devlink port registration and before the netdev > registration what seems to be what other drivers do. What am I missing > here? > >> It is also usual to have the devlink priv as the "main" driver >> structure storage, see how that is done in mlx5 of mlxsw for example. > I will check the way others drivers use the devlink priv. Interestingly, there are 26 calls to devlink_alloc with half of them using the main driver structure and the other half using a specific devlink private struct. This is a huge responsibility now for me! I'm afraid, for the shake of having this merged as soon as possible, I'll keep the current allocation. I did not suffer any problem with this approach while implementing the current support. If, for the further devlink support we plan to add, it turns out to be an issue, we will to the changes then. I would say it is a main change now or then, and current work does not justify it now. >> >>> + netif_warn(efx, probe, net_dev, >>> + "Failed to register devlink\n"); >>> } >>> >>> rc = efx_init_tc(efx); >>> @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) >>> { >>> struct ef100_nic_data *nic_data = efx->nic_data; >>> >>> + efx_fini_devlink(efx); >>> efx_mcdi_detach(efx); >>> efx_mcdi_fini(efx); >>> if (nic_data) >>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>> new file mode 100644 >>> index 000000000000..c506f8f35d25 >>> --- /dev/null >>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>> @@ -0,0 +1,427 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/**************************************************************************** >>> + * Driver for AMD network controllers and boards >>> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms of the GNU General Public License version 2 as published >>> + * by the Free Software Foundation, incorporated herein by reference. >>> + */ >>> + >>> +#include <linux/rtc.h> >>> +#include "net_driver.h" >>> +#include "ef100_nic.h" >>> +#include "efx_devlink.h" >>> +#include "nic.h" >>> +#include "mcdi.h" >>> +#include "mcdi_functions.h" >>> +#include "mcdi_pcol.h" >>> + >>> +/* Custom devlink-info version object names for details that do not map to the >>> + * generic standardized names. >>> + */ >>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >>> +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" >>> + >>> +#define EFX_MAX_VERSION_INFO_LEN 64 >>> + >>> +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, >>> + struct devlink_info_req *req, >>> + unsigned int partition_type, >>> + const char *version_name) >>> +{ >>> + char buf[EFX_MAX_VERSION_INFO_LEN]; >>> + u16 version[4]; >>> + int rc; >>> + >>> + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, >>> + 0); >>> + if (rc) >>> + return rc; >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], >>> + version[1], version[2], version[3]); >>> + devlink_info_version_stored_put(req, version_name, buf); >>> + >>> + return 0; >>> +} >>> + >>> +static void efx_devlink_info_stored_versions(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_MC_FIRMWARE, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_EXPANSION_ROM, >>> + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, >>> + EFX_DEVLINK_INFO_VERSION_FW_UEFI); >>> +} >>> + >>> +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) >>> + >>> +static void efx_devlink_info_board_cfg(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + char sn[EFX_MAX_SERIALNUM_LEN]; >>> + u8 mac_address[ETH_ALEN]; >>> + int rc; >>> + >>> + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); >>> + if (!rc) { >>> + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); >>> + devlink_info_serial_number_put(req, sn); >>> + } >>> +} >>> + >>> +#define EFX_VER_FLAG(_f) \ >>> + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) >>> + >>> +static void efx_devlink_info_running_versions(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); >>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); >>> + char buf[EFX_MAX_VERSION_INFO_LEN]; >>> + unsigned int flags, build_id; >>> + union { >>> + const __le32 *dwords; >>> + const __le16 *words; >>> + const char *str; >>> + } ver; >>> + struct rtc_time build_date; >>> + size_t outlength, offset; >>> + u64 tstamp; >>> + int rc; >>> + >>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), >>> + outbuf, sizeof(outbuf), &outlength); >>> + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) >>> + return; >>> + >>> + /* Handle previous output */ >>> + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { >>> + ver.words = (__le16 *)MCDI_PTR(outbuf, >>> + GET_VERSION_EXT_OUT_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le16_to_cpu(ver.words[0]), >>> + le16_to_cpu(ver.words[1]), >>> + le16_to_cpu(ver.words[2]), >>> + le16_to_cpu(ver.words[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >>> + buf); >>> + return; >>> + } >>> + >>> + /* Handle V2 additions */ >>> + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); >>> + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", >>> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); >>> + devlink_info_version_fixed_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, >>> + buf); >>> + >>> + /* Favour full board version if present (in V5 or later) */ >>> + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", >>> + MCDI_DWORD(outbuf, >>> + GET_VERSION_V2_OUT_BOARD_REVISION)); >>> + devlink_info_version_fixed_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >>> + buf); >>> + } >>> + >>> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); >>> + if (ver.str[0]) >>> + devlink_info_board_serial_number_put(req, ver.str); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V2_OUT_FPGA_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", >>> + le32_to_cpu(ver.dwords[0]), >>> + 'A' + le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2])); >>> + >>> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); >>> + if (ver.str[0]) >>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>> + " (%s)", ver.str); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FPGA_REV, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V2_OUT_CMCFW_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), >>> + le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + tstamp = MCDI_QWORD(outbuf, >>> + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); >>> + if (tstamp) { >>> + rtc_time64_to_tm(tstamp, &build_date); >>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>> + " (%ptRd)", &build_date); >>> + } >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, >>> + buf); >>> + } >>> + >>> + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), >>> + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); >>> + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { >>> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); >>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>> + " (%x) %s", build_id, >>> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); >>> + } >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >>> + buf); >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V2_OUT_SUCFW_VERSION); >>> + tstamp = MCDI_QWORD(outbuf, >>> + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); >>> + rtc_time64_to_tm(tstamp, &build_date); >>> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, >>> + "%u.%u.%u.%u type %x (%ptRd)", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), >>> + build_id, &build_date); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >>> + buf); >>> + } >>> + >>> + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) >>> + return; >>> + >>> + /* Handle V3 additions */ >>> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, >>> + buf); >>> + } >>> + >>> + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) >>> + return; >>> + >>> + /* Handle V4 additions */ >>> + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && >>> + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SUCFW_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >>> + buf); >>> + } >>> + >>> + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) >>> + return; >>> + >>> + /* Handle V5 additions */ >>> + >>> + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V5_OUT_BOARD_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V5_OUT_BUNDLE_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, >>> + buf); >>> + } >>> +} >>> + >>> +#undef EFX_VER_FLAG >>> + >>> +static void efx_devlink_info_query_all(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + efx_devlink_info_board_cfg(efx, req); >>> + efx_devlink_info_stored_versions(efx, req); >>> + efx_devlink_info_running_versions(efx, req); >>> +} >>> + >>> +struct efx_devlink { >>> + struct efx_nic *efx; >>> +}; >>> + >>> +static int efx_devlink_info_get(struct devlink *devlink, >>> + struct devlink_info_req *req, >>> + struct netlink_ext_ack *extack) >>> +{ >>> + struct efx_devlink *devlink_private = devlink_priv(devlink); >>> + struct efx_nic *efx = devlink_private->efx; >>> + >>> + efx_devlink_info_query_all(efx, req); >> I don't understand the reason for having efx_devlink_info_query_all() as >> a separate function in compare to inline its code here. >> > Yes, it could be do as you say. I'll do. > > Thanks > >>> + return 0; >>> +} >>> + >>> +static const struct devlink_ops sfc_devlink_ops = { >>> + .info_get = efx_devlink_info_get, >>> +}; >>> + >>> +void efx_fini_devlink(struct efx_nic *efx) >>> +{ >>> + if (efx->devlink) { >>> + struct efx_devlink *devlink_private; >>> + >>> + devlink_private = devlink_priv(efx->devlink); >>> + >>> + devlink_unregister(efx->devlink); >>> + devlink_free(efx->devlink); >>> + efx->devlink = NULL; >>> + } >>> +} >>> + >>> +int efx_probe_devlink(struct efx_nic *efx) >>> +{ >>> + struct efx_devlink *devlink_private; >>> + >>> + efx->devlink = devlink_alloc(&sfc_devlink_ops, >>> + sizeof(struct efx_devlink), >>> + &efx->pci_dev->dev); >>> + if (!efx->devlink) >>> + return -ENOMEM; >>> + devlink_private = devlink_priv(efx->devlink); >>> + devlink_private->efx = efx; >>> + >>> + devlink_register(efx->devlink); >>> + >>> + return 0; >>> +} >>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h >>> new file mode 100644 >>> index 000000000000..997f878aea93 >>> --- /dev/null >>> +++ b/drivers/net/ethernet/sfc/efx_devlink.h >>> @@ -0,0 +1,20 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/**************************************************************************** >>> + * Driver for AMD network controllers and boards >>> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms of the GNU General Public License version 2 as published >>> + * by the Free Software Foundation, incorporated herein by reference. >>> + */ >>> + >>> +#ifndef _EFX_DEVLINK_H >>> +#define _EFX_DEVLINK_H >>> + >>> +#include "net_driver.h" >>> +#include <net/devlink.h> >>> + >>> +int efx_probe_devlink(struct efx_nic *efx); >>> +void efx_fini_devlink(struct efx_nic *efx); >>> + >>> +#endif /* _EFX_DEVLINK_H */ >>> diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c >>> index af338208eae9..328cae82a7d8 100644 >>> --- a/drivers/net/ethernet/sfc/mcdi.c >>> +++ b/drivers/net/ethernet/sfc/mcdi.c >>> @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) >>> return rc; >>> } >>> >>> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >>> + u32 *subtype, u16 version[4], char *desc, >>> + size_t descsize) >>> +{ >>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); >>> + efx_dword_t *outbuf; >>> + size_t outlen; >>> + u32 flags; >>> + int rc; >>> + >>> + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); >>> + if (!outbuf) >>> + return -ENOMEM; >>> + >>> + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); >>> + >>> + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, >>> + sizeof(inbuf), outbuf, >>> + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, >>> + &outlen); >>> + if (rc) >>> + goto out_free; >>> + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { >>> + rc = -EIO; >>> + goto out_free; >>> + } >>> + >>> + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); >>> + >>> + if (desc && descsize > 0) { >>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { >>> + if (descsize <= >>> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { >>> + rc = -E2BIG; >>> + goto out_free; >>> + } >>> + >>> + strncpy(desc, >>> + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), >>> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); >>> + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; >>> + } else { >>> + desc[0] = '\0'; >>> + } >>> + } >>> + >>> + if (subtype) { >>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) >>> + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); >>> + else >>> + *subtype = 0; >>> + } >>> + >>> + if (version) { >>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { >>> + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); >>> + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); >>> + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); >>> + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); >>> + } else { >>> + version[0] = 0; >>> + version[1] = 0; >>> + version[2] = 0; >>> + version[3] = 0; >>> + } >>> + } >>> + >>> +out_free: >>> + kfree(outbuf); >>> + return rc; >>> +} >>> + >>> int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, >>> size_t len, size_t *retlen, u8 *buffer) >>> { >>> diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h >>> index 7e35fec9da35..63b090587f7a 100644 >>> --- a/drivers/net/ethernet/sfc/mcdi.h >>> +++ b/drivers/net/ethernet/sfc/mcdi.h >>> @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, >>> bool *protected_out); >>> int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); >>> int efx_mcdi_nvram_test_all(struct efx_nic *efx); >>> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >>> + u32 *subtype, u16 version[4], char *desc, >>> + size_t descsize); >>> int efx_mcdi_handle_assertion(struct efx_nic *efx); >>> int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); >>> int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, >>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >>> index 3b49e216768b..d036641dc043 100644 >>> --- a/drivers/net/ethernet/sfc/net_driver.h >>> +++ b/drivers/net/ethernet/sfc/net_driver.h >>> @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { >>> * xdp_rxq_info structures? >>> * @netdev_notifier: Netdevice notifier. >>> * @tc: state for TC offload (EF100). >>> + * @devlink: reference to devlink structure owned by this device >>> * @mem_bar: The BAR that is mapped into membase. >>> * @reg_base: Offset from the start of the bar to the function control window. >>> * @monitor_work: Hardware monitor workitem >>> @@ -1179,6 +1180,7 @@ struct efx_nic { >>> struct notifier_block netdev_notifier; >>> struct efx_tc_state *tc; >>> >>> + struct devlink *devlink; >>> unsigned int mem_bar; >>> u32 reg_base; >>> >>> -- >>> 2.17.1 >>>
On Thu, 19 Jan 2023 11:31:34 +0000 alejandro.lucero-palau@amd.com wrote: > + devlink_unregister(efx->devlink); > + devlink_free(efx->devlink); Please use the devl_ APIs and take the devl_lock() explicitly. Once you start adding sub-objects the API with implicit locking gets racy.
On 1/19/23 17:16, Jakub Kicinski wrote: > On Thu, 19 Jan 2023 11:31:34 +0000 alejandro.lucero-palau@amd.com wrote: >> + devlink_unregister(efx->devlink); >> + devlink_free(efx->devlink); > Please use the devl_ APIs and take the devl_lock() explicitly. > Once you start adding sub-objects the API with implicit locking > gets racy. I need more help here. The explicit locking you refer to, is it for this specific code only? Also, I can not see all drivers locking/unlocking when doing devlink_unregister. Those doing it are calling code which invoke unregister devlink ports, like the NFP and I think ml5x as well. In this case, no devlink port remains at this point, and no netdev either. What is the potential race against?
On Thu, 19 Jan 2023 17:52:42 +0000 Lucero Palau, Alejandro wrote: > On 1/19/23 17:16, Jakub Kicinski wrote: > > On Thu, 19 Jan 2023 11:31:34 +0000 alejandro.lucero-palau@amd.com wrote: > >> + devlink_unregister(efx->devlink); > >> + devlink_free(efx->devlink); > > Please use the devl_ APIs and take the devl_lock() explicitly. > > Once you start adding sub-objects the API with implicit locking > > gets racy. > > I need more help here. > > The explicit locking you refer to, is it for this specific code only? I only had a quick look at the series, but I saw you add ports. So the locking should be something like: devlink = devlink_alloc(); devl_lock(devlink); ... devl_register(devlink); ... netdev_register(netdev); devl_port_register(port_for_the_netdev); ... devl_unlock(); And the inverse on the .remove path. Basically you want to hold the devlink instance lock for most of the .probe and .remove. That way nothing can bother the devlink instance and the driver while the driver is initializing/finalizing. Without holding the lock the linking between the devlink port and the netdev gets a bit iffy. It's a circular dependency of sorts because both the netdev carries a link to the port and the port carries info about the netdev. We've been figuring out workarounds for subtle ordering and locking problems since devlink ports were created. Recently we just gave up and started asking drivers to hold the instance lock across .probe/ /.remove. > Also, I can not see all drivers locking/unlocking when doing > devlink_unregister. Those doing it are calling code which invoke > unregister devlink ports, like the NFP and I think ml5x as well. Right, only netdevsim was fully converted so far. The syzbot and other testers use netdevsim mostly. We'll push actual HW drivers towards this locking slowly. > In this case, no devlink port remains at this point, and no netdev either. > > What is the potential race against? Right, I don't mean this particular spot, just over-trimmed the quote.
On 1/19/23 18:44, Jakub Kicinski wrote: > On Thu, 19 Jan 2023 17:52:42 +0000 Lucero Palau, Alejandro wrote: >> On 1/19/23 17:16, Jakub Kicinski wrote: >>> On Thu, 19 Jan 2023 11:31:34 +0000 alejandro.lucero-palau@amd.com wrote: >>>> + devlink_unregister(efx->devlink); >>>> + devlink_free(efx->devlink); >>> Please use the devl_ APIs and take the devl_lock() explicitly. >>> Once you start adding sub-objects the API with implicit locking >>> gets racy. >> I need more help here. >> >> The explicit locking you refer to, is it for this specific code only? > I only had a quick look at the series, but I saw you add ports. > So the locking should be something like: > > devlink = devlink_alloc(); > devl_lock(devlink); > ... > devl_register(devlink); > ... > netdev_register(netdev); > devl_port_register(port_for_the_netdev); > ... > devl_unlock(); > > And the inverse on the .remove path. > Basically you want to hold the devlink instance lock for most of > the .probe and .remove. That way nothing can bother the devlink > instance and the driver while the driver is initializing/finalizing. > > Without holding the lock the linking between the devlink port and > the netdev gets a bit iffy. It's a circular dependency of sorts > because both the netdev carries a link to the port and the port > carries info about the netdev. > > We've been figuring out workarounds for subtle ordering and locking > problems since devlink ports were created. Recently we just gave up > and started asking drivers to hold the instance lock across .probe/ > /.remove. OK. Thanks for the explanation. I will add the locking. >> Also, I can not see all drivers locking/unlocking when doing >> devlink_unregister. Those doing it are calling code which invoke >> unregister devlink ports, like the NFP and I think ml5x as well. > Right, only netdevsim was fully converted so far. The syzbot and other > testers use netdevsim mostly. We'll push actual HW drivers towards this > locking slowly. > >> In this case, no devlink port remains at this point, and no netdev either. >> >> What is the potential race against? > Right, I don't mean this particular spot, just over-trimmed the quote.
On 1/19/2023 3:31 AM, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alejandro.lucero-palau@amd.com> > > Basic support for devlink info command. > > Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> > --- > drivers/net/ethernet/sfc/Kconfig | 1 + > drivers/net/ethernet/sfc/Makefile | 3 +- > drivers/net/ethernet/sfc/ef100_nic.c | 6 + > drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ > drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ > drivers/net/ethernet/sfc/mcdi.c | 72 +++++ > drivers/net/ethernet/sfc/mcdi.h | 3 + > drivers/net/ethernet/sfc/net_driver.h | 2 + > 8 files changed, 533 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c > create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h > > diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig > index 0950e6b0508f..4af36ba8906b 100644 > --- a/drivers/net/ethernet/sfc/Kconfig > +++ b/drivers/net/ethernet/sfc/Kconfig > @@ -22,6 +22,7 @@ config SFC > depends on PTP_1588_CLOCK_OPTIONAL > select MDIO > select CRC32 > + select NET_DEVLINK > help > This driver supports 10/40-gigabit Ethernet cards based on > the Solarflare SFC9100-family controllers. > diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile > index 712a48d00069..55b9c73cd8ef 100644 > --- a/drivers/net/ethernet/sfc/Makefile > +++ b/drivers/net/ethernet/sfc/Makefile > @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ > mcdi.o mcdi_port.o mcdi_port_common.o \ > mcdi_functions.o mcdi_filters.o mcdi_mon.o \ > ef100.o ef100_nic.o ef100_netdev.o \ > - ef100_ethtool.o ef100_rx.o ef100_tx.o > + ef100_ethtool.o ef100_rx.o ef100_tx.o \ > + efx_devlink.o > sfc-$(CONFIG_SFC_MTD) += mtd.o > sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ > mae.o tc.o tc_bindings.o tc_counters.o > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c > index ad686c671ab8..14af8f314b83 100644 > --- a/drivers/net/ethernet/sfc/ef100_nic.c > +++ b/drivers/net/ethernet/sfc/ef100_nic.c > @@ -27,6 +27,7 @@ > #include "tc.h" > #include "mae.h" > #include "rx_common.h" > +#include "efx_devlink.h" > > #define EF100_MAX_VIS 4096 > #define EF100_NUM_MCDI_BUFFERS 1 > @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) > netif_warn(efx, probe, net_dev, > "Failed to probe base mport rc %d; representors will not function\n", > rc); > + } else { > + if (efx_probe_devlink(efx)) > + netif_warn(efx, probe, net_dev, > + "Failed to register devlink\n"); > } > A bit of a weird construction here with the next step in an else block? I guess this is being treated as an optional feature, and depends on efx_ef100_get_base_mport succeeding? It reads a little awkward, but I can't think of a better way to arrange this unless another helper function was used to combine these two calls in order to avoid the extra indentation. > rc = efx_init_tc(efx); > @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) > { > struct ef100_nic_data *nic_data = efx->nic_data; > > + efx_fini_devlink(efx); > efx_mcdi_detach(efx); > efx_mcdi_fini(efx); > if (nic_data) > diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c > new file mode 100644 > index 000000000000..c506f8f35d25 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_devlink.c > @@ -0,0 +1,427 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/**************************************************************************** > + * Driver for AMD network controllers and boards > + * Copyright (C) 2023, Advanced Micro Devices, Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > + * by the Free Software Foundation, incorporated herein by reference. > + */ > + > +#include <linux/rtc.h> > +#include "net_driver.h" > +#include "ef100_nic.h" > +#include "efx_devlink.h" > +#include "nic.h" > +#include "mcdi.h" > +#include "mcdi_functions.h" > +#include "mcdi_pcol.h" > + > +/* Custom devlink-info version object names for details that do not map to the > + * generic standardized names. > + */ > +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" > +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" > +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" > +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" > +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP > +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" > +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" > +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" > +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" > +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" > +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" Here, you've defined several new versions, only one of which is standard. I don't see an associated documentation addition. Please add a ef100.rst file to Documentation/networking/devlink It is also preferred to use the standard names or extend the set of standard names where appropriate first. Can you explain why you didn't do that here? For example, the documentation for "fw.undi" indicates it may include the UEFI driver, and I would expect your UEFI version to be something like fw.undi or fw.undi.uefi if its distinct... > + > +#define EFX_MAX_VERSION_INFO_LEN 64 > + > +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, > + struct devlink_info_req *req, > + unsigned int partition_type, > + const char *version_name) > +{ > + char buf[EFX_MAX_VERSION_INFO_LEN]; > + u16 version[4]; > + int rc; > + > + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, > + 0); > + if (rc) > + return rc; > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], > + version[1], version[2], version[3]); > + devlink_info_version_stored_put(req, version_name, buf); > + > + return 0; > +} > + > +static void efx_devlink_info_stored_versions(struct efx_nic *efx, > + struct devlink_info_req *req) > +{ > + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, > + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); > + efx_devlink_info_nvram_partition(efx, req, > + NVRAM_PARTITION_TYPE_MC_FIRMWARE, > + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); > + efx_devlink_info_nvram_partition(efx, req, > + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, > + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); > + efx_devlink_info_nvram_partition(efx, req, > + NVRAM_PARTITION_TYPE_EXPANSION_ROM, > + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); > + efx_devlink_info_nvram_partition(efx, req, > + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, > + EFX_DEVLINK_INFO_VERSION_FW_UEFI); > +} > + > +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) > + > +static void efx_devlink_info_board_cfg(struct efx_nic *efx, > + struct devlink_info_req *req) > +{ > + char sn[EFX_MAX_SERIALNUM_LEN]; > + u8 mac_address[ETH_ALEN]; > + int rc; > + > + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); > + if (!rc) { > + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); > + devlink_info_serial_number_put(req, sn); > + } > +} > + > +#define EFX_VER_FLAG(_f) \ > + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) > + > +static void efx_devlink_info_running_versions(struct efx_nic *efx, > + struct devlink_info_req *req) > +{ This function for doing running versions is very long, and seems quite complicated. It's difficult to parse what versions are being reported. I saw that your stored version used a helper function to separate each thing out nicely. Is there any way to do that here? > + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); > + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); > + char buf[EFX_MAX_VERSION_INFO_LEN]; > + unsigned int flags, build_id; > + union { > + const __le32 *dwords; > + const __le16 *words; > + const char *str; > + } ver; For example, you've got this local union on the stack that seems to be reused a bunch... > + struct rtc_time build_date; > + size_t outlength, offset; > + u64 tstamp; > + int rc; > + > + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), > + outbuf, sizeof(outbuf), &outlength); > + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) > + return; > + > + /* Handle previous output */ > + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { > + ver.words = (__le16 *)MCDI_PTR(outbuf, > + GET_VERSION_EXT_OUT_VERSION); > + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le16_to_cpu(ver.words[0]), > + le16_to_cpu(ver.words[1]), > + le16_to_cpu(ver.words[2]), > + le16_to_cpu(ver.words[3])); > + > + devlink_info_version_running_put(req, > + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, > + buf); > + return; > + } > + > + /* Handle V2 additions */ > + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); > + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", > + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); > + devlink_info_version_fixed_put(req, > + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, > + buf); > + > + /* Favour full board version if present (in V5 or later) */ > + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", > + MCDI_DWORD(outbuf, > + GET_VERSION_V2_OUT_BOARD_REVISION)); > + devlink_info_version_fixed_put(req, > + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, > + buf); > + } > + > + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); > + if (ver.str[0]) > + devlink_info_board_serial_number_put(req, ver.str); > + } > + > + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V2_OUT_FPGA_VERSION); > + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", > + le32_to_cpu(ver.dwords[0]), > + 'A' + le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2])); > + > + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); > + if (ver.str[0]) > + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, > + " (%s)", ver.str); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_FPGA_REV, > + buf); > + } > + > + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V2_OUT_CMCFW_VERSION); > + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), > + le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + tstamp = MCDI_QWORD(outbuf, > + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); > + if (tstamp) { > + rtc_time64_to_tm(tstamp, &build_date); > + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, > + " (%ptRd)", &build_date); > + } > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, > + buf); > + } > + > + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); > + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), > + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); > + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { > + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); > + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, > + " (%x) %s", build_id, > + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); > + } > + devlink_info_version_running_put(req, > + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, > + buf); > + > + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V2_OUT_SUCFW_VERSION); > + tstamp = MCDI_QWORD(outbuf, > + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); > + rtc_time64_to_tm(tstamp, &build_date); > + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, > + "%u.%u.%u.%u type %x (%ptRd)", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), > + build_id, &build_date); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, > + buf); > + } > + > + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) > + return; > + > + /* Handle V3 additions */ > + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2])); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, > + buf); > + } > + > + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2])); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, > + buf); > + } > + > + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) > + return; > + > + /* Handle V4 additions */ > + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, > + buf); > + } > + > + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, > + buf); > + } > + > + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, > + buf); > + } > + > + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, > + buf); > + } > + > + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && > + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V4_OUT_SUCFW_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + devlink_info_version_running_put(req, > + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, > + buf); > + } > + > + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) > + return; > + > + /* Handle V5 additions */ > + > + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V5_OUT_BOARD_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + devlink_info_version_running_put(req, > + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, > + buf); > + } > + > + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { > + ver.dwords = (__le32 *)MCDI_PTR(outbuf, > + GET_VERSION_V5_OUT_BUNDLE_VERSION); > + > + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", > + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), > + le32_to_cpu(ver.dwords[2]), > + le32_to_cpu(ver.dwords[3])); > + > + devlink_info_version_running_put(req, > + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, > + buf); > + } > +} In the ice driver I implemented each version extraction as a separate function to make it more clear which versions where which. This running function could benefit from some organization like that. > + > +#undef EFX_VER_FLAG > + > +static void efx_devlink_info_query_all(struct efx_nic *efx, > + struct devlink_info_req *req) > +{ > + efx_devlink_info_board_cfg(efx, req); > + efx_devlink_info_stored_versions(efx, req); > + efx_devlink_info_running_versions(efx, req); > +} Do you really need this to be a separate function and not just inlined into the caller, efx_devlink_info_get. > + > +struct efx_devlink { > + struct efx_nic *efx; > +}; > + > +static int efx_devlink_info_get(struct devlink *devlink, > + struct devlink_info_req *req, > + struct netlink_ext_ack *extack) > +{ > + struct efx_devlink *devlink_private = devlink_priv(devlink); > + struct efx_nic *efx = devlink_private->efx; > + > + efx_devlink_info_query_all(efx, req); > + return 0; > +} > + > +static const struct devlink_ops sfc_devlink_ops = { > + .info_get = efx_devlink_info_get, > +}; > + > +void efx_fini_devlink(struct efx_nic *efx) > +{ > + if (efx->devlink) { > + struct efx_devlink *devlink_private; > + > + devlink_private = devlink_priv(efx->devlink); > + > + devlink_unregister(efx->devlink); > + devlink_free(efx->devlink); > + efx->devlink = NULL; > + } > +} > + > +int efx_probe_devlink(struct efx_nic *efx) > +{ > + struct efx_devlink *devlink_private; > + > + efx->devlink = devlink_alloc(&sfc_devlink_ops, > + sizeof(struct efx_devlink), > + &efx->pci_dev->dev); > + if (!efx->devlink) > + return -ENOMEM; > + devlink_private = devlink_priv(efx->devlink); > + devlink_private->efx = efx; > + > + devlink_register(efx->devlink); > + So drivers implementing devlink typically would allocate the devlink early (almost the first thing it does) and make its private field something like their main private data structure. Obviously you're adding this to a pre-existing driver and it is initially simpler and easier to keep it separate. I'm not sure what other reviewers' stance on this is, but I think its preferable to allocate as an early stage and register once the driver is ready to accept requests from user space. As for a reason why, implementing reload support effectively requires this. > + return 0; > +} > diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h > new file mode 100644 > index 000000000000..997f878aea93 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_devlink.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/**************************************************************************** > + * Driver for AMD network controllers and boards > + * Copyright (C) 2023, Advanced Micro Devices, Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > + * by the Free Software Foundation, incorporated herein by reference. > + */ > + > +#ifndef _EFX_DEVLINK_H > +#define _EFX_DEVLINK_H > + > +#include "net_driver.h" > +#include <net/devlink.h> > + > +int efx_probe_devlink(struct efx_nic *efx); > +void efx_fini_devlink(struct efx_nic *efx); > + > +#endif /* _EFX_DEVLINK_H */ > diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c > index af338208eae9..328cae82a7d8 100644 > --- a/drivers/net/ethernet/sfc/mcdi.c > +++ b/drivers/net/ethernet/sfc/mcdi.c > @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) > return rc; > } > > +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, > + u32 *subtype, u16 version[4], char *desc, > + size_t descsize) > +{ > + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); > + efx_dword_t *outbuf; > + size_t outlen; > + u32 flags; > + int rc; > + > + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); > + if (!outbuf) > + return -ENOMEM; > + > + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); > + > + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, > + sizeof(inbuf), outbuf, > + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, > + &outlen); > + if (rc) > + goto out_free; > + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { > + rc = -EIO; > + goto out_free; > + } > + > + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); > + > + if (desc && descsize > 0) { > + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { > + if (descsize <= > + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { > + rc = -E2BIG; > + goto out_free; > + } > + > + strncpy(desc, > + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), > + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); > + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; > + } else { > + desc[0] = '\0'; > + } > + } > + > + if (subtype) { > + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) > + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); > + else > + *subtype = 0; > + } > + > + if (version) { > + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { > + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); > + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); > + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); > + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); > + } else { > + version[0] = 0; > + version[1] = 0; > + version[2] = 0; > + version[3] = 0; > + } > + } > + > +out_free: > + kfree(outbuf); > + return rc; > +} > + > int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, > size_t len, size_t *retlen, u8 *buffer) > { > diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h > index 7e35fec9da35..63b090587f7a 100644 > --- a/drivers/net/ethernet/sfc/mcdi.h > +++ b/drivers/net/ethernet/sfc/mcdi.h > @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, > bool *protected_out); > int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); > int efx_mcdi_nvram_test_all(struct efx_nic *efx); > +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, > + u32 *subtype, u16 version[4], char *desc, > + size_t descsize); > int efx_mcdi_handle_assertion(struct efx_nic *efx); > int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); > int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, > diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h > index 3b49e216768b..d036641dc043 100644 > --- a/drivers/net/ethernet/sfc/net_driver.h > +++ b/drivers/net/ethernet/sfc/net_driver.h > @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { > * xdp_rxq_info structures? > * @netdev_notifier: Netdevice notifier. > * @tc: state for TC offload (EF100). > + * @devlink: reference to devlink structure owned by this device > * @mem_bar: The BAR that is mapped into membase. > * @reg_base: Offset from the start of the bar to the function control window. > * @monitor_work: Hardware monitor workitem > @@ -1179,6 +1180,7 @@ struct efx_nic { > struct notifier_block netdev_notifier; > struct efx_tc_state *tc; > > + struct devlink *devlink; > unsigned int mem_bar; > u32 reg_base; >
Thu, Jan 19, 2023 at 03:51:53PM CET, alejandro.lucero-palau@amd.com wrote: > >On 1/19/23 12:14, Jiri Pirko wrote: >> Thu, Jan 19, 2023 at 12:31:34PM CET, alejandro.lucero-palau@amd.com wrote: >>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>> >>> Basic support for devlink info command. >>> >>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>> --- >>> drivers/net/ethernet/sfc/Kconfig | 1 + >>> drivers/net/ethernet/sfc/Makefile | 3 +- >>> drivers/net/ethernet/sfc/ef100_nic.c | 6 + >>> drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ >>> drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ >>> drivers/net/ethernet/sfc/mcdi.c | 72 +++++ >>> drivers/net/ethernet/sfc/mcdi.h | 3 + >>> drivers/net/ethernet/sfc/net_driver.h | 2 + >>> 8 files changed, 533 insertions(+), 1 deletion(-) >>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c >>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h >> Could you please split? >> >> At least the devlink introduction part and info implementation. >> > >Sure. > >>> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig >>> index 0950e6b0508f..4af36ba8906b 100644 >>> --- a/drivers/net/ethernet/sfc/Kconfig >>> +++ b/drivers/net/ethernet/sfc/Kconfig >>> @@ -22,6 +22,7 @@ config SFC >>> depends on PTP_1588_CLOCK_OPTIONAL >>> select MDIO >>> select CRC32 >>> + select NET_DEVLINK >>> help >>> This driver supports 10/40-gigabit Ethernet cards based on >>> the Solarflare SFC9100-family controllers. >>> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >>> index 712a48d00069..55b9c73cd8ef 100644 >>> --- a/drivers/net/ethernet/sfc/Makefile >>> +++ b/drivers/net/ethernet/sfc/Makefile >>> @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ >>> mcdi.o mcdi_port.o mcdi_port_common.o \ >>> mcdi_functions.o mcdi_filters.o mcdi_mon.o \ >>> ef100.o ef100_nic.o ef100_netdev.o \ >>> - ef100_ethtool.o ef100_rx.o ef100_tx.o >>> + ef100_ethtool.o ef100_rx.o ef100_tx.o \ >>> + efx_devlink.o >>> sfc-$(CONFIG_SFC_MTD) += mtd.o >>> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ >>> mae.o tc.o tc_bindings.o tc_counters.o >>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>> index ad686c671ab8..14af8f314b83 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>> @@ -27,6 +27,7 @@ >>> #include "tc.h" >>> #include "mae.h" >>> #include "rx_common.h" >>> +#include "efx_devlink.h" >>> >>> #define EF100_MAX_VIS 4096 >>> #define EF100_NUM_MCDI_BUFFERS 1 >>> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >>> netif_warn(efx, probe, net_dev, >>> "Failed to probe base mport rc %d; representors will not function\n", >>> rc); >>> + } else { >>> + if (efx_probe_devlink(efx)) >> I don't understand why the devlink register call is here. >> 1) You can registers it for PF and VF. I don't follow why you do it only >> for PF. > >We discuss the possibility of creating the devlink interface for VFs, >but we decided not to. Arguably, this should not be available for VFs >owned by VMs/containers, or at least in our case, since the interface is >being used for getting information about the whole device or for >getting/setting the MAC address. We plan to support more devlink >functionalities in the near future, so maybe we add such support then if >we consider it is needed. Okay. > >> 2) It should be done as the first thing in the probe flow. Then devlink >> port register, only after that netdev. It makes sense from the >> perspective of object hierarchy. > >I can not see a problem here. It is not the first thing done in the >probe function, but I can not see any dependency for being so. And it is >done before the devlink port registration and before the netdev >registration what seems to be what other drivers do. What am I missing >here? Yep, currently, it does not matter for you, but when you implement devlink reload for example, it will become a problem. Better to have it done correctly from the beginning. Up to you. Remember, what other drivers do does not mean it is a good practise : > >> >> It is also usual to have the devlink priv as the "main" driver >> structure storage, see how that is done in mlx5 of mlxsw for example. > >I will check the way others drivers use the devlink priv. > >> >> >>> + netif_warn(efx, probe, net_dev, >>> + "Failed to register devlink\n"); >>> } >>> >>> rc = efx_init_tc(efx); >>> @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) >>> { >>> struct ef100_nic_data *nic_data = efx->nic_data; >>> >>> + efx_fini_devlink(efx); >>> efx_mcdi_detach(efx); >>> efx_mcdi_fini(efx); >>> if (nic_data) >>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>> new file mode 100644 >>> index 000000000000..c506f8f35d25 >>> --- /dev/null >>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>> @@ -0,0 +1,427 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/**************************************************************************** >>> + * Driver for AMD network controllers and boards >>> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms of the GNU General Public License version 2 as published >>> + * by the Free Software Foundation, incorporated herein by reference. >>> + */ >>> + >>> +#include <linux/rtc.h> >>> +#include "net_driver.h" >>> +#include "ef100_nic.h" >>> +#include "efx_devlink.h" >>> +#include "nic.h" >>> +#include "mcdi.h" >>> +#include "mcdi_functions.h" >>> +#include "mcdi_pcol.h" >>> + >>> +/* Custom devlink-info version object names for details that do not map to the >>> + * generic standardized names. >>> + */ >>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >>> +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" >>> + >>> +#define EFX_MAX_VERSION_INFO_LEN 64 >>> + >>> +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, >>> + struct devlink_info_req *req, >>> + unsigned int partition_type, >>> + const char *version_name) >>> +{ >>> + char buf[EFX_MAX_VERSION_INFO_LEN]; >>> + u16 version[4]; >>> + int rc; >>> + >>> + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, >>> + 0); >>> + if (rc) >>> + return rc; >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], >>> + version[1], version[2], version[3]); >>> + devlink_info_version_stored_put(req, version_name, buf); >>> + >>> + return 0; >>> +} >>> + >>> +static void efx_devlink_info_stored_versions(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_MC_FIRMWARE, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_EXPANSION_ROM, >>> + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); >>> + efx_devlink_info_nvram_partition(efx, req, >>> + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, >>> + EFX_DEVLINK_INFO_VERSION_FW_UEFI); >>> +} >>> + >>> +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) >>> + >>> +static void efx_devlink_info_board_cfg(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + char sn[EFX_MAX_SERIALNUM_LEN]; >>> + u8 mac_address[ETH_ALEN]; >>> + int rc; >>> + >>> + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); >>> + if (!rc) { >>> + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); >>> + devlink_info_serial_number_put(req, sn); >>> + } >>> +} >>> + >>> +#define EFX_VER_FLAG(_f) \ >>> + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) >>> + >>> +static void efx_devlink_info_running_versions(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); >>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); >>> + char buf[EFX_MAX_VERSION_INFO_LEN]; >>> + unsigned int flags, build_id; >>> + union { >>> + const __le32 *dwords; >>> + const __le16 *words; >>> + const char *str; >>> + } ver; >>> + struct rtc_time build_date; >>> + size_t outlength, offset; >>> + u64 tstamp; >>> + int rc; >>> + >>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), >>> + outbuf, sizeof(outbuf), &outlength); >>> + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) >>> + return; >>> + >>> + /* Handle previous output */ >>> + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { >>> + ver.words = (__le16 *)MCDI_PTR(outbuf, >>> + GET_VERSION_EXT_OUT_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le16_to_cpu(ver.words[0]), >>> + le16_to_cpu(ver.words[1]), >>> + le16_to_cpu(ver.words[2]), >>> + le16_to_cpu(ver.words[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >>> + buf); >>> + return; >>> + } >>> + >>> + /* Handle V2 additions */ >>> + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); >>> + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", >>> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); >>> + devlink_info_version_fixed_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, >>> + buf); >>> + >>> + /* Favour full board version if present (in V5 or later) */ >>> + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", >>> + MCDI_DWORD(outbuf, >>> + GET_VERSION_V2_OUT_BOARD_REVISION)); >>> + devlink_info_version_fixed_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >>> + buf); >>> + } >>> + >>> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); >>> + if (ver.str[0]) >>> + devlink_info_board_serial_number_put(req, ver.str); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V2_OUT_FPGA_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", >>> + le32_to_cpu(ver.dwords[0]), >>> + 'A' + le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2])); >>> + >>> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); >>> + if (ver.str[0]) >>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>> + " (%s)", ver.str); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FPGA_REV, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V2_OUT_CMCFW_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), >>> + le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + tstamp = MCDI_QWORD(outbuf, >>> + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); >>> + if (tstamp) { >>> + rtc_time64_to_tm(tstamp, &build_date); >>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>> + " (%ptRd)", &build_date); >>> + } >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, >>> + buf); >>> + } >>> + >>> + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); >>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), >>> + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); >>> + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { >>> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); >>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>> + " (%x) %s", build_id, >>> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); >>> + } >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >>> + buf); >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V2_OUT_SUCFW_VERSION); >>> + tstamp = MCDI_QWORD(outbuf, >>> + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); >>> + rtc_time64_to_tm(tstamp, &build_date); >>> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, >>> + "%u.%u.%u.%u type %x (%ptRd)", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), >>> + build_id, &build_date); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >>> + buf); >>> + } >>> + >>> + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) >>> + return; >>> + >>> + /* Handle V3 additions */ >>> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, >>> + buf); >>> + } >>> + >>> + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) >>> + return; >>> + >>> + /* Handle V4 additions */ >>> + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && >>> + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V4_OUT_SUCFW_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >>> + buf); >>> + } >>> + >>> + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) >>> + return; >>> + >>> + /* Handle V5 additions */ >>> + >>> + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V5_OUT_BOARD_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >>> + buf); >>> + } >>> + >>> + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { >>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>> + GET_VERSION_V5_OUT_BUNDLE_VERSION); >>> + >>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>> + le32_to_cpu(ver.dwords[2]), >>> + le32_to_cpu(ver.dwords[3])); >>> + >>> + devlink_info_version_running_put(req, >>> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, >>> + buf); >>> + } >>> +} >>> + >>> +#undef EFX_VER_FLAG >>> + >>> +static void efx_devlink_info_query_all(struct efx_nic *efx, >>> + struct devlink_info_req *req) >>> +{ >>> + efx_devlink_info_board_cfg(efx, req); >>> + efx_devlink_info_stored_versions(efx, req); >>> + efx_devlink_info_running_versions(efx, req); >>> +} >>> + >>> +struct efx_devlink { >>> + struct efx_nic *efx; >>> +}; >>> + >>> +static int efx_devlink_info_get(struct devlink *devlink, >>> + struct devlink_info_req *req, >>> + struct netlink_ext_ack *extack) >>> +{ >>> + struct efx_devlink *devlink_private = devlink_priv(devlink); >>> + struct efx_nic *efx = devlink_private->efx; >>> + >>> + efx_devlink_info_query_all(efx, req); >> I don't understand the reason for having efx_devlink_info_query_all() as >> a separate function in compare to inline its code here. >> > >Yes, it could be do as you say. I'll do. > >Thanks > >>> + return 0; >>> +} >>> + >>> +static const struct devlink_ops sfc_devlink_ops = { >>> + .info_get = efx_devlink_info_get, >>> +}; >>> + >>> +void efx_fini_devlink(struct efx_nic *efx) >>> +{ >>> + if (efx->devlink) { >>> + struct efx_devlink *devlink_private; >>> + >>> + devlink_private = devlink_priv(efx->devlink); >>> + >>> + devlink_unregister(efx->devlink); >>> + devlink_free(efx->devlink); >>> + efx->devlink = NULL; >>> + } >>> +} >>> + >>> +int efx_probe_devlink(struct efx_nic *efx) >>> +{ >>> + struct efx_devlink *devlink_private; >>> + >>> + efx->devlink = devlink_alloc(&sfc_devlink_ops, >>> + sizeof(struct efx_devlink), >>> + &efx->pci_dev->dev); >>> + if (!efx->devlink) >>> + return -ENOMEM; >>> + devlink_private = devlink_priv(efx->devlink); >>> + devlink_private->efx = efx; >>> + >>> + devlink_register(efx->devlink); >>> + >>> + return 0; >>> +} >>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h >>> new file mode 100644 >>> index 000000000000..997f878aea93 >>> --- /dev/null >>> +++ b/drivers/net/ethernet/sfc/efx_devlink.h >>> @@ -0,0 +1,20 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/**************************************************************************** >>> + * Driver for AMD network controllers and boards >>> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms of the GNU General Public License version 2 as published >>> + * by the Free Software Foundation, incorporated herein by reference. >>> + */ >>> + >>> +#ifndef _EFX_DEVLINK_H >>> +#define _EFX_DEVLINK_H >>> + >>> +#include "net_driver.h" >>> +#include <net/devlink.h> >>> + >>> +int efx_probe_devlink(struct efx_nic *efx); >>> +void efx_fini_devlink(struct efx_nic *efx); >>> + >>> +#endif /* _EFX_DEVLINK_H */ >>> diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c >>> index af338208eae9..328cae82a7d8 100644 >>> --- a/drivers/net/ethernet/sfc/mcdi.c >>> +++ b/drivers/net/ethernet/sfc/mcdi.c >>> @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) >>> return rc; >>> } >>> >>> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >>> + u32 *subtype, u16 version[4], char *desc, >>> + size_t descsize) >>> +{ >>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); >>> + efx_dword_t *outbuf; >>> + size_t outlen; >>> + u32 flags; >>> + int rc; >>> + >>> + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); >>> + if (!outbuf) >>> + return -ENOMEM; >>> + >>> + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); >>> + >>> + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, >>> + sizeof(inbuf), outbuf, >>> + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, >>> + &outlen); >>> + if (rc) >>> + goto out_free; >>> + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { >>> + rc = -EIO; >>> + goto out_free; >>> + } >>> + >>> + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); >>> + >>> + if (desc && descsize > 0) { >>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { >>> + if (descsize <= >>> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { >>> + rc = -E2BIG; >>> + goto out_free; >>> + } >>> + >>> + strncpy(desc, >>> + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), >>> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); >>> + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; >>> + } else { >>> + desc[0] = '\0'; >>> + } >>> + } >>> + >>> + if (subtype) { >>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) >>> + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); >>> + else >>> + *subtype = 0; >>> + } >>> + >>> + if (version) { >>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { >>> + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); >>> + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); >>> + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); >>> + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); >>> + } else { >>> + version[0] = 0; >>> + version[1] = 0; >>> + version[2] = 0; >>> + version[3] = 0; >>> + } >>> + } >>> + >>> +out_free: >>> + kfree(outbuf); >>> + return rc; >>> +} >>> + >>> int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, >>> size_t len, size_t *retlen, u8 *buffer) >>> { >>> diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h >>> index 7e35fec9da35..63b090587f7a 100644 >>> --- a/drivers/net/ethernet/sfc/mcdi.h >>> +++ b/drivers/net/ethernet/sfc/mcdi.h >>> @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, >>> bool *protected_out); >>> int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); >>> int efx_mcdi_nvram_test_all(struct efx_nic *efx); >>> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >>> + u32 *subtype, u16 version[4], char *desc, >>> + size_t descsize); >>> int efx_mcdi_handle_assertion(struct efx_nic *efx); >>> int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); >>> int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, >>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >>> index 3b49e216768b..d036641dc043 100644 >>> --- a/drivers/net/ethernet/sfc/net_driver.h >>> +++ b/drivers/net/ethernet/sfc/net_driver.h >>> @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { >>> * xdp_rxq_info structures? >>> * @netdev_notifier: Netdevice notifier. >>> * @tc: state for TC offload (EF100). >>> + * @devlink: reference to devlink structure owned by this device >>> * @mem_bar: The BAR that is mapped into membase. >>> * @reg_base: Offset from the start of the bar to the function control window. >>> * @monitor_work: Hardware monitor workitem >>> @@ -1179,6 +1180,7 @@ struct efx_nic { >>> struct notifier_block netdev_notifier; >>> struct efx_tc_state *tc; >>> >>> + struct devlink *devlink; >>> unsigned int mem_bar; >>> u32 reg_base; >>> >>> -- >>> 2.17.1 >>> >
Thu, Jan 19, 2023 at 05:29:37PM CET, alejandro.lucero-palau@amd.com wrote: > >On 1/19/23 14:51, Lucero Palau, Alejandro wrote: >> On 1/19/23 12:14, Jiri Pirko wrote: >>> Thu, Jan 19, 2023 at 12:31:34PM CET, alejandro.lucero-palau@amd.com wrote: >>>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>> >>>> Basic support for devlink info command. >>>> >>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>> --- >>>> drivers/net/ethernet/sfc/Kconfig | 1 + >>>> drivers/net/ethernet/sfc/Makefile | 3 +- >>>> drivers/net/ethernet/sfc/ef100_nic.c | 6 + >>>> drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ >>>> drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ >>>> drivers/net/ethernet/sfc/mcdi.c | 72 +++++ >>>> drivers/net/ethernet/sfc/mcdi.h | 3 + >>>> drivers/net/ethernet/sfc/net_driver.h | 2 + >>>> 8 files changed, 533 insertions(+), 1 deletion(-) >>>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c >>>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h >>> Could you please split? >>> >>> At least the devlink introduction part and info implementation. >>> >> Sure. > >Just for being sure, would it be fine to have a simple info >implementation first along with the devlink infrastructure then a second >patch adding the more complex devlink info support? I guess I need at >least one operation supported initially. No, it does not need to support any op. > >>>> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig >>>> index 0950e6b0508f..4af36ba8906b 100644 >>>> --- a/drivers/net/ethernet/sfc/Kconfig >>>> +++ b/drivers/net/ethernet/sfc/Kconfig >>>> @@ -22,6 +22,7 @@ config SFC >>>> depends on PTP_1588_CLOCK_OPTIONAL >>>> select MDIO >>>> select CRC32 >>>> + select NET_DEVLINK >>>> help >>>> This driver supports 10/40-gigabit Ethernet cards based on >>>> the Solarflare SFC9100-family controllers. >>>> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >>>> index 712a48d00069..55b9c73cd8ef 100644 >>>> --- a/drivers/net/ethernet/sfc/Makefile >>>> +++ b/drivers/net/ethernet/sfc/Makefile >>>> @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ >>>> mcdi.o mcdi_port.o mcdi_port_common.o \ >>>> mcdi_functions.o mcdi_filters.o mcdi_mon.o \ >>>> ef100.o ef100_nic.o ef100_netdev.o \ >>>> - ef100_ethtool.o ef100_rx.o ef100_tx.o >>>> + ef100_ethtool.o ef100_rx.o ef100_tx.o \ >>>> + efx_devlink.o >>>> sfc-$(CONFIG_SFC_MTD) += mtd.o >>>> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ >>>> mae.o tc.o tc_bindings.o tc_counters.o >>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>>> index ad686c671ab8..14af8f314b83 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>>> @@ -27,6 +27,7 @@ >>>> #include "tc.h" >>>> #include "mae.h" >>>> #include "rx_common.h" >>>> +#include "efx_devlink.h" >>>> >>>> #define EF100_MAX_VIS 4096 >>>> #define EF100_NUM_MCDI_BUFFERS 1 >>>> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >>>> netif_warn(efx, probe, net_dev, >>>> "Failed to probe base mport rc %d; representors will not function\n", >>>> rc); >>>> + } else { >>>> + if (efx_probe_devlink(efx)) >>> I don't understand why the devlink register call is here. >>> 1) You can registers it for PF and VF. I don't follow why you do it only >>> for PF. >> We discuss the possibility of creating the devlink interface for VFs, >> but we decided not to. Arguably, this should not be available for VFs >> owned by VMs/containers, or at least in our case, since the interface is >> being used for getting information about the whole device or for >> getting/setting the MAC address. We plan to support more devlink >> functionalities in the near future, so maybe we add such support then if >> we consider it is needed. >> >>> 2) It should be done as the first thing in the probe flow. Then devlink >>> port register, only after that netdev. It makes sense from the >>> perspective of object hierarchy. >> I can not see a problem here. It is not the first thing done in the >> probe function, but I can not see any dependency for being so. And it is >> done before the devlink port registration and before the netdev >> registration what seems to be what other drivers do. What am I missing >> here? >> >>> It is also usual to have the devlink priv as the "main" driver >>> structure storage, see how that is done in mlx5 of mlxsw for example. >> I will check the way others drivers use the devlink priv. > > >Interestingly, there are 26 calls to devlink_alloc with half of them >using the main driver structure and the other half using a specific >devlink private struct. This is a huge responsibility now for me! > >I'm afraid, for the shake of having this merged as soon as possible, >I'll keep the current allocation. I did not suffer any problem with this >approach while implementing the current support. If, for the further >devlink support we plan to add, it turns out to be an issue, we will to >the changes then. I would say it is a main change now or then, and >current work does not justify it now. Up to you, it will eventualy bite you in the *** when you implement devlink reload for example. > > >>> >>>> + netif_warn(efx, probe, net_dev, >>>> + "Failed to register devlink\n"); >>>> } >>>> >>>> rc = efx_init_tc(efx); >>>> @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) >>>> { >>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>> >>>> + efx_fini_devlink(efx); >>>> efx_mcdi_detach(efx); >>>> efx_mcdi_fini(efx); >>>> if (nic_data) >>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>>> new file mode 100644 >>>> index 000000000000..c506f8f35d25 >>>> --- /dev/null >>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>>> @@ -0,0 +1,427 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-only >>>> +/**************************************************************************** >>>> + * Driver for AMD network controllers and boards >>>> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify it >>>> + * under the terms of the GNU General Public License version 2 as published >>>> + * by the Free Software Foundation, incorporated herein by reference. >>>> + */ >>>> + >>>> +#include <linux/rtc.h> >>>> +#include "net_driver.h" >>>> +#include "ef100_nic.h" >>>> +#include "efx_devlink.h" >>>> +#include "nic.h" >>>> +#include "mcdi.h" >>>> +#include "mcdi_functions.h" >>>> +#include "mcdi_pcol.h" >>>> + >>>> +/* Custom devlink-info version object names for details that do not map to the >>>> + * generic standardized names. >>>> + */ >>>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >>>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >>>> +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >>>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >>>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >>>> +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >>>> +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >>>> +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >>>> +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >>>> +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >>>> +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" >>>> + >>>> +#define EFX_MAX_VERSION_INFO_LEN 64 >>>> + >>>> +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, >>>> + struct devlink_info_req *req, >>>> + unsigned int partition_type, >>>> + const char *version_name) >>>> +{ >>>> + char buf[EFX_MAX_VERSION_INFO_LEN]; >>>> + u16 version[4]; >>>> + int rc; >>>> + >>>> + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, >>>> + 0); >>>> + if (rc) >>>> + return rc; >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], >>>> + version[1], version[2], version[3]); >>>> + devlink_info_version_stored_put(req, version_name, buf); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void efx_devlink_info_stored_versions(struct efx_nic *efx, >>>> + struct devlink_info_req *req) >>>> +{ >>>> + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, >>>> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); >>>> + efx_devlink_info_nvram_partition(efx, req, >>>> + NVRAM_PARTITION_TYPE_MC_FIRMWARE, >>>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); >>>> + efx_devlink_info_nvram_partition(efx, req, >>>> + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, >>>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); >>>> + efx_devlink_info_nvram_partition(efx, req, >>>> + NVRAM_PARTITION_TYPE_EXPANSION_ROM, >>>> + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); >>>> + efx_devlink_info_nvram_partition(efx, req, >>>> + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, >>>> + EFX_DEVLINK_INFO_VERSION_FW_UEFI); >>>> +} >>>> + >>>> +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) >>>> + >>>> +static void efx_devlink_info_board_cfg(struct efx_nic *efx, >>>> + struct devlink_info_req *req) >>>> +{ >>>> + char sn[EFX_MAX_SERIALNUM_LEN]; >>>> + u8 mac_address[ETH_ALEN]; >>>> + int rc; >>>> + >>>> + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); >>>> + if (!rc) { >>>> + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); >>>> + devlink_info_serial_number_put(req, sn); >>>> + } >>>> +} >>>> + >>>> +#define EFX_VER_FLAG(_f) \ >>>> + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) >>>> + >>>> +static void efx_devlink_info_running_versions(struct efx_nic *efx, >>>> + struct devlink_info_req *req) >>>> +{ >>>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); >>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); >>>> + char buf[EFX_MAX_VERSION_INFO_LEN]; >>>> + unsigned int flags, build_id; >>>> + union { >>>> + const __le32 *dwords; >>>> + const __le16 *words; >>>> + const char *str; >>>> + } ver; >>>> + struct rtc_time build_date; >>>> + size_t outlength, offset; >>>> + u64 tstamp; >>>> + int rc; >>>> + >>>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), >>>> + outbuf, sizeof(outbuf), &outlength); >>>> + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) >>>> + return; >>>> + >>>> + /* Handle previous output */ >>>> + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { >>>> + ver.words = (__le16 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_EXT_OUT_VERSION); >>>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le16_to_cpu(ver.words[0]), >>>> + le16_to_cpu(ver.words[1]), >>>> + le16_to_cpu(ver.words[2]), >>>> + le16_to_cpu(ver.words[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >>>> + buf); >>>> + return; >>>> + } >>>> + >>>> + /* Handle V2 additions */ >>>> + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); >>>> + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", >>>> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); >>>> + devlink_info_version_fixed_put(req, >>>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, >>>> + buf); >>>> + >>>> + /* Favour full board version if present (in V5 or later) */ >>>> + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", >>>> + MCDI_DWORD(outbuf, >>>> + GET_VERSION_V2_OUT_BOARD_REVISION)); >>>> + devlink_info_version_fixed_put(req, >>>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >>>> + buf); >>>> + } >>>> + >>>> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); >>>> + if (ver.str[0]) >>>> + devlink_info_board_serial_number_put(req, ver.str); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V2_OUT_FPGA_VERSION); >>>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", >>>> + le32_to_cpu(ver.dwords[0]), >>>> + 'A' + le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2])); >>>> + >>>> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); >>>> + if (ver.str[0]) >>>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>>> + " (%s)", ver.str); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_FPGA_REV, >>>> + buf); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V2_OUT_CMCFW_VERSION); >>>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), >>>> + le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + tstamp = MCDI_QWORD(outbuf, >>>> + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); >>>> + if (tstamp) { >>>> + rtc_time64_to_tm(tstamp, &build_date); >>>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>>> + " (%ptRd)", &build_date); >>>> + } >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, >>>> + buf); >>>> + } >>>> + >>>> + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); >>>> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), >>>> + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); >>>> + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { >>>> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); >>>> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >>>> + " (%x) %s", build_id, >>>> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); >>>> + } >>>> + devlink_info_version_running_put(req, >>>> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >>>> + buf); >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V2_OUT_SUCFW_VERSION); >>>> + tstamp = MCDI_QWORD(outbuf, >>>> + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); >>>> + rtc_time64_to_tm(tstamp, &build_date); >>>> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, >>>> + "%u.%u.%u.%u type %x (%ptRd)", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), >>>> + build_id, &build_date); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >>>> + buf); >>>> + } >>>> + >>>> + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) >>>> + return; >>>> + >>>> + /* Handle V3 additions */ >>>> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, >>>> + buf); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, >>>> + buf); >>>> + } >>>> + >>>> + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) >>>> + return; >>>> + >>>> + /* Handle V4 additions */ >>>> + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, >>>> + buf); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, >>>> + buf); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, >>>> + buf); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, >>>> + buf); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && >>>> + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V4_OUT_SUCFW_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >>>> + buf); >>>> + } >>>> + >>>> + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) >>>> + return; >>>> + >>>> + /* Handle V5 additions */ >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V5_OUT_BOARD_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >>>> + buf); >>>> + } >>>> + >>>> + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { >>>> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >>>> + GET_VERSION_V5_OUT_BUNDLE_VERSION); >>>> + >>>> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >>>> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >>>> + le32_to_cpu(ver.dwords[2]), >>>> + le32_to_cpu(ver.dwords[3])); >>>> + >>>> + devlink_info_version_running_put(req, >>>> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, >>>> + buf); >>>> + } >>>> +} >>>> + >>>> +#undef EFX_VER_FLAG >>>> + >>>> +static void efx_devlink_info_query_all(struct efx_nic *efx, >>>> + struct devlink_info_req *req) >>>> +{ >>>> + efx_devlink_info_board_cfg(efx, req); >>>> + efx_devlink_info_stored_versions(efx, req); >>>> + efx_devlink_info_running_versions(efx, req); >>>> +} >>>> + >>>> +struct efx_devlink { >>>> + struct efx_nic *efx; >>>> +}; >>>> + >>>> +static int efx_devlink_info_get(struct devlink *devlink, >>>> + struct devlink_info_req *req, >>>> + struct netlink_ext_ack *extack) >>>> +{ >>>> + struct efx_devlink *devlink_private = devlink_priv(devlink); >>>> + struct efx_nic *efx = devlink_private->efx; >>>> + >>>> + efx_devlink_info_query_all(efx, req); >>> I don't understand the reason for having efx_devlink_info_query_all() as >>> a separate function in compare to inline its code here. >>> >> Yes, it could be do as you say. I'll do. >> >> Thanks >> >>>> + return 0; >>>> +} >>>> + >>>> +static const struct devlink_ops sfc_devlink_ops = { >>>> + .info_get = efx_devlink_info_get, >>>> +}; >>>> + >>>> +void efx_fini_devlink(struct efx_nic *efx) >>>> +{ >>>> + if (efx->devlink) { >>>> + struct efx_devlink *devlink_private; >>>> + >>>> + devlink_private = devlink_priv(efx->devlink); >>>> + >>>> + devlink_unregister(efx->devlink); >>>> + devlink_free(efx->devlink); >>>> + efx->devlink = NULL; >>>> + } >>>> +} >>>> + >>>> +int efx_probe_devlink(struct efx_nic *efx) >>>> +{ >>>> + struct efx_devlink *devlink_private; >>>> + >>>> + efx->devlink = devlink_alloc(&sfc_devlink_ops, >>>> + sizeof(struct efx_devlink), >>>> + &efx->pci_dev->dev); >>>> + if (!efx->devlink) >>>> + return -ENOMEM; >>>> + devlink_private = devlink_priv(efx->devlink); >>>> + devlink_private->efx = efx; >>>> + >>>> + devlink_register(efx->devlink); >>>> + >>>> + return 0; >>>> +} >>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h >>>> new file mode 100644 >>>> index 000000000000..997f878aea93 >>>> --- /dev/null >>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.h >>>> @@ -0,0 +1,20 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>> +/**************************************************************************** >>>> + * Driver for AMD network controllers and boards >>>> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify it >>>> + * under the terms of the GNU General Public License version 2 as published >>>> + * by the Free Software Foundation, incorporated herein by reference. >>>> + */ >>>> + >>>> +#ifndef _EFX_DEVLINK_H >>>> +#define _EFX_DEVLINK_H >>>> + >>>> +#include "net_driver.h" >>>> +#include <net/devlink.h> >>>> + >>>> +int efx_probe_devlink(struct efx_nic *efx); >>>> +void efx_fini_devlink(struct efx_nic *efx); >>>> + >>>> +#endif /* _EFX_DEVLINK_H */ >>>> diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c >>>> index af338208eae9..328cae82a7d8 100644 >>>> --- a/drivers/net/ethernet/sfc/mcdi.c >>>> +++ b/drivers/net/ethernet/sfc/mcdi.c >>>> @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) >>>> return rc; >>>> } >>>> >>>> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >>>> + u32 *subtype, u16 version[4], char *desc, >>>> + size_t descsize) >>>> +{ >>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); >>>> + efx_dword_t *outbuf; >>>> + size_t outlen; >>>> + u32 flags; >>>> + int rc; >>>> + >>>> + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); >>>> + if (!outbuf) >>>> + return -ENOMEM; >>>> + >>>> + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); >>>> + >>>> + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, >>>> + sizeof(inbuf), outbuf, >>>> + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, >>>> + &outlen); >>>> + if (rc) >>>> + goto out_free; >>>> + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { >>>> + rc = -EIO; >>>> + goto out_free; >>>> + } >>>> + >>>> + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); >>>> + >>>> + if (desc && descsize > 0) { >>>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { >>>> + if (descsize <= >>>> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { >>>> + rc = -E2BIG; >>>> + goto out_free; >>>> + } >>>> + >>>> + strncpy(desc, >>>> + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), >>>> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); >>>> + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; >>>> + } else { >>>> + desc[0] = '\0'; >>>> + } >>>> + } >>>> + >>>> + if (subtype) { >>>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) >>>> + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); >>>> + else >>>> + *subtype = 0; >>>> + } >>>> + >>>> + if (version) { >>>> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { >>>> + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); >>>> + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); >>>> + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); >>>> + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); >>>> + } else { >>>> + version[0] = 0; >>>> + version[1] = 0; >>>> + version[2] = 0; >>>> + version[3] = 0; >>>> + } >>>> + } >>>> + >>>> +out_free: >>>> + kfree(outbuf); >>>> + return rc; >>>> +} >>>> + >>>> int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, >>>> size_t len, size_t *retlen, u8 *buffer) >>>> { >>>> diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h >>>> index 7e35fec9da35..63b090587f7a 100644 >>>> --- a/drivers/net/ethernet/sfc/mcdi.h >>>> +++ b/drivers/net/ethernet/sfc/mcdi.h >>>> @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, >>>> bool *protected_out); >>>> int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); >>>> int efx_mcdi_nvram_test_all(struct efx_nic *efx); >>>> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >>>> + u32 *subtype, u16 version[4], char *desc, >>>> + size_t descsize); >>>> int efx_mcdi_handle_assertion(struct efx_nic *efx); >>>> int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); >>>> int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, >>>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >>>> index 3b49e216768b..d036641dc043 100644 >>>> --- a/drivers/net/ethernet/sfc/net_driver.h >>>> +++ b/drivers/net/ethernet/sfc/net_driver.h >>>> @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { >>>> * xdp_rxq_info structures? >>>> * @netdev_notifier: Netdevice notifier. >>>> * @tc: state for TC offload (EF100). >>>> + * @devlink: reference to devlink structure owned by this device >>>> * @mem_bar: The BAR that is mapped into membase. >>>> * @reg_base: Offset from the start of the bar to the function control window. >>>> * @monitor_work: Hardware monitor workitem >>>> @@ -1179,6 +1180,7 @@ struct efx_nic { >>>> struct notifier_block netdev_notifier; >>>> struct efx_tc_state *tc; >>>> >>>> + struct devlink *devlink; >>>> unsigned int mem_bar; >>>> u32 reg_base; >>>> >>>> -- >>>> 2.17.1 >>>> >
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/alejandro-lucero-palau-amd-com/sfc-add-devlink-support-for-ef100/20230119-193440
patch link: https://lore.kernel.org/r/20230119113140.20208-2-alejandro.lucero-palau%40amd.com
patch subject: [PATCH net-next 1/7] sfc: add devlink support for ef100
config: i386-randconfig-a013-20221205 (https://download.01.org/0day-ci/archive/20230120/202301202124.sfqty0op-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/f393f2642abb0e7536df6f9f08e0d69ecfd5b435
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review alejandro-lucero-palau-amd-com/sfc-add-devlink-support-for-ef100/20230119-193440
git checkout f393f2642abb0e7536df6f9f08e0d69ecfd5b435
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "efx_mcdi_nvram_metadata" [drivers/net/ethernet/sfc/sfc.ko] undefined!
On 1/19/23 23:40, Jacob Keller wrote: > > On 1/19/2023 3:31 AM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> >> Basic support for devlink info command. >> >> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> --- >> drivers/net/ethernet/sfc/Kconfig | 1 + >> drivers/net/ethernet/sfc/Makefile | 3 +- >> drivers/net/ethernet/sfc/ef100_nic.c | 6 + >> drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ >> drivers/net/ethernet/sfc/mcdi.c | 72 +++++ >> drivers/net/ethernet/sfc/mcdi.h | 3 + >> drivers/net/ethernet/sfc/net_driver.h | 2 + >> 8 files changed, 533 insertions(+), 1 deletion(-) >> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c >> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h >> >> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig >> index 0950e6b0508f..4af36ba8906b 100644 >> --- a/drivers/net/ethernet/sfc/Kconfig >> +++ b/drivers/net/ethernet/sfc/Kconfig >> @@ -22,6 +22,7 @@ config SFC >> depends on PTP_1588_CLOCK_OPTIONAL >> select MDIO >> select CRC32 >> + select NET_DEVLINK >> help >> This driver supports 10/40-gigabit Ethernet cards based on >> the Solarflare SFC9100-family controllers. >> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >> index 712a48d00069..55b9c73cd8ef 100644 >> --- a/drivers/net/ethernet/sfc/Makefile >> +++ b/drivers/net/ethernet/sfc/Makefile >> @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ >> mcdi.o mcdi_port.o mcdi_port_common.o \ >> mcdi_functions.o mcdi_filters.o mcdi_mon.o \ >> ef100.o ef100_nic.o ef100_netdev.o \ >> - ef100_ethtool.o ef100_rx.o ef100_tx.o >> + ef100_ethtool.o ef100_rx.o ef100_tx.o \ >> + efx_devlink.o >> sfc-$(CONFIG_SFC_MTD) += mtd.o >> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ >> mae.o tc.o tc_bindings.o tc_counters.o >> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >> index ad686c671ab8..14af8f314b83 100644 >> --- a/drivers/net/ethernet/sfc/ef100_nic.c >> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >> @@ -27,6 +27,7 @@ >> #include "tc.h" >> #include "mae.h" >> #include "rx_common.h" >> +#include "efx_devlink.h" >> >> #define EF100_MAX_VIS 4096 >> #define EF100_NUM_MCDI_BUFFERS 1 >> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >> netif_warn(efx, probe, net_dev, >> "Failed to probe base mport rc %d; representors will not function\n", >> rc); >> + } else { >> + if (efx_probe_devlink(efx)) >> + netif_warn(efx, probe, net_dev, >> + "Failed to register devlink\n"); >> } >> > A bit of a weird construction here with the next step in an else block? > I guess this is being treated as an optional feature, and depends on > efx_ef100_get_base_mport succeeding? Right. The mae ports initialization can fail but the driver can still initialize the device with limited functionality. > > It reads a little awkward, but I can't think of a better way to arrange > this unless another helper function was used to combine these two calls > in order to avoid the extra indentation. > I could add some check at the beginning of those functions and returning doing nothing if the checks fails. That would avoid this ugliness. >> rc = efx_init_tc(efx); >> @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) >> { >> struct ef100_nic_data *nic_data = efx->nic_data; >> >> + efx_fini_devlink(efx); >> efx_mcdi_detach(efx); >> efx_mcdi_fini(efx); >> if (nic_data) >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >> new file mode 100644 >> index 000000000000..c506f8f35d25 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >> @@ -0,0 +1,427 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/**************************************************************************** >> + * Driver for AMD network controllers and boards >> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation, incorporated herein by reference. >> + */ >> + >> +#include <linux/rtc.h> >> +#include "net_driver.h" >> +#include "ef100_nic.h" >> +#include "efx_devlink.h" >> +#include "nic.h" >> +#include "mcdi.h" >> +#include "mcdi_functions.h" >> +#include "mcdi_pcol.h" >> + >> +/* Custom devlink-info version object names for details that do not map to the >> + * generic standardized names. >> + */ >> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >> +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >> +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >> +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >> +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" > Here, you've defined several new versions, only one of which is > standard. I don't see an associated documentation addition. Please add a > ef100.rst file to Documentation/networking/devlink > > It is also preferred to use the standard names or extend the set of > standard names where appropriate first. Can you explain why you didn't > do that here? > > For example, the documentation for "fw.undi" indicates it may include > the UEFI driver, and I would expect your UEFI version to be something > like fw.undi or fw.undi.uefi if its distinct... OK. I'll add that doc file in the next version and use those standard definitions when possible. >> + >> +#define EFX_MAX_VERSION_INFO_LEN 64 >> + >> +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, >> + struct devlink_info_req *req, >> + unsigned int partition_type, >> + const char *version_name) >> +{ >> + char buf[EFX_MAX_VERSION_INFO_LEN]; >> + u16 version[4]; >> + int rc; >> + >> + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, >> + 0); >> + if (rc) >> + return rc; >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], >> + version[1], version[2], version[3]); >> + devlink_info_version_stored_put(req, version_name, buf); >> + >> + return 0; >> +} >> + >> +static void efx_devlink_info_stored_versions(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, >> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_MC_FIRMWARE, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_EXPANSION_ROM, >> + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, >> + EFX_DEVLINK_INFO_VERSION_FW_UEFI); >> +} >> + >> +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) >> + >> +static void efx_devlink_info_board_cfg(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + char sn[EFX_MAX_SERIALNUM_LEN]; >> + u8 mac_address[ETH_ALEN]; >> + int rc; >> + >> + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); >> + if (!rc) { >> + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); >> + devlink_info_serial_number_put(req, sn); >> + } >> +} >> + >> +#define EFX_VER_FLAG(_f) \ >> + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) >> + >> +static void efx_devlink_info_running_versions(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ > This function for doing running versions is very long, and seems quite > complicated. It's difficult to parse what versions are being reported. > > I saw that your stored version used a helper function to separate each > thing out nicely. Is there any way to do that here? > OK. I'll split it up based on the handling per version. >> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); >> + char buf[EFX_MAX_VERSION_INFO_LEN]; >> + unsigned int flags, build_id; >> + union { >> + const __le32 *dwords; >> + const __le16 *words; >> + const char *str; >> + } ver; > For example, you've got this local union on the stack that seems to be > reused a bunch... > >> + struct rtc_time build_date; >> + size_t outlength, offset; >> + u64 tstamp; >> + int rc; >> + >> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), >> + outbuf, sizeof(outbuf), &outlength); >> + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) >> + return; >> + >> + /* Handle previous output */ >> + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { >> + ver.words = (__le16 *)MCDI_PTR(outbuf, >> + GET_VERSION_EXT_OUT_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le16_to_cpu(ver.words[0]), >> + le16_to_cpu(ver.words[1]), >> + le16_to_cpu(ver.words[2]), >> + le16_to_cpu(ver.words[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >> + buf); >> + return; >> + } >> + >> + /* Handle V2 additions */ >> + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); >> + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", >> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); >> + devlink_info_version_fixed_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, >> + buf); >> + >> + /* Favour full board version if present (in V5 or later) */ >> + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", >> + MCDI_DWORD(outbuf, >> + GET_VERSION_V2_OUT_BOARD_REVISION)); >> + devlink_info_version_fixed_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >> + buf); >> + } >> + >> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); >> + if (ver.str[0]) >> + devlink_info_board_serial_number_put(req, ver.str); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_FPGA_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", >> + le32_to_cpu(ver.dwords[0]), >> + 'A' + le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); >> + if (ver.str[0]) >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%s)", ver.str); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FPGA_REV, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_CMCFW_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), >> + le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + tstamp = MCDI_QWORD(outbuf, >> + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); >> + if (tstamp) { >> + rtc_time64_to_tm(tstamp, &build_date); >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%ptRd)", &build_date); >> + } >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, >> + buf); >> + } >> + >> + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), >> + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); >> + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { >> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%x) %s", build_id, >> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); >> + } >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >> + buf); >> + >> + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_SUCFW_VERSION); >> + tstamp = MCDI_QWORD(outbuf, >> + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); >> + rtc_time64_to_tm(tstamp, &build_date); >> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, >> + "%u.%u.%u.%u type %x (%ptRd)", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), >> + build_id, &build_date); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) >> + return; >> + >> + /* Handle V3 additions */ >> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) >> + return; >> + >> + /* Handle V4 additions */ >> + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && >> + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SUCFW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) >> + return; >> + >> + /* Handle V5 additions */ >> + >> + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V5_OUT_BOARD_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V5_OUT_BUNDLE_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, >> + buf); >> + } >> +} > In the ice driver I implemented each version extraction as a separate > function to make it more clear which versions where which. This running > function could benefit from some organization like that. > >> + >> +#undef EFX_VER_FLAG >> + >> +static void efx_devlink_info_query_all(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + efx_devlink_info_board_cfg(efx, req); >> + efx_devlink_info_stored_versions(efx, req); >> + efx_devlink_info_running_versions(efx, req); >> +} > Do you really need this to be a separate function and not just inlined > into the caller, efx_devlink_info_get. That was also pointed out by Jiri Pirko and I agree this is not necessary. >> + >> +struct efx_devlink { >> + struct efx_nic *efx; >> +}; >> + >> +static int efx_devlink_info_get(struct devlink *devlink, >> + struct devlink_info_req *req, >> + struct netlink_ext_ack *extack) >> +{ >> + struct efx_devlink *devlink_private = devlink_priv(devlink); >> + struct efx_nic *efx = devlink_private->efx; >> + >> + efx_devlink_info_query_all(efx, req); >> + return 0; >> +} >> + >> +static const struct devlink_ops sfc_devlink_ops = { >> + .info_get = efx_devlink_info_get, >> +}; >> + >> +void efx_fini_devlink(struct efx_nic *efx) >> +{ >> + if (efx->devlink) { >> + struct efx_devlink *devlink_private; >> + >> + devlink_private = devlink_priv(efx->devlink); >> + >> + devlink_unregister(efx->devlink); >> + devlink_free(efx->devlink); >> + efx->devlink = NULL; >> + } >> +} >> + >> +int efx_probe_devlink(struct efx_nic *efx) >> +{ >> + struct efx_devlink *devlink_private; >> + >> + efx->devlink = devlink_alloc(&sfc_devlink_ops, >> + sizeof(struct efx_devlink), >> + &efx->pci_dev->dev); >> + if (!efx->devlink) >> + return -ENOMEM; >> + devlink_private = devlink_priv(efx->devlink); >> + devlink_private->efx = efx; >> + >> + devlink_register(efx->devlink); >> + > So drivers implementing devlink typically would allocate the devlink > early (almost the first thing it does) and make its private field > something like their main private data structure. > > Obviously you're adding this to a pre-existing driver and it is > initially simpler and easier to keep it separate. I'm not sure what > other reviewers' stance on this is, but I think its preferable to > allocate as an early stage and register once the driver is ready to > accept requests from user space. > > As for a reason why, implementing reload support effectively requires this. > We have discussed this internally and we do not plan to support reload. >> + return 0; >> +} >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h >> new file mode 100644 >> index 000000000000..997f878aea93 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_devlink.h >> @@ -0,0 +1,20 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/**************************************************************************** >> + * Driver for AMD network controllers and boards >> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation, incorporated herein by reference. >> + */ >> + >> +#ifndef _EFX_DEVLINK_H >> +#define _EFX_DEVLINK_H >> + >> +#include "net_driver.h" >> +#include <net/devlink.h> >> + >> +int efx_probe_devlink(struct efx_nic *efx); >> +void efx_fini_devlink(struct efx_nic *efx); >> + >> +#endif /* _EFX_DEVLINK_H */ >> diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c >> index af338208eae9..328cae82a7d8 100644 >> --- a/drivers/net/ethernet/sfc/mcdi.c >> +++ b/drivers/net/ethernet/sfc/mcdi.c >> @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) >> return rc; >> } >> >> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >> + u32 *subtype, u16 version[4], char *desc, >> + size_t descsize) >> +{ >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); >> + efx_dword_t *outbuf; >> + size_t outlen; >> + u32 flags; >> + int rc; >> + >> + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); >> + if (!outbuf) >> + return -ENOMEM; >> + >> + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); >> + >> + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, >> + sizeof(inbuf), outbuf, >> + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, >> + &outlen); >> + if (rc) >> + goto out_free; >> + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { >> + rc = -EIO; >> + goto out_free; >> + } >> + >> + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); >> + >> + if (desc && descsize > 0) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { >> + if (descsize <= >> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { >> + rc = -E2BIG; >> + goto out_free; >> + } >> + >> + strncpy(desc, >> + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), >> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); >> + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; >> + } else { >> + desc[0] = '\0'; >> + } >> + } >> + >> + if (subtype) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) >> + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); >> + else >> + *subtype = 0; >> + } >> + >> + if (version) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { >> + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); >> + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); >> + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); >> + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); >> + } else { >> + version[0] = 0; >> + version[1] = 0; >> + version[2] = 0; >> + version[3] = 0; >> + } >> + } >> + >> +out_free: >> + kfree(outbuf); >> + return rc; >> +} >> + >> int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, >> size_t len, size_t *retlen, u8 *buffer) >> { >> diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h >> index 7e35fec9da35..63b090587f7a 100644 >> --- a/drivers/net/ethernet/sfc/mcdi.h >> +++ b/drivers/net/ethernet/sfc/mcdi.h >> @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, >> bool *protected_out); >> int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); >> int efx_mcdi_nvram_test_all(struct efx_nic *efx); >> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >> + u32 *subtype, u16 version[4], char *desc, >> + size_t descsize); >> int efx_mcdi_handle_assertion(struct efx_nic *efx); >> int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); >> int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, >> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >> index 3b49e216768b..d036641dc043 100644 >> --- a/drivers/net/ethernet/sfc/net_driver.h >> +++ b/drivers/net/ethernet/sfc/net_driver.h >> @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { >> * xdp_rxq_info structures? >> * @netdev_notifier: Netdevice notifier. >> * @tc: state for TC offload (EF100). >> + * @devlink: reference to devlink structure owned by this device >> * @mem_bar: The BAR that is mapped into membase. >> * @reg_base: Offset from the start of the bar to the function control window. >> * @monitor_work: Hardware monitor workitem >> @@ -1179,6 +1180,7 @@ struct efx_nic { >> struct notifier_block netdev_notifier; >> struct efx_tc_state *tc; >> >> + struct devlink *devlink; >> unsigned int mem_bar; >> u32 reg_base; >>
On 20/01/2023 14:11, Lucero Palau, Alejandro wrote: > > On 1/19/23 23:40, Jacob Keller wrote: >> >> On 1/19/2023 3:31 AM, alejandro.lucero-palau@amd.com wrote: >>> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >>> netif_warn(efx, probe, net_dev, >>> "Failed to probe base mport rc %d; representors will not function\n", >>> rc); >>> + } else { >>> + if (efx_probe_devlink(efx)) >>> + netif_warn(efx, probe, net_dev, >>> + "Failed to register devlink\n"); >>> } >>> >> A bit of a weird construction here with the next step in an else block? >> I guess this is being treated as an optional feature, and depends on >> efx_ef100_get_base_mport succeeding? > > Right. The mae ports initialization can fail but the driver can still > initialize the device with limited functionality. But in that case, we probably should support e.g. devlink info, even though without the MAE ports we can't support devlink port.
On 1/24/23 06:13, Edward Cree wrote: > On 20/01/2023 14:11, Lucero Palau, Alejandro wrote: >> On 1/19/23 23:40, Jacob Keller wrote: >>> On 1/19/2023 3:31 AM, alejandro.lucero-palau@amd.com wrote: >>>> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >>>> netif_warn(efx, probe, net_dev, >>>> "Failed to probe base mport rc %d; representors will not function\n", >>>> rc); >>>> + } else { >>>> + if (efx_probe_devlink(efx)) >>>> + netif_warn(efx, probe, net_dev, >>>> + "Failed to register devlink\n"); >>>> } >>>> >>> A bit of a weird construction here with the next step in an else block? >>> I guess this is being treated as an optional feature, and depends on >>> efx_ef100_get_base_mport succeeding? >> Right. The mae ports initialization can fail but the driver can still >> initialize the device with limited functionality. > But in that case, we probably should support e.g. devlink info, even > though without the MAE ports we can't support devlink port. Good point. I've already modified the code for avoiding the weirdness and this should not be hard to do. Thanks
On 1/19/23 23:40, Jacob Keller wrote: > > On 1/19/2023 3:31 AM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> >> Basic support for devlink info command. >> >> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> --- >> drivers/net/ethernet/sfc/Kconfig | 1 + >> drivers/net/ethernet/sfc/Makefile | 3 +- >> drivers/net/ethernet/sfc/ef100_nic.c | 6 + >> drivers/net/ethernet/sfc/efx_devlink.c | 427 +++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/efx_devlink.h | 20 ++ >> drivers/net/ethernet/sfc/mcdi.c | 72 +++++ >> drivers/net/ethernet/sfc/mcdi.h | 3 + >> drivers/net/ethernet/sfc/net_driver.h | 2 + >> 8 files changed, 533 insertions(+), 1 deletion(-) >> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c >> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h >> >> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig >> index 0950e6b0508f..4af36ba8906b 100644 >> --- a/drivers/net/ethernet/sfc/Kconfig >> +++ b/drivers/net/ethernet/sfc/Kconfig >> @@ -22,6 +22,7 @@ config SFC >> depends on PTP_1588_CLOCK_OPTIONAL >> select MDIO >> select CRC32 >> + select NET_DEVLINK >> help >> This driver supports 10/40-gigabit Ethernet cards based on >> the Solarflare SFC9100-family controllers. >> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >> index 712a48d00069..55b9c73cd8ef 100644 >> --- a/drivers/net/ethernet/sfc/Makefile >> +++ b/drivers/net/ethernet/sfc/Makefile >> @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ >> mcdi.o mcdi_port.o mcdi_port_common.o \ >> mcdi_functions.o mcdi_filters.o mcdi_mon.o \ >> ef100.o ef100_nic.o ef100_netdev.o \ >> - ef100_ethtool.o ef100_rx.o ef100_tx.o >> + ef100_ethtool.o ef100_rx.o ef100_tx.o \ >> + efx_devlink.o >> sfc-$(CONFIG_SFC_MTD) += mtd.o >> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ >> mae.o tc.o tc_bindings.o tc_counters.o >> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >> index ad686c671ab8..14af8f314b83 100644 >> --- a/drivers/net/ethernet/sfc/ef100_nic.c >> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >> @@ -27,6 +27,7 @@ >> #include "tc.h" >> #include "mae.h" >> #include "rx_common.h" >> +#include "efx_devlink.h" >> >> #define EF100_MAX_VIS 4096 >> #define EF100_NUM_MCDI_BUFFERS 1 >> @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) >> netif_warn(efx, probe, net_dev, >> "Failed to probe base mport rc %d; representors will not function\n", >> rc); >> + } else { >> + if (efx_probe_devlink(efx)) >> + netif_warn(efx, probe, net_dev, >> + "Failed to register devlink\n"); >> } >> > A bit of a weird construction here with the next step in an else block? > I guess this is being treated as an optional feature, and depends on > efx_ef100_get_base_mport succeeding? > > It reads a little awkward, but I can't think of a better way to arrange > this unless another helper function was used to combine these two calls > in order to avoid the extra indentation. > > >> rc = efx_init_tc(efx); >> @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) >> { >> struct ef100_nic_data *nic_data = efx->nic_data; >> >> + efx_fini_devlink(efx); >> efx_mcdi_detach(efx); >> efx_mcdi_fini(efx); >> if (nic_data) >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >> new file mode 100644 >> index 000000000000..c506f8f35d25 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >> @@ -0,0 +1,427 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/**************************************************************************** >> + * Driver for AMD network controllers and boards >> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation, incorporated herein by reference. >> + */ >> + >> +#include <linux/rtc.h> >> +#include "net_driver.h" >> +#include "ef100_nic.h" >> +#include "efx_devlink.h" >> +#include "nic.h" >> +#include "mcdi.h" >> +#include "mcdi_functions.h" >> +#include "mcdi_pcol.h" >> + >> +/* Custom devlink-info version object names for details that do not map to the >> + * generic standardized names. >> + */ >> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >> +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >> +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >> +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >> +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >> +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" > Here, you've defined several new versions, only one of which is > standard. I don't see an associated documentation addition. Please add a > ef100.rst file to Documentation/networking/devlink > > It is also preferred to use the standard names or extend the set of > standard names where appropriate first. Can you explain why you didn't > do that here? > > For example, the documentation for "fw.undi" indicates it may include > the UEFI driver, and I would expect your UEFI version to be something > like fw.undi or fw.undi.uefi if its distinct... I'm adding the doc file for v2. About uefi/undi, we have uefi but not undi. >> + >> +#define EFX_MAX_VERSION_INFO_LEN 64 >> + >> +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, >> + struct devlink_info_req *req, >> + unsigned int partition_type, >> + const char *version_name) >> +{ >> + char buf[EFX_MAX_VERSION_INFO_LEN]; >> + u16 version[4]; >> + int rc; >> + >> + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, >> + 0); >> + if (rc) >> + return rc; >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], >> + version[1], version[2], version[3]); >> + devlink_info_version_stored_put(req, version_name, buf); >> + >> + return 0; >> +} >> + >> +static void efx_devlink_info_stored_versions(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, >> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_MC_FIRMWARE, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_EXPANSION_ROM, >> + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); >> + efx_devlink_info_nvram_partition(efx, req, >> + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, >> + EFX_DEVLINK_INFO_VERSION_FW_UEFI); >> +} >> + >> +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) >> + >> +static void efx_devlink_info_board_cfg(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + char sn[EFX_MAX_SERIALNUM_LEN]; >> + u8 mac_address[ETH_ALEN]; >> + int rc; >> + >> + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); >> + if (!rc) { >> + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); >> + devlink_info_serial_number_put(req, sn); >> + } >> +} >> + >> +#define EFX_VER_FLAG(_f) \ >> + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) >> + >> +static void efx_devlink_info_running_versions(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ > This function for doing running versions is very long, and seems quite > complicated. It's difficult to parse what versions are being reported. > > I saw that your stored version used a helper function to separate each > thing out nicely. Is there any way to do that here? > >> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); >> + char buf[EFX_MAX_VERSION_INFO_LEN]; >> + unsigned int flags, build_id; >> + union { >> + const __le32 *dwords; >> + const __le16 *words; >> + const char *str; >> + } ver; > For example, you've got this local union on the stack that seems to be > reused a bunch... > >> + struct rtc_time build_date; >> + size_t outlength, offset; >> + u64 tstamp; >> + int rc; >> + >> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), >> + outbuf, sizeof(outbuf), &outlength); >> + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) >> + return; >> + >> + /* Handle previous output */ >> + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { >> + ver.words = (__le16 *)MCDI_PTR(outbuf, >> + GET_VERSION_EXT_OUT_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le16_to_cpu(ver.words[0]), >> + le16_to_cpu(ver.words[1]), >> + le16_to_cpu(ver.words[2]), >> + le16_to_cpu(ver.words[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >> + buf); >> + return; >> + } >> + >> + /* Handle V2 additions */ >> + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); >> + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", >> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); >> + devlink_info_version_fixed_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, >> + buf); >> + >> + /* Favour full board version if present (in V5 or later) */ >> + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", >> + MCDI_DWORD(outbuf, >> + GET_VERSION_V2_OUT_BOARD_REVISION)); >> + devlink_info_version_fixed_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >> + buf); >> + } >> + >> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); >> + if (ver.str[0]) >> + devlink_info_board_serial_number_put(req, ver.str); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_FPGA_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", >> + le32_to_cpu(ver.dwords[0]), >> + 'A' + le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); >> + if (ver.str[0]) >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%s)", ver.str); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FPGA_REV, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_CMCFW_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), >> + le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + tstamp = MCDI_QWORD(outbuf, >> + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); >> + if (tstamp) { >> + rtc_time64_to_tm(tstamp, &build_date); >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%ptRd)", &build_date); >> + } >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, >> + buf); >> + } >> + >> + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); >> + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), >> + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); >> + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { >> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); >> + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, >> + " (%x) %s", build_id, >> + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); >> + } >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, >> + buf); >> + >> + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V2_OUT_SUCFW_VERSION); >> + tstamp = MCDI_QWORD(outbuf, >> + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); >> + rtc_time64_to_tm(tstamp, &build_date); >> + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, >> + "%u.%u.%u.%u type %x (%ptRd)", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), >> + build_id, &build_date); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) >> + return; >> + >> + /* Handle V3 additions */ >> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) >> + return; >> + >> + /* Handle V4 additions */ >> + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && >> + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V4_OUT_SUCFW_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, >> + buf); >> + } >> + >> + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) >> + return; >> + >> + /* Handle V5 additions */ >> + >> + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V5_OUT_BOARD_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, >> + buf); >> + } >> + >> + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { >> + ver.dwords = (__le32 *)MCDI_PTR(outbuf, >> + GET_VERSION_V5_OUT_BUNDLE_VERSION); >> + >> + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", >> + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), >> + le32_to_cpu(ver.dwords[2]), >> + le32_to_cpu(ver.dwords[3])); >> + >> + devlink_info_version_running_put(req, >> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, >> + buf); >> + } >> +} > In the ice driver I implemented each version extraction as a separate > function to make it more clear which versions where which. This running > function could benefit from some organization like that. > >> + >> +#undef EFX_VER_FLAG >> + >> +static void efx_devlink_info_query_all(struct efx_nic *efx, >> + struct devlink_info_req *req) >> +{ >> + efx_devlink_info_board_cfg(efx, req); >> + efx_devlink_info_stored_versions(efx, req); >> + efx_devlink_info_running_versions(efx, req); >> +} > Do you really need this to be a separate function and not just inlined > into the caller, efx_devlink_info_get. > >> + >> +struct efx_devlink { >> + struct efx_nic *efx; >> +}; >> + >> +static int efx_devlink_info_get(struct devlink *devlink, >> + struct devlink_info_req *req, >> + struct netlink_ext_ack *extack) >> +{ >> + struct efx_devlink *devlink_private = devlink_priv(devlink); >> + struct efx_nic *efx = devlink_private->efx; >> + >> + efx_devlink_info_query_all(efx, req); >> + return 0; >> +} >> + >> +static const struct devlink_ops sfc_devlink_ops = { >> + .info_get = efx_devlink_info_get, >> +}; >> + >> +void efx_fini_devlink(struct efx_nic *efx) >> +{ >> + if (efx->devlink) { >> + struct efx_devlink *devlink_private; >> + >> + devlink_private = devlink_priv(efx->devlink); >> + >> + devlink_unregister(efx->devlink); >> + devlink_free(efx->devlink); >> + efx->devlink = NULL; >> + } >> +} >> + >> +int efx_probe_devlink(struct efx_nic *efx) >> +{ >> + struct efx_devlink *devlink_private; >> + >> + efx->devlink = devlink_alloc(&sfc_devlink_ops, >> + sizeof(struct efx_devlink), >> + &efx->pci_dev->dev); >> + if (!efx->devlink) >> + return -ENOMEM; >> + devlink_private = devlink_priv(efx->devlink); >> + devlink_private->efx = efx; >> + >> + devlink_register(efx->devlink); >> + > So drivers implementing devlink typically would allocate the devlink > early (almost the first thing it does) and make its private field > something like their main private data structure. > > Obviously you're adding this to a pre-existing driver and it is > initially simpler and easier to keep it separate. I'm not sure what > other reviewers' stance on this is, but I think its preferable to > allocate as an early stage and register once the driver is ready to > accept requests from user space. > > As for a reason why, implementing reload support effectively requires this. > >> + return 0; >> +} >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h >> new file mode 100644 >> index 000000000000..997f878aea93 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_devlink.h >> @@ -0,0 +1,20 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/**************************************************************************** >> + * Driver for AMD network controllers and boards >> + * Copyright (C) 2023, Advanced Micro Devices, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation, incorporated herein by reference. >> + */ >> + >> +#ifndef _EFX_DEVLINK_H >> +#define _EFX_DEVLINK_H >> + >> +#include "net_driver.h" >> +#include <net/devlink.h> >> + >> +int efx_probe_devlink(struct efx_nic *efx); >> +void efx_fini_devlink(struct efx_nic *efx); >> + >> +#endif /* _EFX_DEVLINK_H */ >> diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c >> index af338208eae9..328cae82a7d8 100644 >> --- a/drivers/net/ethernet/sfc/mcdi.c >> +++ b/drivers/net/ethernet/sfc/mcdi.c >> @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) >> return rc; >> } >> >> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >> + u32 *subtype, u16 version[4], char *desc, >> + size_t descsize) >> +{ >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); >> + efx_dword_t *outbuf; >> + size_t outlen; >> + u32 flags; >> + int rc; >> + >> + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); >> + if (!outbuf) >> + return -ENOMEM; >> + >> + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); >> + >> + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, >> + sizeof(inbuf), outbuf, >> + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, >> + &outlen); >> + if (rc) >> + goto out_free; >> + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { >> + rc = -EIO; >> + goto out_free; >> + } >> + >> + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); >> + >> + if (desc && descsize > 0) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { >> + if (descsize <= >> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { >> + rc = -E2BIG; >> + goto out_free; >> + } >> + >> + strncpy(desc, >> + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), >> + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); >> + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; >> + } else { >> + desc[0] = '\0'; >> + } >> + } >> + >> + if (subtype) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) >> + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); >> + else >> + *subtype = 0; >> + } >> + >> + if (version) { >> + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { >> + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); >> + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); >> + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); >> + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); >> + } else { >> + version[0] = 0; >> + version[1] = 0; >> + version[2] = 0; >> + version[3] = 0; >> + } >> + } >> + >> +out_free: >> + kfree(outbuf); >> + return rc; >> +} >> + >> int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, >> size_t len, size_t *retlen, u8 *buffer) >> { >> diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h >> index 7e35fec9da35..63b090587f7a 100644 >> --- a/drivers/net/ethernet/sfc/mcdi.h >> +++ b/drivers/net/ethernet/sfc/mcdi.h >> @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, >> bool *protected_out); >> int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); >> int efx_mcdi_nvram_test_all(struct efx_nic *efx); >> +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, >> + u32 *subtype, u16 version[4], char *desc, >> + size_t descsize); >> int efx_mcdi_handle_assertion(struct efx_nic *efx); >> int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); >> int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, >> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >> index 3b49e216768b..d036641dc043 100644 >> --- a/drivers/net/ethernet/sfc/net_driver.h >> +++ b/drivers/net/ethernet/sfc/net_driver.h >> @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { >> * xdp_rxq_info structures? >> * @netdev_notifier: Netdevice notifier. >> * @tc: state for TC offload (EF100). >> + * @devlink: reference to devlink structure owned by this device >> * @mem_bar: The BAR that is mapped into membase. >> * @reg_base: Offset from the start of the bar to the function control window. >> * @monitor_work: Hardware monitor workitem >> @@ -1179,6 +1180,7 @@ struct efx_nic { >> struct notifier_block netdev_notifier; >> struct efx_tc_state *tc; >> >> + struct devlink *devlink; >> unsigned int mem_bar; >> u32 reg_base; >>
On 1/24/2023 3:05 AM, Lucero Palau, Alejandro wrote: > > On 1/19/23 23:40, Jacob Keller wrote: >> >> On 1/19/2023 3:31 AM, alejandro.lucero-palau@amd.com wrote: >>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" >>> +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" >>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" >>> +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" >>> +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" >>> +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" >> Here, you've defined several new versions, only one of which is >> standard. I don't see an associated documentation addition. Please add a >> ef100.rst file to Documentation/networking/devlink >> >> It is also preferred to use the standard names or extend the set of >> standard names where appropriate first. Can you explain why you didn't >> do that here? >> >> For example, the documentation for "fw.undi" indicates it may include >> the UEFI driver, and I would expect your UEFI version to be something >> like fw.undi or fw.undi.uefi if its distinct... > > > I'm adding the doc file for v2. > > About uefi/undi, we have uefi but not undi. > I believe this is the same as ice, and we still used fw.undi. From the doc: UNDI software, may include the UEFI driver, firmware or both. Basically whether it includes the UEFI driver, firmware, or both it should still be fw.undi, and i would expect if you really must separately version the components those would need sub-fields added to the devlink-info.rst
diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig index 0950e6b0508f..4af36ba8906b 100644 --- a/drivers/net/ethernet/sfc/Kconfig +++ b/drivers/net/ethernet/sfc/Kconfig @@ -22,6 +22,7 @@ config SFC depends on PTP_1588_CLOCK_OPTIONAL select MDIO select CRC32 + select NET_DEVLINK help This driver supports 10/40-gigabit Ethernet cards based on the Solarflare SFC9100-family controllers. diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile index 712a48d00069..55b9c73cd8ef 100644 --- a/drivers/net/ethernet/sfc/Makefile +++ b/drivers/net/ethernet/sfc/Makefile @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ mcdi.o mcdi_port.o mcdi_port_common.o \ mcdi_functions.o mcdi_filters.o mcdi_mon.o \ ef100.o ef100_nic.o ef100_netdev.o \ - ef100_ethtool.o ef100_rx.o ef100_tx.o + ef100_ethtool.o ef100_rx.o ef100_tx.o \ + efx_devlink.o sfc-$(CONFIG_SFC_MTD) += mtd.o sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ mae.o tc.o tc_bindings.o tc_counters.o diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c index ad686c671ab8..14af8f314b83 100644 --- a/drivers/net/ethernet/sfc/ef100_nic.c +++ b/drivers/net/ethernet/sfc/ef100_nic.c @@ -27,6 +27,7 @@ #include "tc.h" #include "mae.h" #include "rx_common.h" +#include "efx_devlink.h" #define EF100_MAX_VIS 4096 #define EF100_NUM_MCDI_BUFFERS 1 @@ -1124,6 +1125,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx) netif_warn(efx, probe, net_dev, "Failed to probe base mport rc %d; representors will not function\n", rc); + } else { + if (efx_probe_devlink(efx)) + netif_warn(efx, probe, net_dev, + "Failed to register devlink\n"); } rc = efx_init_tc(efx); @@ -1157,6 +1162,7 @@ void ef100_remove(struct efx_nic *efx) { struct ef100_nic_data *nic_data = efx->nic_data; + efx_fini_devlink(efx); efx_mcdi_detach(efx); efx_mcdi_fini(efx); if (nic_data) diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c new file mode 100644 index 000000000000..c506f8f35d25 --- /dev/null +++ b/drivers/net/ethernet/sfc/efx_devlink.c @@ -0,0 +1,427 @@ +// SPDX-License-Identifier: GPL-2.0-only +/**************************************************************************** + * Driver for AMD network controllers and boards + * Copyright (C) 2023, Advanced Micro Devices, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation, incorporated herein by reference. + */ + +#include <linux/rtc.h> +#include "net_driver.h" +#include "ef100_nic.h" +#include "efx_devlink.h" +#include "nic.h" +#include "mcdi.h" +#include "mcdi_functions.h" +#include "mcdi_pcol.h" + +/* Custom devlink-info version object names for details that do not map to the + * generic standardized names. + */ +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC "fw.mgmt.suc" +#define EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC "fw.mgmt.cmc" +#define EFX_DEVLINK_INFO_VERSION_FPGA_REV "fpga.rev" +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_HW "fpga.app" +#define EFX_DEVLINK_INFO_VERSION_DATAPATH_FW DEVLINK_INFO_VERSION_GENERIC_FW_APP +#define EFX_DEVLINK_INFO_VERSION_SOC_BOOT "coproc.boot" +#define EFX_DEVLINK_INFO_VERSION_SOC_UBOOT "coproc.uboot" +#define EFX_DEVLINK_INFO_VERSION_SOC_MAIN "coproc.main" +#define EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY "coproc.recovery" +#define EFX_DEVLINK_INFO_VERSION_FW_EXPROM "fw.exprom" +#define EFX_DEVLINK_INFO_VERSION_FW_UEFI "fw.uefi" + +#define EFX_MAX_VERSION_INFO_LEN 64 + +static int efx_devlink_info_nvram_partition(struct efx_nic *efx, + struct devlink_info_req *req, + unsigned int partition_type, + const char *version_name) +{ + char buf[EFX_MAX_VERSION_INFO_LEN]; + u16 version[4]; + int rc; + + rc = efx_mcdi_nvram_metadata(efx, partition_type, NULL, version, NULL, + 0); + if (rc) + return rc; + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", version[0], + version[1], version[2], version[3]); + devlink_info_version_stored_put(req, version_name, buf); + + return 0; +} + +static void efx_devlink_info_stored_versions(struct efx_nic *efx, + struct devlink_info_req *req) +{ + efx_devlink_info_nvram_partition(efx, req, NVRAM_PARTITION_TYPE_BUNDLE, + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID); + efx_devlink_info_nvram_partition(efx, req, + NVRAM_PARTITION_TYPE_MC_FIRMWARE, + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT); + efx_devlink_info_nvram_partition(efx, req, + NVRAM_PARTITION_TYPE_SUC_FIRMWARE, + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC); + efx_devlink_info_nvram_partition(efx, req, + NVRAM_PARTITION_TYPE_EXPANSION_ROM, + EFX_DEVLINK_INFO_VERSION_FW_EXPROM); + efx_devlink_info_nvram_partition(efx, req, + NVRAM_PARTITION_TYPE_EXPANSION_UEFI, + EFX_DEVLINK_INFO_VERSION_FW_UEFI); +} + +#define EFX_MAX_SERIALNUM_LEN (ETH_ALEN * 2 + 1) + +static void efx_devlink_info_board_cfg(struct efx_nic *efx, + struct devlink_info_req *req) +{ + char sn[EFX_MAX_SERIALNUM_LEN]; + u8 mac_address[ETH_ALEN]; + int rc; + + rc = efx_mcdi_get_board_cfg(efx, (u8 *)mac_address, NULL, NULL); + if (!rc) { + snprintf(sn, EFX_MAX_SERIALNUM_LEN, "%pm", mac_address); + devlink_info_serial_number_put(req, sn); + } +} + +#define EFX_VER_FLAG(_f) \ + (MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN) + +static void efx_devlink_info_running_versions(struct efx_nic *efx, + struct devlink_info_req *req) +{ + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_VERSION_V5_OUT_LEN); + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_VERSION_EXT_IN_LEN); + char buf[EFX_MAX_VERSION_INFO_LEN]; + unsigned int flags, build_id; + union { + const __le32 *dwords; + const __le16 *words; + const char *str; + } ver; + struct rtc_time build_date; + size_t outlength, offset; + u64 tstamp; + int rc; + + rc = efx_mcdi_rpc(efx, MC_CMD_GET_VERSION, inbuf, sizeof(inbuf), + outbuf, sizeof(outbuf), &outlength); + if (rc || outlength < MC_CMD_GET_VERSION_OUT_LEN) + return; + + /* Handle previous output */ + if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) { + ver.words = (__le16 *)MCDI_PTR(outbuf, + GET_VERSION_EXT_OUT_VERSION); + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le16_to_cpu(ver.words[0]), + le16_to_cpu(ver.words[1]), + le16_to_cpu(ver.words[2]), + le16_to_cpu(ver.words[3])); + + devlink_info_version_running_put(req, + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, + buf); + return; + } + + /* Handle V2 additions */ + flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS); + if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) { + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s", + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME)); + devlink_info_version_fixed_put(req, + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, + buf); + + /* Favour full board version if present (in V5 or later) */ + if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u", + MCDI_DWORD(outbuf, + GET_VERSION_V2_OUT_BOARD_REVISION)); + devlink_info_version_fixed_put(req, + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, + buf); + } + + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL); + if (ver.str[0]) + devlink_info_board_serial_number_put(req, ver.str); + } + + if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V2_OUT_FPGA_VERSION); + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u", + le32_to_cpu(ver.dwords[0]), + 'A' + le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2])); + + ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA); + if (ver.str[0]) + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, + " (%s)", ver.str); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_FPGA_REV, + buf); + } + + if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V2_OUT_CMCFW_VERSION); + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), + le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + tstamp = MCDI_QWORD(outbuf, + GET_VERSION_V2_OUT_CMCFW_BUILD_DATE); + if (tstamp) { + rtc_time64_to_tm(tstamp, &build_date); + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, + " (%ptRd)", &build_date); + } + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC, + buf); + } + + ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION); + offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]), + le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3])); + if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) { + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID); + snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset, + " (%x) %s", build_id, + MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME)); + } + devlink_info_version_running_put(req, + DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, + buf); + + if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V2_OUT_SUCFW_VERSION); + tstamp = MCDI_QWORD(outbuf, + GET_VERSION_V2_OUT_SUCFW_BUILD_DATE); + rtc_time64_to_tm(tstamp, &build_date); + build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, + "%u.%u.%u.%u type %x (%ptRd)", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]), + build_id, &build_date); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, + buf); + } + + if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN) + return; + + /* Handle V3 additions */ + if (flags & BIT(EFX_VER_FLAG(DATAPATH_HW_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V3_OUT_DATAPATH_HW_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2])); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_DATAPATH_HW, + buf); + } + + if (flags & BIT(EFX_VER_FLAG(DATAPATH_FW_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V3_OUT_DATAPATH_FW_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2])); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_DATAPATH_FW, + buf); + } + + if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN) + return; + + /* Handle V4 additions */ + if (flags & BIT(EFX_VER_FLAG(SOC_BOOT_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V4_OUT_SOC_BOOT_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_SOC_BOOT, + buf); + } + + if (flags & BIT(EFX_VER_FLAG(SOC_UBOOT_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V4_OUT_SOC_UBOOT_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_SOC_UBOOT, + buf); + } + + if (flags & BIT(EFX_VER_FLAG(SOC_MAIN_ROOTFS_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V4_OUT_SOC_MAIN_ROOTFS_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_SOC_MAIN, + buf); + } + + if (flags & BIT(EFX_VER_FLAG(SOC_RECOVERY_BUILDROOT_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V4_OUT_SOC_RECOVERY_BUILDROOT_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_SOC_RECOVERY, + buf); + } + + if (flags & BIT(EFX_VER_FLAG(SUCFW_VERSION)) && + ~flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V4_OUT_SUCFW_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + devlink_info_version_running_put(req, + EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC, + buf); + } + + if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN) + return; + + /* Handle V5 additions */ + + if (flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V5_OUT_BOARD_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + devlink_info_version_running_put(req, + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, + buf); + } + + if (flags & BIT(EFX_VER_FLAG(BUNDLE_VERSION))) { + ver.dwords = (__le32 *)MCDI_PTR(outbuf, + GET_VERSION_V5_OUT_BUNDLE_VERSION); + + snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u", + le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]), + le32_to_cpu(ver.dwords[2]), + le32_to_cpu(ver.dwords[3])); + + devlink_info_version_running_put(req, + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, + buf); + } +} + +#undef EFX_VER_FLAG + +static void efx_devlink_info_query_all(struct efx_nic *efx, + struct devlink_info_req *req) +{ + efx_devlink_info_board_cfg(efx, req); + efx_devlink_info_stored_versions(efx, req); + efx_devlink_info_running_versions(efx, req); +} + +struct efx_devlink { + struct efx_nic *efx; +}; + +static int efx_devlink_info_get(struct devlink *devlink, + struct devlink_info_req *req, + struct netlink_ext_ack *extack) +{ + struct efx_devlink *devlink_private = devlink_priv(devlink); + struct efx_nic *efx = devlink_private->efx; + + efx_devlink_info_query_all(efx, req); + return 0; +} + +static const struct devlink_ops sfc_devlink_ops = { + .info_get = efx_devlink_info_get, +}; + +void efx_fini_devlink(struct efx_nic *efx) +{ + if (efx->devlink) { + struct efx_devlink *devlink_private; + + devlink_private = devlink_priv(efx->devlink); + + devlink_unregister(efx->devlink); + devlink_free(efx->devlink); + efx->devlink = NULL; + } +} + +int efx_probe_devlink(struct efx_nic *efx) +{ + struct efx_devlink *devlink_private; + + efx->devlink = devlink_alloc(&sfc_devlink_ops, + sizeof(struct efx_devlink), + &efx->pci_dev->dev); + if (!efx->devlink) + return -ENOMEM; + devlink_private = devlink_priv(efx->devlink); + devlink_private->efx = efx; + + devlink_register(efx->devlink); + + return 0; +} diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h new file mode 100644 index 000000000000..997f878aea93 --- /dev/null +++ b/drivers/net/ethernet/sfc/efx_devlink.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/**************************************************************************** + * Driver for AMD network controllers and boards + * Copyright (C) 2023, Advanced Micro Devices, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation, incorporated herein by reference. + */ + +#ifndef _EFX_DEVLINK_H +#define _EFX_DEVLINK_H + +#include "net_driver.h" +#include <net/devlink.h> + +int efx_probe_devlink(struct efx_nic *efx); +void efx_fini_devlink(struct efx_nic *efx); + +#endif /* _EFX_DEVLINK_H */ diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c index af338208eae9..328cae82a7d8 100644 --- a/drivers/net/ethernet/sfc/mcdi.c +++ b/drivers/net/ethernet/sfc/mcdi.c @@ -2308,6 +2308,78 @@ static int efx_mcdi_nvram_update_finish(struct efx_nic *efx, unsigned int type) return rc; } +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, + u32 *subtype, u16 version[4], char *desc, + size_t descsize) +{ + MCDI_DECLARE_BUF(inbuf, MC_CMD_NVRAM_METADATA_IN_LEN); + efx_dword_t *outbuf; + size_t outlen; + u32 flags; + int rc; + + outbuf = kzalloc(MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, GFP_KERNEL); + if (!outbuf) + return -ENOMEM; + + MCDI_SET_DWORD(inbuf, NVRAM_METADATA_IN_TYPE, type); + + rc = efx_mcdi_rpc_quiet(efx, MC_CMD_NVRAM_METADATA, inbuf, + sizeof(inbuf), outbuf, + MC_CMD_NVRAM_METADATA_OUT_LENMAX_MCDI2, + &outlen); + if (rc) + goto out_free; + if (outlen < MC_CMD_NVRAM_METADATA_OUT_LENMIN) { + rc = -EIO; + goto out_free; + } + + flags = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_FLAGS); + + if (desc && descsize > 0) { + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_VALID_LBN)) { + if (descsize <= + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)) { + rc = -E2BIG; + goto out_free; + } + + strncpy(desc, + MCDI_PTR(outbuf, NVRAM_METADATA_OUT_DESCRIPTION), + MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)); + desc[MC_CMD_NVRAM_METADATA_OUT_DESCRIPTION_NUM(outlen)] = '\0'; + } else { + desc[0] = '\0'; + } + } + + if (subtype) { + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_SUBTYPE_VALID_LBN)) + *subtype = MCDI_DWORD(outbuf, NVRAM_METADATA_OUT_SUBTYPE); + else + *subtype = 0; + } + + if (version) { + if (flags & BIT(MC_CMD_NVRAM_METADATA_OUT_VERSION_VALID_LBN)) { + version[0] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_W); + version[1] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_X); + version[2] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Y); + version[3] = MCDI_WORD(outbuf, NVRAM_METADATA_OUT_VERSION_Z); + } else { + version[0] = 0; + version[1] = 0; + version[2] = 0; + version[3] = 0; + } + } + +out_free: + kfree(outbuf); + return rc; +} + int efx_mcdi_mtd_read(struct mtd_info *mtd, loff_t start, size_t len, size_t *retlen, u8 *buffer) { diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h index 7e35fec9da35..63b090587f7a 100644 --- a/drivers/net/ethernet/sfc/mcdi.h +++ b/drivers/net/ethernet/sfc/mcdi.h @@ -379,6 +379,9 @@ int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type, bool *protected_out); int efx_new_mcdi_nvram_test_all(struct efx_nic *efx); int efx_mcdi_nvram_test_all(struct efx_nic *efx); +int efx_mcdi_nvram_metadata(struct efx_nic *efx, unsigned int type, + u32 *subtype, u16 version[4], char *desc, + size_t descsize); int efx_mcdi_handle_assertion(struct efx_nic *efx); int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode); int efx_mcdi_wol_filter_set_magic(struct efx_nic *efx, const u8 *mac, diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index 3b49e216768b..d036641dc043 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode { * xdp_rxq_info structures? * @netdev_notifier: Netdevice notifier. * @tc: state for TC offload (EF100). + * @devlink: reference to devlink structure owned by this device * @mem_bar: The BAR that is mapped into membase. * @reg_base: Offset from the start of the bar to the function control window. * @monitor_work: Hardware monitor workitem @@ -1179,6 +1180,7 @@ struct efx_nic { struct notifier_block netdev_notifier; struct efx_tc_state *tc; + struct devlink *devlink; unsigned int mem_bar; u32 reg_base;