diff mbox series

[v5,net-next,2/8] sfc: add devlink info support for ef100

Message ID 20230202111423.56831-3-alejandro.lucero-palau@amd.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series sfc: devlink support for ef100 | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

Lucero Palau, Alejandro Feb. 2, 2023, 11:14 a.m. UTC
From: Alejandro Lucero <alejandro.lucero-palau@amd.com>

Support for devlink info command.

Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com>
---
 Documentation/networking/devlink/sfc.rst |  57 +++
 MAINTAINERS                              |   1 +
 drivers/net/ethernet/sfc/efx_devlink.c   | 459 +++++++++++++++++++++++
 drivers/net/ethernet/sfc/efx_devlink.h   |  17 +
 drivers/net/ethernet/sfc/mcdi.c          |  72 ++++
 drivers/net/ethernet/sfc/mcdi.h          |   3 +
 6 files changed, 609 insertions(+)
 create mode 100644 Documentation/networking/devlink/sfc.rst

Comments

Jiri Pirko Feb. 2, 2023, 11:58 a.m. UTC | #1
Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:
>From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>
>Support for devlink info command.

You are quite brief for couple hundred line patch. Care to shed some
more details for the reader? Also, use imperative mood (applies to the
rest of the pathes)

[...]


>+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;
>+	char msg[NETLINK_MAX_FMTMSG_LEN];
>+	int errors_reported = 0;
>+	int rc;
>+
>+	/* Several different MCDI commands are used. We report first error
>+	 * through extack along with total number of errors. Specific error
>+	 * information via system messages.
>+	 */
>+	rc = efx_devlink_info_board_cfg(efx, req);
>+	if (rc) {
>+		sprintf(msg, "Getting board info failed");
>+		errors_reported++;
>+	}
>+	rc = efx_devlink_info_stored_versions(efx, req);
>+	if (rc) {
>+		if (!errors_reported)
>+			sprintf(msg, "Getting stored versions failed");
>+		errors_reported += rc;
>+	}
>+	rc = efx_devlink_info_running_versions(efx, req);
>+	if (rc) {
>+		if (!errors_reported)
>+			sprintf(msg, "Getting board info failed");
>+		errors_reported++;


Under which circumstances any of the errors above happen? Is it a common
thing? Or is it result of some fatal event?

You treat it like it is quite common, which seems very odd to me.
If they are rare, just return error right away to the caller.



>+	}
>+
>+	if (errors_reported)
>+		NL_SET_ERR_MSG_FMT(extack,
>+				   "%s. %d total errors. Check system messages",
>+				   msg, errors_reported);
>+	return 0;
>+}
>+
> static const struct devlink_ops sfc_devlink_ops = {
>+	.info_get			= efx_devlink_info_get,
> };

