| 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: <a9a56e4c-1a57-a282-1ae5-182656437e01@amd.com>
Date: Fri, 20 Jan 2023 14:11:59 +0000
From: "Lucero Palau, Alejandro" <alejandro.lucero-palau@....com>
To: Jacob Keller <jacob.e.keller@...el.com>,
"Lucero Palau, Alejandro" <alejandro.lucero-palau@....com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-net-drivers (AMD-Xilinx)" <linux-net-drivers@....com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"habetsm@...il.com" <habetsm@...il.com>,
"ecree.xilinx@...il.com" <ecree.xilinx@...il.com>
Subject: Re: [PATCH net-next 1/7] sfc: add devlink support for ef100
On 1/19/23 23:40, Jacob Keller wrote:
>
> 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?
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;
>>
Powered by blists - more mailing lists