| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <24448301-f6cd-2b8e-f9fa-570bc10953c9@intel.com>
Date: Thu, 19 Jan 2023 15:40:51 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: <alejandro.lucero-palau@....com>, <netdev@...r.kernel.org>,
<linux-net-drivers@....com>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
<edumazet@...gle.com>, <habetsm@...il.com>,
<ecree.xilinx@...il.com>
Subject: Re: [PATCH net-next 1/7] sfc: add devlink support for ef100
On 1/19/2023 3:31 AM, alejandro.lucero-palau@....com wrote:
> From: Alejandro Lucero <alejandro.lucero-palau@....com>
>
> Basic support for devlink info command.
>
> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@....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;
>
Powered by blists - more mailing lists