[...]
kernel test robot Feb. 3, 2023, 2:37 a.m. UTC | #2
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/20230202-191843
patch link:    https://lore.kernel.org/r/20230202111423.56831-3-alejandro.lucero-palau%40amd.com
patch subject: [PATCH v5 net-next 2/8] sfc: add devlink info support for ef100
config: microblaze-randconfig-s042-20230202 (https://download.01.org/0day-ci/archive/20230203/202302031027.lyf8KjKA-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/ae013a0522dccc6ec3db361d23a5cbf2e1de2702
        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/20230202-191843
        git checkout ae013a0522dccc6ec3db361d23a5cbf2e1de2702
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze 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 >>):

   microblaze-linux-ld: drivers/net/ethernet/sfc/efx_devlink.o: in function `efx_devlink_info_running_v2.constprop.0':
>> drivers/net/ethernet/sfc/efx_devlink.c:157: undefined reference to `rtc_time64_to_tm'
>> microblaze-linux-ld: drivers/net/ethernet/sfc/efx_devlink.c:186: undefined reference to `rtc_time64_to_tm'


vim +157 drivers/net/ethernet/sfc/efx_devlink.c

    86	
    87	#define EFX_VER_FLAG(_f)	\
    88		(MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN)
    89	
    90	static void efx_devlink_info_running_v2(struct efx_nic *efx,
    91						struct devlink_info_req *req,
    92						unsigned int flags, efx_dword_t *outbuf)
    93	{
    94		char buf[EFX_MAX_VERSION_INFO_LEN];
    95		union {
    96			const __le32 *dwords;
    97			const __le16 *words;
    98			const char *str;
    99		} ver;
   100		struct rtc_time build_date;
   101		unsigned int build_id;
   102		size_t offset;
   103		u64 tstamp;
   104	
   105		if (flags & BIT(EFX_VER_FLAG(BOARD_EXT_INFO))) {
   106			snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%s",
   107				 MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_NAME));
   108			devlink_info_version_fixed_put(req,
   109						       DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,
   110						       buf);
   111	
   112			/* Favour full board version if present (in V5 or later) */
   113			if (~flags & BIT(EFX_VER_FLAG(BOARD_VERSION))) {
   114				snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u",
   115					 MCDI_DWORD(outbuf,
   116						    GET_VERSION_V2_OUT_BOARD_REVISION));
   117				devlink_info_version_fixed_put(req,
   118							       DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,
   119							       buf);
   120			}
   121	
   122			ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_BOARD_SERIAL);
   123			if (ver.str[0])
   124				devlink_info_board_serial_number_put(req, ver.str);
   125		}
   126	
   127		if (flags & BIT(EFX_VER_FLAG(FPGA_EXT_INFO))) {
   128			ver.dwords = (__le32 *)MCDI_PTR(outbuf,
   129							GET_VERSION_V2_OUT_FPGA_VERSION);
   130			offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u_%c%u",
   131					  le32_to_cpu(ver.dwords[0]),
   132					  'A' + le32_to_cpu(ver.dwords[1]),
   133					  le32_to_cpu(ver.dwords[2]));
   134	
   135			ver.str = MCDI_PTR(outbuf, GET_VERSION_V2_OUT_FPGA_EXTRA);
   136			if (ver.str[0])
   137				snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset,
   138					 " (%s)", ver.str);
   139	
   140			devlink_info_version_running_put(req,
   141							 EFX_DEVLINK_INFO_VERSION_FPGA_REV,
   142							 buf);
   143		}
   144	
   145		if (flags & BIT(EFX_VER_FLAG(CMC_EXT_INFO))) {
   146			ver.dwords = (__le32 *)MCDI_PTR(outbuf,
   147							GET_VERSION_V2_OUT_CMCFW_VERSION);
   148			offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u",
   149					  le32_to_cpu(ver.dwords[0]),
   150					  le32_to_cpu(ver.dwords[1]),
   151					  le32_to_cpu(ver.dwords[2]),
   152					  le32_to_cpu(ver.dwords[3]));
   153	
   154			tstamp = MCDI_QWORD(outbuf,
   155					    GET_VERSION_V2_OUT_CMCFW_BUILD_DATE);
   156			if (tstamp) {
 > 157				rtc_time64_to_tm(tstamp, &build_date);
   158				snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset,
   159					 " (%ptRd)", &build_date);
   160			}
   161	
   162			devlink_info_version_running_put(req,
   163							 EFX_DEVLINK_INFO_VERSION_FW_MGMT_CMC,
   164							 buf);
   165		}
   166	
   167		ver.words = (__le16 *)MCDI_PTR(outbuf, GET_VERSION_V2_OUT_VERSION);
   168		offset = snprintf(buf, EFX_MAX_VERSION_INFO_LEN, "%u.%u.%u.%u",
   169				  le16_to_cpu(ver.words[0]), le16_to_cpu(ver.words[1]),
   170				  le16_to_cpu(ver.words[2]), le16_to_cpu(ver.words[3]));
   171		if (flags & BIT(EFX_VER_FLAG(MCFW_EXT_INFO))) {
   172			build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_ID);
   173			snprintf(&buf[offset], EFX_MAX_VERSION_INFO_LEN - offset,
   174				 " (%x) %s", build_id,
   175				 MCDI_PTR(outbuf, GET_VERSION_V2_OUT_MCFW_BUILD_NAME));
   176		}
   177		devlink_info_version_running_put(req,
   178						 DEVLINK_INFO_VERSION_GENERIC_FW_MGMT,
   179						 buf);
   180	
   181		if (flags & BIT(EFX_VER_FLAG(SUCFW_EXT_INFO))) {
   182			ver.dwords = (__le32 *)MCDI_PTR(outbuf,
   183							GET_VERSION_V2_OUT_SUCFW_VERSION);
   184			tstamp = MCDI_QWORD(outbuf,
   185					    GET_VERSION_V2_OUT_SUCFW_BUILD_DATE);
 > 186			rtc_time64_to_tm(tstamp, &build_date);
   187			build_id = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_SUCFW_CHIP_ID);
   188	
   189			snprintf(buf, EFX_MAX_VERSION_INFO_LEN,
   190				 "%u.%u.%u.%u type %x (%ptRd)",
   191				 le32_to_cpu(ver.dwords[0]), le32_to_cpu(ver.dwords[1]),
   192				 le32_to_cpu(ver.dwords[2]), le32_to_cpu(ver.dwords[3]),
   193				 build_id, &build_date);
   194	
   195			devlink_info_version_running_put(req,
   196							 EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC,
   197							 buf);
   198		}
   199	}
   200
kernel test robot Feb. 3, 2023, 11:38 a.m. UTC | #3
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/20230202-191843
patch link:    https://lore.kernel.org/r/20230202111423.56831-3-alejandro.lucero-palau%40amd.com
patch subject: [PATCH v5 net-next 2/8] sfc: add devlink info support for ef100
reproduce:
        # https://github.com/intel-lab-lkp/linux/commit/ae013a0522dccc6ec3db361d23a5cbf2e1de2702
        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/20230202-191843
        git checkout ae013a0522dccc6ec3db361d23a5cbf2e1de2702
        make menuconfig
        # enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
        make htmldocs

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 >>):

>> Documentation/networking/devlink/sfc.rst:27: WARNING: Bullet list ends without a blank line; unexpected unindent.
>> Documentation/networking/devlink/sfc.rst:29: WARNING: Block quote ends without a blank line; unexpected unindent.
>> Documentation/networking/devlink/sfc.rst:15: WARNING: Error parsing content block for the "list-table" directive: exactly one bullet list expected.

vim +27 Documentation/networking/devlink/sfc.rst

    14	
  > 15	.. list-table:: devlink info versions implemented
    16	    :widths: 5 5 90
    17	
    18	   * - Name
    19	     - Type
    20	     - Description
    21	   * - ``fw.mgmt.suc``
    22	     - running
    23	     - For boards where the management function is split between multiple
    24	       control units, this is the SUC control unit's firmware version.
    25	   * - ``fw.mgmt.cmc``
    26	     - running
  > 27	    - For boards where the management function is split between multiple
    28	       control units, this is the CMC control unit's firmware version.
  > 29	   * - ``fpga.rev``
Lucero Palau, Alejandro Feb. 7, 2023, 2:42 p.m. UTC | #4
On 2/2/23 11:58, Jiri Pirko wrote:
> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>>
>> Support for devlink info command.
> You are quite brief for couple hundred line patch. Care to shed some
> more details for the reader? Also, use imperative mood (applies to the
> rest of the pathes)
>
> [...]
>

OK. I'll be more talkative and imperative here.

>> +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;
>> +	char msg[NETLINK_MAX_FMTMSG_LEN];
>> +	int errors_reported = 0;
>> +	int rc;
>> +
>> +	/* Several different MCDI commands are used. We report first error
>> +	 * through extack along with total number of errors. Specific error
>> +	 * information via system messages.
>> +	 */
>> +	rc = efx_devlink_info_board_cfg(efx, req);
>> +	if (rc) {
>> +		sprintf(msg, "Getting board info failed");
>> +		errors_reported++;
>> +	}
>> +	rc = efx_devlink_info_stored_versions(efx, req);
>> +	if (rc) {
>> +		if (!errors_reported)
>> +			sprintf(msg, "Getting stored versions failed");
>> +		errors_reported += rc;
>> +	}
>> +	rc = efx_devlink_info_running_versions(efx, req);
>> +	if (rc) {
>> +		if (!errors_reported)
>> +			sprintf(msg, "Getting board info failed");
>> +		errors_reported++;
>
> Under which circumstances any of the errors above happen? Is it a common
> thing? Or is it result of some fatal event?

They are not common at all. If any of those happen, it is a bad sign, 
and it is more than likely there are more than one because something is 
not working properly. That is the reason I only report first error found 
plus the total number of errors detected.


>
> You treat it like it is quite common, which seems very odd to me.
> If they are rare, just return error right away to the caller.

Well, that is done now. And as I say, I'm not reporting all but just the 
first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.

If errors trigger, a more complete information will appear in system 
messages, so that is the reason with:

+               NL_SET_ERR_MSG_FMT(extack,
+                                  "%s. %d total errors. Check system messages",
+                                  msg, errors_reported);

I guess you are concerned with the extack report being overwhelmed, but 
I do not think that is the case.

>
>
>> +	}
>> +
>> +	if (errors_reported)
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "%s. %d total errors. Check system messages",
>> +				   msg, errors_reported);
>> +	return 0;
>> +}
>> +
>> static const struct devlink_ops sfc_devlink_ops = {
>> +	.info_get			= efx_devlink_info_get,
>> };
> [...]
Jiri Pirko Feb. 7, 2023, 2:58 p.m. UTC | #5
Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@amd.com wrote:
>
>On 2/2/23 11:58, Jiri Pirko wrote:
>> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:
>>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>>>
>>> Support for devlink info command.
>> You are quite brief for couple hundred line patch. Care to shed some
>> more details for the reader? Also, use imperative mood (applies to the
>> rest of the pathes)
>>
>> [...]
>>
>
>OK. I'll be more talkative and imperative here.
>
>>> +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;
>>> +	char msg[NETLINK_MAX_FMTMSG_LEN];
>>> +	int errors_reported = 0;
>>> +	int rc;
>>> +
>>> +	/* Several different MCDI commands are used. We report first error
>>> +	 * through extack along with total number of errors. Specific error
>>> +	 * information via system messages.
>>> +	 */
>>> +	rc = efx_devlink_info_board_cfg(efx, req);
>>> +	if (rc) {
>>> +		sprintf(msg, "Getting board info failed");
>>> +		errors_reported++;
>>> +	}
>>> +	rc = efx_devlink_info_stored_versions(efx, req);
>>> +	if (rc) {
>>> +		if (!errors_reported)
>>> +			sprintf(msg, "Getting stored versions failed");
>>> +		errors_reported += rc;
>>> +	}
>>> +	rc = efx_devlink_info_running_versions(efx, req);
>>> +	if (rc) {
>>> +		if (!errors_reported)
>>> +			sprintf(msg, "Getting board info failed");
>>> +		errors_reported++;
>>
>> Under which circumstances any of the errors above happen? Is it a common
>> thing? Or is it result of some fatal event?
>
>They are not common at all. If any of those happen, it is a bad sign, 
>and it is more than likely there are more than one because something is 
>not working properly. That is the reason I only report first error found 
>plus the total number of errors detected.
>
>
>>
>> You treat it like it is quite common, which seems very odd to me.
>> If they are rare, just return error right away to the caller.
>
>Well, that is done now. And as I say, I'm not reporting all but just the 
>first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.
>
>If errors trigger, a more complete information will appear in system 
>messages, so that is the reason with:
>
>+               NL_SET_ERR_MSG_FMT(extack,
>+                                  "%s. %d total errors. Check system messages",
>+                                  msg, errors_reported);
>
>I guess you are concerned with the extack report being overwhelmed, but 
>I do not think that is the case.

No, I'm wondering why you just don't put error message into exack and
return -ESOMEERROR right away.

>
>>
>>
>>> +	}
>>> +
>>> +	if (errors_reported)
>>> +		NL_SET_ERR_MSG_FMT(extack,
>>> +				   "%s. %d total errors. Check system messages",
>>> +				   msg, errors_reported);
>>> +	return 0;
>>> +}
>>> +
>>> static const struct devlink_ops sfc_devlink_ops = {
>>> +	.info_get			= efx_devlink_info_get,
>>> };
>> [...]
Lucero Palau, Alejandro Feb. 7, 2023, 3:10 p.m. UTC | #6
On 2/7/23 14:58, Jiri Pirko wrote:
> Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@amd.com wrote:
>> On 2/2/23 11:58, Jiri Pirko wrote:
>>> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:
>>>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>>>>
>>>> Support for devlink info command.
>>> You are quite brief for couple hundred line patch. Care to shed some
>>> more details for the reader? Also, use imperative mood (applies to the
>>> rest of the pathes)
>>>
>>> [...]
>>>
>> OK. I'll be more talkative and imperative here.
>>
>>>> +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;
>>>> +	char msg[NETLINK_MAX_FMTMSG_LEN];
>>>> +	int errors_reported = 0;
>>>> +	int rc;
>>>> +
>>>> +	/* Several different MCDI commands are used. We report first error
>>>> +	 * through extack along with total number of errors. Specific error
>>>> +	 * information via system messages.
>>>> +	 */
>>>> +	rc = efx_devlink_info_board_cfg(efx, req);
>>>> +	if (rc) {
>>>> +		sprintf(msg, "Getting board info failed");
>>>> +		errors_reported++;
>>>> +	}
>>>> +	rc = efx_devlink_info_stored_versions(efx, req);
>>>> +	if (rc) {
>>>> +		if (!errors_reported)
>>>> +			sprintf(msg, "Getting stored versions failed");
>>>> +		errors_reported += rc;
>>>> +	}
>>>> +	rc = efx_devlink_info_running_versions(efx, req);
>>>> +	if (rc) {
>>>> +		if (!errors_reported)
>>>> +			sprintf(msg, "Getting board info failed");
>>>> +		errors_reported++;
>>> Under which circumstances any of the errors above happen? Is it a common
>>> thing? Or is it result of some fatal event?
>> They are not common at all. If any of those happen, it is a bad sign,
>> and it is more than likely there are more than one because something is
>> not working properly. That is the reason I only report first error found
>> plus the total number of errors detected.
>>
>>
>>> You treat it like it is quite common, which seems very odd to me.
>>> If they are rare, just return error right away to the caller.
>> Well, that is done now. And as I say, I'm not reporting all but just the
>> first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.
>>
>> If errors trigger, a more complete information will appear in system
>> messages, so that is the reason with:
>>
>> +               NL_SET_ERR_MSG_FMT(extack,
>> +                                  "%s. %d total errors. Check system messages",
>> +                                  msg, errors_reported);
>>
>> I guess you are concerned with the extack report being overwhelmed, but
>> I do not think that is the case.
> No, I'm wondering why you just don't put error message into exack and
> return -ESOMEERROR right away.

Well, I thought the idea was to give more information to user space 
about the problem.

Previous patchsets were not reporting any error nor error information 
through extack. Now we have both.

>>>
>>>> +	}
>>>> +
>>>> +	if (errors_reported)
>>>> +		NL_SET_ERR_MSG_FMT(extack,
>>>> +				   "%s. %d total errors. Check system messages",
>>>> +				   msg, errors_reported);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> static const struct devlink_ops sfc_devlink_ops = {
>>>> +	.info_get			= efx_devlink_info_get,
>>>> };
>>> [...]
Lucero Palau, Alejandro Feb. 7, 2023, 5:24 p.m. UTC | #7
On 2/7/23 15:10, Lucero Palau, Alejandro wrote:
> On 2/7/23 14:58, Jiri Pirko wrote:
>> Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@amd.com wrote:
>>> On 2/2/23 11:58, Jiri Pirko wrote:
>>>> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:
>>>>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>>>>>
>>>>> Support for devlink info command.
>>>> You are quite brief for couple hundred line patch. Care to shed some
>>>> more details for the reader? Also, use imperative mood (applies to the
>>>> rest of the pathes)
>>>>
>>>> [...]
>>>>
>>> OK. I'll be more talkative and imperative here.
>>>
>>>>> +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;
>>>>> +	char msg[NETLINK_MAX_FMTMSG_LEN];
>>>>> +	int errors_reported = 0;
>>>>> +	int rc;
>>>>> +
>>>>> +	/* Several different MCDI commands are used. We report first error
>>>>> +	 * through extack along with total number of errors. Specific error
>>>>> +	 * information via system messages.
>>>>> +	 */
>>>>> +	rc = efx_devlink_info_board_cfg(efx, req);
>>>>> +	if (rc) {
>>>>> +		sprintf(msg, "Getting board info failed");
>>>>> +		errors_reported++;
>>>>> +	}
>>>>> +	rc = efx_devlink_info_stored_versions(efx, req);
>>>>> +	if (rc) {
>>>>> +		if (!errors_reported)
>>>>> +			sprintf(msg, "Getting stored versions failed");
>>>>> +		errors_reported += rc;
>>>>> +	}
>>>>> +	rc = efx_devlink_info_running_versions(efx, req);
>>>>> +	if (rc) {
>>>>> +		if (!errors_reported)
>>>>> +			sprintf(msg, "Getting board info failed");
>>>>> +		errors_reported++;
>>>> Under which circumstances any of the errors above happen? Is it a common
>>>> thing? Or is it result of some fatal event?
>>> They are not common at all. If any of those happen, it is a bad sign,
>>> and it is more than likely there are more than one because something is
>>> not working properly. That is the reason I only report first error found
>>> plus the total number of errors detected.
>>>
>>>
>>>> You treat it like it is quite common, which seems very odd to me.
>>>> If they are rare, just return error right away to the caller.
>>> Well, that is done now. And as I say, I'm not reporting all but just the
>>> first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.
>>>
>>> If errors trigger, a more complete information will appear in system
>>> messages, so that is the reason with:
>>>
>>> +               NL_SET_ERR_MSG_FMT(extack,
>>> +                                  "%s. %d total errors. Check system messages",
>>> +                                  msg, errors_reported);
>>>
>>> I guess you are concerned with the extack report being overwhelmed, but
>>> I do not think that is the case.
>> No, I'm wondering why you just don't put error message into exack and
>> return -ESOMEERROR right away.
> Well, I thought the idea was to give more information to user space
> about the problem.
>
> Previous patchsets were not reporting any error nor error information
> through extack. Now we have both.


Just trying to make more sense of this.

Because that limit with NETLINK_MAX_FMTMSG_LEN, what I think is big 
enough, some control needs to be taken about what to report. It could be 
just to write the buffer with the last error and report that last one 
only, with no need of keeping total errors count. But I felt once we 
handle any error, reporting that extra info about the total errors 
detected should not be a problem at all, even if it is an unlikely 
situation.

BTW, I said we were reporting both, the error and the extack error 
message, but I've realized the function was not returning any error but 
always 0, so I'll fix that.


>>>>> +	}
>>>>> +
>>>>> +	if (errors_reported)
>>>>> +		NL_SET_ERR_MSG_FMT(extack,
>>>>> +				   "%s. %d total errors. Check system messages",
>>>>> +				   msg, errors_reported);
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> static const struct devlink_ops sfc_devlink_ops = {
>>>>> +	.info_get			= efx_devlink_info_get,
>>>>> };
>>>> [...]
Jiri Pirko Feb. 8, 2023, 7:35 a.m. UTC | #8
Tue, Feb 07, 2023 at 06:24:05PM CET, alejandro.lucero-palau@amd.com wrote:
>
>On 2/7/23 15:10, Lucero Palau, Alejandro wrote:
>> On 2/7/23 14:58, Jiri Pirko wrote:
>>> Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@amd.com wrote:
>>>> On 2/2/23 11:58, Jiri Pirko wrote:
>>>>> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:
>>>>>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>>>>>>
>>>>>> Support for devlink info command.
>>>>> You are quite brief for couple hundred line patch. Care to shed some
>>>>> more details for the reader? Also, use imperative mood (applies to the
>>>>> rest of the pathes)
>>>>>
>>>>> [...]
>>>>>
>>>> OK. I'll be more talkative and imperative here.
>>>>
>>>>>> +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;
>>>>>> +	char msg[NETLINK_MAX_FMTMSG_LEN];
>>>>>> +	int errors_reported = 0;
>>>>>> +	int rc;
>>>>>> +
>>>>>> +	/* Several different MCDI commands are used. We report first error
>>>>>> +	 * through extack along with total number of errors. Specific error
>>>>>> +	 * information via system messages.
>>>>>> +	 */
>>>>>> +	rc = efx_devlink_info_board_cfg(efx, req);
>>>>>> +	if (rc) {
>>>>>> +		sprintf(msg, "Getting board info failed");
>>>>>> +		errors_reported++;
>>>>>> +	}
>>>>>> +	rc = efx_devlink_info_stored_versions(efx, req);
>>>>>> +	if (rc) {
>>>>>> +		if (!errors_reported)
>>>>>> +			sprintf(msg, "Getting stored versions failed");
>>>>>> +		errors_reported += rc;
>>>>>> +	}
>>>>>> +	rc = efx_devlink_info_running_versions(efx, req);
>>>>>> +	if (rc) {
>>>>>> +		if (!errors_reported)
>>>>>> +			sprintf(msg, "Getting board info failed");
>>>>>> +		errors_reported++;
>>>>> Under which circumstances any of the errors above happen? Is it a common
>>>>> thing? Or is it result of some fatal event?
>>>> They are not common at all. If any of those happen, it is a bad sign,
>>>> and it is more than likely there are more than one because something is
>>>> not working properly. That is the reason I only report first error found
>>>> plus the total number of errors detected.
>>>>
>>>>
>>>>> You treat it like it is quite common, which seems very odd to me.
>>>>> If they are rare, just return error right away to the caller.
>>>> Well, that is done now. And as I say, I'm not reporting all but just the
>>>> first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.
>>>>
>>>> If errors trigger, a more complete information will appear in system
>>>> messages, so that is the reason with:
>>>>
>>>> +               NL_SET_ERR_MSG_FMT(extack,
>>>> +                                  "%s. %d total errors. Check system messages",
>>>> +                                  msg, errors_reported);
>>>>
>>>> I guess you are concerned with the extack report being overwhelmed, but
>>>> I do not think that is the case.
>>> No, I'm wondering why you just don't put error message into exack and
>>> return -ESOMEERROR right away.
>> Well, I thought the idea was to give more information to user space
>> about the problem.
>>
>> Previous patchsets were not reporting any error nor error information
>> through extack. Now we have both.
>
>
>Just trying to make more sense of this.
>
>Because that limit with NETLINK_MAX_FMTMSG_LEN, what I think is big 
>enough, some control needs to be taken about what to report. It could be 
>just to write the buffer with the last error and report that last one 

Wait. My point is: fail on the first error returning the error to
info_get() caller. Just that. No accumulation of anything.


>only, with no need of keeping total errors count. But I felt once we 
>handle any error, reporting that extra info about the total errors 
>detected should not be a problem at all, even if it is an unlikely 
>situation.
>
>BTW, I said we were reporting both, the error and the extack error 
>message, but I've realized the function was not returning any error but 
>always 0, so I'll fix that.
>
>
>>>>>> +	}
>>>>>> +
>>>>>> +	if (errors_reported)
>>>>>> +		NL_SET_ERR_MSG_FMT(extack,
>>>>>> +				   "%s. %d total errors. Check system messages",
>>>>>> +				   msg, errors_reported);
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> static const struct devlink_ops sfc_devlink_ops = {
>>>>>> +	.info_get			= efx_devlink_info_get,
>>>>>> };
>>>>> [...]
Lucero Palau, Alejandro Feb. 8, 2023, 8:53 a.m. UTC | #9
On 2/8/23 07:35, Jiri Pirko wrote:
> Tue, Feb 07, 2023 at 06:24:05PM CET, alejandro.lucero-palau@amd.com wrote:
>> On 2/7/23 15:10, Lucero Palau, Alejandro wrote:
>>> On 2/7/23 14:58, Jiri Pirko wrote:
>>>> Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@amd.com wrote:
>>>>> On 2/2/23 11:58, Jiri Pirko wrote:
>>>>>> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:
>>>>>>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>>>>>>>
>>>>>>> Support for devlink info command.
>>>>>> You are quite brief for couple hundred line patch. Care to shed some
>>>>>> more details for the reader? Also, use imperative mood (applies to the
>>>>>> rest of the pathes)
>>>>>>
>>>>>> [...]
>>>>>>
>>>>> OK. I'll be more talkative and imperative here.
>>>>>
>>>>>>> +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;
>>>>>>> +	char msg[NETLINK_MAX_FMTMSG_LEN];
>>>>>>> +	int errors_reported = 0;
>>>>>>> +	int rc;
>>>>>>> +
>>>>>>> +	/* Several different MCDI commands are used. We report first error
>>>>>>> +	 * through extack along with total number of errors. Specific error
>>>>>>> +	 * information via system messages.
>>>>>>> +	 */
>>>>>>> +	rc = efx_devlink_info_board_cfg(efx, req);
>>>>>>> +	if (rc) {
>>>>>>> +		sprintf(msg, "Getting board info failed");
>>>>>>> +		errors_reported++;
>>>>>>> +	}
>>>>>>> +	rc = efx_devlink_info_stored_versions(efx, req);
>>>>>>> +	if (rc) {
>>>>>>> +		if (!errors_reported)
>>>>>>> +			sprintf(msg, "Getting stored versions failed");
>>>>>>> +		errors_reported += rc;
>>>>>>> +	}
>>>>>>> +	rc = efx_devlink_info_running_versions(efx, req);
>>>>>>> +	if (rc) {
>>>>>>> +		if (!errors_reported)
>>>>>>> +			sprintf(msg, "Getting board info failed");
>>>>>>> +		errors_reported++;
>>>>>> Under which circumstances any of the errors above happen? Is it a common
>>>>>> thing? Or is it result of some fatal event?
>>>>> They are not common at all. If any of those happen, it is a bad sign,
>>>>> and it is more than likely there are more than one because something is
>>>>> not working properly. That is the reason I only report first error found
>>>>> plus the total number of errors detected.
>>>>>
>>>>>
>>>>>> You treat it like it is quite common, which seems very odd to me.
>>>>>> If they are rare, just return error right away to the caller.
>>>>> Well, that is done now. And as I say, I'm not reporting all but just the
>>>>> first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.
>>>>>
>>>>> If errors trigger, a more complete information will appear in system
>>>>> messages, so that is the reason with:
>>>>>
>>>>> +               NL_SET_ERR_MSG_FMT(extack,
>>>>> +                                  "%s. %d total errors. Check system messages",
>>>>> +                                  msg, errors_reported);
>>>>>
>>>>> I guess you are concerned with the extack report being overwhelmed, but
>>>>> I do not think that is the case.
>>>> No, I'm wondering why you just don't put error message into exack and
>>>> return -ESOMEERROR right away.
>>> Well, I thought the idea was to give more information to user space
>>> about the problem.
>>>
>>> Previous patchsets were not reporting any error nor error information
>>> through extack. Now we have both.
>>
>> Just trying to make more sense of this.
>>
>> Because that limit with NETLINK_MAX_FMTMSG_LEN, what I think is big
>> enough, some control needs to be taken about what to report. It could be
>> just to write the buffer with the last error and report that last one
> Wait. My point is: fail on the first error returning the error to
> info_get() caller. Just that. No accumulation of anything.


OK. I'll do just that in v6.

Thanks


>
>> only, with no need of keeping total errors count. But I felt once we
>> handle any error, reporting that extra info about the total errors
>> detected should not be a problem at all, even if it is an unlikely
>> situation.
>>
>> BTW, I said we were reporting both, the error and the extack error
>> message, but I've realized the function was not returning any error but
>> always 0, so I'll fix that.
>>
>>
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (errors_reported)
>>>>>>> +		NL_SET_ERR_MSG_FMT(extack,
>>>>>>> +				   "%s. %d total errors. Check system messages",
>>>>>>> +				   msg, errors_reported);
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> static const struct devlink_ops sfc_devlink_ops = {
>>>>>>> +	.info_get			= efx_devlink_info_get,
>>>>>>> };
>>>>>> [...]
diff mbox series

Patch

diff --git a/Documentation/networking/devlink/sfc.rst b/Documentation/networking/devlink/sfc.rst
new file mode 100644
index 000000000000..a8c0eb69f0b6
--- /dev/null
+++ b/Documentation/networking/devlink/sfc.rst
@@ -0,0 +1,57 @@ 
+.. SPDX-License-Identifier: GPL-2.0
+
+===================
+sfc devlink support
+===================
+
+This document describes the devlink features implemented by the ``sfc``
+device driver for the ef100 device.
+
+Info versions
+=============
+
+The ``sfc`` driver reports the following versions
+
+.. list-table:: devlink info versions implemented
+    :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``fw.mgmt.suc``
+     - running
+     - For boards where the management function is split between multiple
+       control units, this is the SUC control unit's firmware version.
+   * - ``fw.mgmt.cmc``
+     - running
+    - For boards where the management function is split between multiple
+       control units, this is the CMC control unit's firmware version.
+   * - ``fpga.rev``
+     - running
+     - FPGA design revision.
+   * - ``fpga.app``
+     - running
+     - Datapath programmable logic version.
+   * - ``fw.app``
+     - running
+     - Datapath software/microcode/firmware version.
+   * - ``coproc.boot``
+     - running
+     - SmartNIC application co-processor (APU) first stage boot loader version.
+   * - ``coproc.uboot``
+     - running
+     - SmartNIC application co-processor (APU) co-operating system loader version.
+   * - ``coproc.main``
+     - running
+     - SmartNIC application co-processor (APU) main operating system version.
+   * - ``coproc.recovery``
+     - running
+     - SmartNIC application co-processor (APU) recovery operating system version.
+   * - ``fw.exprom``
+     - running
+     - Expansion ROM version. For boards where the expansion ROM is split between
+       multiple images (e.g. PXE and UEFI), this is the specifically the PXE boot
+       ROM version.
+   * - ``fw.uefi``
+     - running
+    - UEFI driver version (No UNDI support).
diff --git a/MAINTAINERS b/MAINTAINERS
index f82dd8d43c2b..f3ee2845e562 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18919,6 +18919,7 @@  M:	Edward Cree <ecree.xilinx@gmail.com>
 M:	Martin Habets <habetsm.xilinx@gmail.com>
 L:	netdev@vger.kernel.org
 S:	Supported
+F:	Documentation/networking/devlink/sfc.rst
 F:	drivers/net/ethernet/sfc/
 
 SFF/SFP/SFP+ MODULE SUPPORT
diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c
index 933e60876a93..ad6be8d09e96 100644
--- a/drivers/net/ethernet/sfc/efx_devlink.c
+++ b/drivers/net/ethernet/sfc/efx_devlink.c
@@ -21,7 +21,466 @@  struct efx_devlink {
 	struct efx_nic *efx;
 };
 
+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) {
+		netif_err(efx, drv, efx->net_dev, "mcdi nvram %s: failed\n",
+			  version_name);
+		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 int efx_devlink_info_stored_versions(struct efx_nic *efx,
+					    struct devlink_info_req *req)
+{
+	int errors = 0;
+	int rc;
+
+	rc = efx_devlink_info_nvram_partition(efx, req,
+					      NVRAM_PARTITION_TYPE_BUNDLE,
+					      DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID);
+	if (rc)
+		errors++;
+
+	rc = efx_devlink_info_nvram_partition(efx, req,
+					      NVRAM_PARTITION_TYPE_MC_FIRMWARE,
+					      DEVLINK_INFO_VERSION_GENERIC_FW_MGMT);
+	if (rc)
+		errors++;
+
+	rc = efx_devlink_info_nvram_partition(efx, req,
+					      NVRAM_PARTITION_TYPE_SUC_FIRMWARE,
+					      EFX_DEVLINK_INFO_VERSION_FW_MGMT_SUC);
+	if (rc)
+		errors++;
+
+	rc = efx_devlink_info_nvram_partition(efx, req,
+					      NVRAM_PARTITION_TYPE_EXPANSION_ROM,
+					      EFX_DEVLINK_INFO_VERSION_FW_EXPROM);
+	if (rc)
+		errors++;
+
+	rc = efx_devlink_info_nvram_partition(efx, req,
+					      NVRAM_PARTITION_TYPE_EXPANSION_UEFI,
+					      EFX_DEVLINK_INFO_VERSION_FW_UEFI);
+	if (rc)
+		errors++;
+
+	return errors;
+}
+
+#define EFX_VER_FLAG(_f)	\
+	(MC_CMD_GET_VERSION_V5_OUT_ ## _f ## _PRESENT_LBN)
+
+static void efx_devlink_info_running_v2(struct efx_nic *efx,
+					struct devlink_info_req *req,
+					unsigned int flags, efx_dword_t *outbuf)
+{
+	char buf[EFX_MAX_VERSION_INFO_LEN];
+	union {
+		const __le32 *dwords;
+		const __le16 *words;
+		const char *str;
+	} ver;
+	struct rtc_time build_date;
+	unsigned int build_id;
+	size_t offset;
+	u64 tstamp;
+
+	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);
+	}
+}
+
+static void efx_devlink_info_running_v3(struct efx_nic *efx,
+					struct devlink_info_req *req,
+					unsigned int flags, efx_dword_t *outbuf)
+{
+	char buf[EFX_MAX_VERSION_INFO_LEN];
+	union {
+		const __le32 *dwords;
+		const __le16 *words;
+		const char *str;
+	} ver;
+
+	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);
+	}
+}
+
+static void efx_devlink_info_running_v4(struct efx_nic *efx,
+					struct devlink_info_req *req,
+					unsigned int flags, efx_dword_t *outbuf)
+{
+	char buf[EFX_MAX_VERSION_INFO_LEN];
+	union {
+		const __le32 *dwords;
+		const __le16 *words;
+		const char *str;
+	} ver;
+
+	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);
+	}
+}
+
+static void efx_devlink_info_running_v5(struct efx_nic *efx,
+					struct devlink_info_req *req,
+					unsigned int flags, efx_dword_t *outbuf)
+{
+	char buf[EFX_MAX_VERSION_INFO_LEN];
+	union {
+		const __le32 *dwords;
+		const __le16 *words;
+		const char *str;
+	} ver;
+
+	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);
+	}
+}
+
+static int 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];
+	union {
+		const __le32 *dwords;
+		const __le16 *words;
+		const char *str;
+	} ver;
+	size_t outlength;
+	unsigned int flags;
+	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) {
+		netif_err(efx, drv, efx->net_dev,
+			  "mcdi MC_CMD_GET_VERSION failed\n");
+		return rc;
+	}
+
+	/* Handle previous output */
+	if (outlength < MC_CMD_GET_VERSION_V2_OUT_LEN) {
+		ver.words = (__le16 *)MCDI_PTR(outbuf,
+					       GET_VERSION_EXT_OUT_VERSION);
+		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 0;
+	}
+
+	/* Handle V2 additions */
+	flags = MCDI_DWORD(outbuf, GET_VERSION_V2_OUT_FLAGS);
+	efx_devlink_info_running_v2(efx, req, flags, outbuf);
+
+	if (outlength < MC_CMD_GET_VERSION_V3_OUT_LEN)
+		return 0;
+
+	/* Handle V3 additions */
+	efx_devlink_info_running_v3(efx, req, flags, outbuf);
+
+	if (outlength < MC_CMD_GET_VERSION_V4_OUT_LEN)
+		return 0;
+
+	/* Handle V4 additions */
+	efx_devlink_info_running_v4(efx, req, flags, outbuf);
+
+	if (outlength < MC_CMD_GET_VERSION_V5_OUT_LEN)
+		return 0;
+
+	/* Handle V5 additions */
+	efx_devlink_info_running_v5(efx, req, flags, outbuf);
+
+	return 0;
+}
+
+#define EFX_MAX_SERIALNUM_LEN	(ETH_ALEN * 2 + 1)
+
+static int 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);
+	}
+	return rc;
+}
+
+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;
+	char msg[NETLINK_MAX_FMTMSG_LEN];
+	int errors_reported = 0;
+	int rc;
+
+	/* Several different MCDI commands are used. We report first error
+	 * through extack along with total number of errors. Specific error
+	 * information via system messages.
+	 */
+	rc = efx_devlink_info_board_cfg(efx, req);
+	if (rc) {
+		sprintf(msg, "Getting board info failed");
+		errors_reported++;
+	}
+	rc = efx_devlink_info_stored_versions(efx, req);
+	if (rc) {
+		if (!errors_reported)
+			sprintf(msg, "Getting stored versions failed");
+		errors_reported += rc;
+	}
+	rc = efx_devlink_info_running_versions(efx, req);
+	if (rc) {
+		if (!errors_reported)
+			sprintf(msg, "Getting board info failed");
+		errors_reported++;
+	}
+
+	if (errors_reported)
+		NL_SET_ERR_MSG_FMT(extack,
+				   "%s. %d total errors. Check system messages",
+				   msg, errors_reported);
+	return 0;
+}
+
 static const struct devlink_ops sfc_devlink_ops = {
+	.info_get			= efx_devlink_info_get,
 };
 
 void efx_fini_devlink_lock(struct efx_nic *efx)
diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h
index 8ff85b035e87..a5269361c3e0 100644
--- a/drivers/net/ethernet/sfc/efx_devlink.h
+++ b/drivers/net/ethernet/sfc/efx_devlink.h
@@ -14,6 +14,23 @@ 
 #include "net_driver.h"
 #include <net/devlink.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
+
 int efx_probe_devlink_and_lock(struct efx_nic *efx);
 void efx_probe_devlink_unlock(struct efx_nic *efx);
 void efx_fini_devlink_lock(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c
index af338208eae9..a7f2c31071e8 100644
--- a/drivers/net/ethernet/sfc/mcdi.c
+++ b/drivers/net/ethernet/sfc/mcdi.c
@@ -2175,6 +2175,78 @@  int efx_mcdi_get_privilege_mask(struct efx_nic *efx, u32 *mask)
 	return 0;
 }
 
+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;
+}
+
 #ifdef CONFIG_SFC_MTD
 
 #define EFX_MCDI_NVRAM_LEN_MAX 128
diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h
index 7e35fec9da35..5cb202684858 100644
--- a/drivers/net/ethernet/sfc/mcdi.h
+++ b/drivers/net/ethernet/sfc/mcdi.h
@@ -378,6 +378,9 @@  int efx_mcdi_nvram_info(struct efx_nic *efx, unsigned int type,
 			size_t *size_out, size_t *erase_size_out,
 			bool *protected_out);
 int efx_new_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_nvram_test_all(struct efx_nic *efx);
 int efx_mcdi_handle_assertion(struct efx_nic *efx);
 int efx_mcdi_set_id_led(struct efx_nic *efx, enum efx_led_mode mode);