[PATCH net-next 00/11] bnxt_en updates to devlink cmd

[PATCH net-next 00/11] bnxt_en updates to devlink cmd

[Vasundhara Volam]

This series adds support for some of the generic as well as driver
specific macros to devlink info_get cb. Updates the devlink-info.rst
and bnxt.rst documentation accordingly.

Patchset also adds support for enable_ecn generic parameter and its
usage in bnxt_en driver.

Pavan Chebbi (2):
  devlink: Add new enable_ecn generic device param
  bnxt_en: Use "enable_ecn" devlink param

Vasundhara Volam (9):
  devlink: add macro for "drv.spec"
  bnxt_en: Add driver HWRM spec version to devlink info_get cb
  devlink: add macro for "hw.addr"
  bnxt_en: Refactor bnxt_hwrm_get_nvm_cfg_ver()
  bnxt_en: Add hw addr and multihost base hw addr to devlink info_get
    cb.
  PCI: Add new PCI_VPD_RO_KEYWORD_SERIALNO macro
  bnxt_en: Read partno and serialno of the board from VPD
  bnxt_en: Add partno and serialno to devlink info_get cb
  bnxt_en: Update firmware interface spec to 1.10.1.26.

 Documentation/networking/devlink/bnxt.rst          |  18 ++
 Documentation/networking/devlink/devlink-info.rst  |  11 +
 .../networking/devlink/devlink-params.rst          |   5 +
 drivers/net/ethernet/broadcom/bnxt/bnxt.c          |  59 ++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.h          |   4 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 168 +++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |   3 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 307 +++++++++++++++++++--
 include/linux/pci.h                                |   1 +
 include/net/devlink.h                              |  10 +
 net/core/devlink.c                                 |   5 +
 11 files changed, 546 insertions(+), 45 deletions(-)

-- 
1.8.3.1

[PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Vasundhara Volam]

Add definition and documentation for the new generic info "drv.spec".
"drv.spec" specifies the version of the software interfaces between
driver and firmware.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/devlink-info.rst | 6 ++++++
 include/net/devlink.h                             | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
index 70981dd..0765a48 100644
--- a/Documentation/networking/devlink/devlink-info.rst
+++ b/Documentation/networking/devlink/devlink-info.rst
@@ -59,6 +59,12 @@ board.manufacture
 
 An identifier of the company or the facility which produced the part.
 
+drv.spec
+--------
+
+Firmware interface specification version of the software interfaces between
+driver and firmware. This tag displays spec version implemented by driver.
+
 fw
 --
 
diff --git a/include/net/devlink.h b/include/net/devlink.h
index c9ca86b..9c4d889 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -476,6 +476,9 @@ enum devlink_param_generic_id {
 /* Revision of asic design */
 #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV	"asic.rev"
 
+/* FW interface specification version implemented by driver */
+#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC	"drv.spec"
+
 /* Overall FW version */
 #define DEVLINK_INFO_VERSION_GENERIC_FW		"fw"
 /* Control processor FW version */
-- 
1.8.3.1

[PATCH net-next 02/11] bnxt_en: Add driver HWRM spec version to devlink info_get cb

[Vasundhara Volam]

Also update bnxt.rst documentation file.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/bnxt.rst         | 3 +++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
index 82ef9ec..2709161 100644
--- a/Documentation/networking/devlink/bnxt.rst
+++ b/Documentation/networking/devlink/bnxt.rst
@@ -57,6 +57,9 @@ The ``bnxt_en`` driver reports the following versions
    * - ``asic.rev``
      - fixed
      - ASIC design revision
+   * - ``drv.spec``
+     - running
+     - HWRM specification version supported by driver HWRM implementation
    * - ``fw.psid``
      - stored, running
      - Firmware parameter set version of the board
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index d3c93cc..4a623ff 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -425,6 +425,11 @@ static int bnxt_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 			return rc;
 	}
 
+	rc = devlink_info_version_running_put(req,
+		DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC, HWRM_VERSION_STR);
+	if (rc)
+		return rc;
+
 	if (strlen(ver_resp->active_pkg_name)) {
 		rc =
 		    devlink_info_version_running_put(req,
-- 
1.8.3.1

[PATCH net-next 03/11] devlink: add macro for "hw.addr"

[Vasundhara Volam]

Add definition and documentation for the new generic info "hw.addr".
"hw.addr" displays the hardware address of the interface.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/devlink-info.rst | 5 +++++
 include/net/devlink.h                             | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
index 0765a48..25658f2 100644
--- a/Documentation/networking/devlink/devlink-info.rst
+++ b/Documentation/networking/devlink/devlink-info.rst
@@ -65,6 +65,11 @@ drv.spec
 Firmware interface specification version of the software interfaces between
 driver and firmware. This tag displays spec version implemented by driver.
 
+hw.addr
+-------
+
+Hardware address of the interface.
+
 fw
 --
 
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 9c4d889..e130d24 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -479,6 +479,9 @@ enum devlink_param_generic_id {
 /* FW interface specification version implemented by driver */
 #define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC	"drv.spec"
 
+/* Hardware address */
+#define DEVLINK_INFO_VERSION_GENERIC_HW_ADDR	"hw.addr"
+
 /* Overall FW version */
 #define DEVLINK_INFO_VERSION_GENERIC_FW		"fw"
 /* Control processor FW version */
-- 
1.8.3.1

[PATCH net-next 04/11] bnxt_en: Refactor bnxt_hwrm_get_nvm_cfg_ver()

[Vasundhara Volam]

Refactor bnxt_hwrm_get_nvm_cfg_ver() and move hwrm specific
code to bnxt_hwrm_nvm_get_var(), to be called multiple times.

Also, rename bnxt_hwrm_get_nvm_cfg_ver() to bnxt_get_nvm_cfg_ver()

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 27 +++++++++++++++--------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 4a623ff..f08db49 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -359,25 +359,34 @@ static void bnxt_copy_from_nvm_data(union devlink_param_value *dst,
 		dst->vu8 = (u8)val32;
 }
 
-static int bnxt_hwrm_get_nvm_cfg_ver(struct bnxt *bp,
-				     union devlink_param_value *nvm_cfg_ver)
+static int bnxt_hwrm_nvm_get_var(struct bnxt *bp, dma_addr_t data_dma_addr,
+				 u16 offset, u16 num_bits)
 {
 	struct hwrm_nvm_get_variable_input req = {0};
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
+	req.dest_data_addr = cpu_to_le64(data_dma_addr);
+	req.option_num = cpu_to_le16(offset);
+	req.data_len = cpu_to_le16(num_bits);
+
+	return hwrm_send_message_silent(bp, &req, sizeof(req),
+					HWRM_CMD_TIMEOUT);
+}
+
+static int bnxt_get_nvm_cfg_ver(struct bnxt *bp,
+				union devlink_param_value *nvm_cfg_ver)
+{
 	union bnxt_nvm_data *data;
 	dma_addr_t data_dma_addr;
 	int rc;
 
-	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
 	data = dma_alloc_coherent(&bp->pdev->dev, sizeof(*data),
 				  &data_dma_addr, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	req.dest_data_addr = cpu_to_le64(data_dma_addr);
-	req.data_len = cpu_to_le16(BNXT_NVM_CFG_VER_BITS);
-	req.option_num = cpu_to_le16(NVM_OFF_NVM_CFG_VER);
-
-	rc = hwrm_send_message_silent(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+	rc = bnxt_hwrm_nvm_get_var(bp, data_dma_addr, NVM_OFF_NVM_CFG_VER,
+				   BNXT_NVM_CFG_VER_BITS);
 	if (!rc)
 		bnxt_copy_from_nvm_data(nvm_cfg_ver, data,
 					BNXT_NVM_CFG_VER_BITS,
@@ -439,7 +448,7 @@ static int bnxt_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 			return rc;
 	}
 
-	if (BNXT_PF(bp) && !bnxt_hwrm_get_nvm_cfg_ver(bp, &nvm_cfg_ver)) {
+	if (BNXT_PF(bp) && !bnxt_get_nvm_cfg_ver(bp, &nvm_cfg_ver)) {
 		u32 ver = nvm_cfg_ver.vu32;
 
 		sprintf(buf, "%X.%X.%X", (ver >> 16) & 0xF, (ver >> 8) & 0xF,
-- 
1.8.3.1

[PATCH net-next 05/11] bnxt_en: Add hw addr and multihost base hw addr to devlink info_get cb.

[Vasundhara Volam]

In most of the scenarios, device serial number is not supported. So
MAC address is used for proper asset tracking by cloud customers. In
case of multihost NICs, base MAC address is unique for entire NIC and
this can be used for asset tracking. Add the multihost base MAC address
and interface MAC address information to info_get command.

Also update bnxt.rst documentation file.

Example display:

$ devlink dev info pci/0000:3b:00.1
pci/0000:3b:00.1:
  driver bnxt_en
  serial_number B0-26-28-FF-FE-C8-85-20
  versions:
      fixed:
        asic.id 1750
        asic.rev 1
      running:
        drv.spec 1.10.1.12
        hw.addr b0:26:28:c8:85:21
        hw.mh_base_addr b0:26:28:c8:85:20
        fw 216.0.286.0
        fw.psid 0.0.6
        fw.app 216.0.251.0

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/bnxt.rst         |  7 ++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 40 +++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  2 ++
 3 files changed, 49 insertions(+)

diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
index 2709161..f850a18 100644
--- a/Documentation/networking/devlink/bnxt.rst
+++ b/Documentation/networking/devlink/bnxt.rst
@@ -60,6 +60,13 @@ The ``bnxt_en`` driver reports the following versions
    * - ``drv.spec``
      - running
      - HWRM specification version supported by driver HWRM implementation
+   * - ``hw.addr``
+     - stored, running
+     - Hardware address of the interface
+   * - ``hw.mh_base_addr``
+     - stored, running
+     - Base hardware address of the multihost NIC. Displayed only on multihost
+       system
    * - ``fw.psid``
      - stored, running
      - Firmware parameter set version of the board
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index f08db49..607e27a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -396,6 +396,32 @@ static int bnxt_get_nvm_cfg_ver(struct bnxt *bp,
 	return rc;
 }
 
+static int bnxt_get_mh_base_addr(struct bnxt *bp, u8 *base_addr)
+{
+	dma_addr_t data_dma_addr;
+	__le32 *data;
+	int rc;
+
+	data = dma_alloc_coherent(&bp->pdev->dev, 2 * sizeof(*data),
+				  &data_dma_addr, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	rc = bnxt_hwrm_nvm_get_var(bp, data_dma_addr, NVM_OFF_MAC_ADDR,
+				   BNXT_NVM_MAC_ADDR_BITS);
+	if (!rc) {
+		u32 mac_hi = le32_to_cpu(data[0]);
+		u32 mac_lo = le32_to_cpu(data[1]);
+
+		sprintf(base_addr, "%02x:%02x:%02x:%02x:%02x:%02x",
+			(u8)(mac_hi >> 8), (u8)(mac_hi), (u8)(mac_lo >> 24),
+			(u8)(mac_lo >> 16), (u8)(mac_lo >> 8), (u8)(mac_lo));
+	}
+	dma_free_coherent(&bp->pdev->dev, 2 * sizeof(*data), data,
+			  data_dma_addr);
+	return rc;
+}
+
 static int bnxt_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 			    struct netlink_ext_ack *extack)
 {
@@ -405,6 +431,7 @@ static int bnxt_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 	char mgmt_ver[FW_VER_STR_LEN];
 	char roce_ver[FW_VER_STR_LEN];
 	char fw_ver[FW_VER_STR_LEN];
+	u8 mh_base_addr[ETH_ALEN];
 	char buf[32];
 	int rc;
 
@@ -439,6 +466,19 @@ static int bnxt_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 	if (rc)
 		return rc;
 
+	sprintf(buf, "%pM", bp->dev->dev_addr);
+	rc = devlink_info_version_running_put(req,
+				DEVLINK_INFO_VERSION_GENERIC_HW_ADDR, buf);
+	if (rc)
+		return rc;
+
+	if (BNXT_MH(bp) && !bnxt_get_mh_base_addr(bp, &mh_base_addr[0])) {
+		rc = devlink_info_version_running_put(req, "hw.mh_base_addr",
+						      mh_base_addr);
+		if (rc)
+			return rc;
+	}
+
 	if (strlen(ver_resp->active_pkg_name)) {
 		rc =
 		    devlink_info_version_running_put(req,
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index 95f893f..e720b1d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -33,6 +33,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
 	}
 }
 
+#define NVM_OFF_MAC_ADDR		1
 #define NVM_OFF_MSIX_VEC_PER_PF_MAX	108
 #define NVM_OFF_MSIX_VEC_PER_PF_MIN	114
 #define NVM_OFF_IGNORE_ARI		164
@@ -40,6 +41,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
 #define NVM_OFF_ENABLE_SRIOV		401
 #define NVM_OFF_NVM_CFG_VER		602
 
+#define BNXT_NVM_MAC_ADDR_BITS		64
 #define BNXT_NVM_CFG_VER_BITS		24
 #define BNXT_NVM_CFG_VER_BYTES		4
 
-- 
1.8.3.1

Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Jakub Kicinski]

On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote:
> Add definition and documentation for the new generic info "drv.spec".
> "drv.spec" specifies the version of the software interfaces between
> driver and firmware.
> 
> Cc: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  Documentation/networking/devlink/devlink-info.rst | 6 ++++++
>  include/net/devlink.h                             | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
> index 70981dd..0765a48 100644
> --- a/Documentation/networking/devlink/devlink-info.rst
> +++ b/Documentation/networking/devlink/devlink-info.rst
> @@ -59,6 +59,12 @@ board.manufacture
>  
>  An identifier of the company or the facility which produced the part.
>  
> +drv.spec
> +--------
> +
> +Firmware interface specification version of the software interfaces between

Why did you call this "drv" if the first sentence of the description
says it's a property of the firmware?

Upcoming Intel patches call this "fw.mgmt.api". Please use that name,
it makes far more sense.

> +driver and firmware. This tag displays spec version implemented by driver.
> +
>  fw
>  --
>  
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index c9ca86b..9c4d889 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -476,6 +476,9 @@ enum devlink_param_generic_id {
>  /* Revision of asic design */
>  #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV	"asic.rev"
>  
> +/* FW interface specification version implemented by driver */
> +#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC	"drv.spec"
> +
>  /* Overall FW version */
>  #define DEVLINK_INFO_VERSION_GENERIC_FW		"fw"
>  /* Control processor FW version */

Re: [PATCH net-next 05/11] bnxt_en: Add hw addr and multihost base hw addr to devlink info_get cb.

[Jakub Kicinski]

On Tue, 17 Mar 2020 20:44:42 +0530 Vasundhara Volam wrote:
> In most of the scenarios, device serial number is not supported. So
> MAC address is used for proper asset tracking by cloud customers. In
> case of multihost NICs, base MAC address is unique for entire NIC and
> this can be used for asset tracking. Add the multihost base MAC address
> and interface MAC address information to info_get command.
> 
> Also update bnxt.rst documentation file.
> 
> Example display:
> 
> $ devlink dev info pci/0000:3b:00.1
> pci/0000:3b:00.1:
>   driver bnxt_en
>   serial_number B0-26-28-FF-FE-C8-85-20
>   versions:
>       fixed:
>         asic.id 1750
>         asic.rev 1
>       running:
>         drv.spec 1.10.1.12
>         hw.addr b0:26:28:c8:85:21
>         hw.mh_base_addr b0:26:28:c8:85:20
>         fw 216.0.286.0
>         fw.psid 0.0.6
>         fw.app 216.0.251.0
> 
> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Nack. 

Make a proper serial number for the device, the point of common
Linux interfaces is to abstract differences between vendors. You
basically say "Broadcom is special, we will use our own identifier".

Also how is this a running version if it's supposed to be used for
asset management.

Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Jacob Keller]

On 3/17/2020 10:40 AM, Jakub Kicinski wrote:
> On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote:
>> Add definition and documentation for the new generic info "drv.spec".
>> "drv.spec" specifies the version of the software interfaces between
>> driver and firmware.
>>
>> Cc: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> ---
>>  Documentation/networking/devlink/devlink-info.rst | 6 ++++++
>>  include/net/devlink.h                             | 3 +++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
>> index 70981dd..0765a48 100644
>> --- a/Documentation/networking/devlink/devlink-info.rst
>> +++ b/Documentation/networking/devlink/devlink-info.rst
>> @@ -59,6 +59,12 @@ board.manufacture
>>  
>>  An identifier of the company or the facility which produced the part.
>>  
>> +drv.spec
>> +--------
>> +
>> +Firmware interface specification version of the software interfaces between
> 
> Why did you call this "drv" if the first sentence of the description
> says it's a property of the firmware?
> 
> Upcoming Intel patches call this "fw.mgmt.api". Please use that name,
> it makes far more sense.
> 

Yep, I think this is equivalent to "fw.mgmt.api" as well.

If we want to make this a standard name, I'm fine with that, and we can
update the ice driver to use the macro.

Thanks,
Jake

>> +driver and firmware. This tag displays spec version implemented by driver.
>> +
>>  fw
>>  --
>>  
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index c9ca86b..9c4d889 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -476,6 +476,9 @@ enum devlink_param_generic_id {
>>  /* Revision of asic design */
>>  #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV	"asic.rev"
>>  
>> +/* FW interface specification version implemented by driver */
>> +#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC	"drv.spec"
>> +
>>  /* Overall FW version */
>>  #define DEVLINK_INFO_VERSION_GENERIC_FW		"fw"
>>  /* Control processor FW version */
> 

Re: [PATCH net-next 05/11] bnxt_en: Add hw addr and multihost base hw addr to devlink info_get cb.

[Vasundhara Volam]

On Tue, Mar 17, 2020 at 11:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 17 Mar 2020 20:44:42 +0530 Vasundhara Volam wrote:
> > In most of the scenarios, device serial number is not supported. So
> > MAC address is used for proper asset tracking by cloud customers. In
> > case of multihost NICs, base MAC address is unique for entire NIC and
> > this can be used for asset tracking. Add the multihost base MAC address
> > and interface MAC address information to info_get command.
> >
> > Also update bnxt.rst documentation file.
> >
> > Example display:
> >
> > $ devlink dev info pci/0000:3b:00.1
> > pci/0000:3b:00.1:
> >   driver bnxt_en
> >   serial_number B0-26-28-FF-FE-C8-85-20
> >   versions:
> >       fixed:
> >         asic.id 1750
> >         asic.rev 1
> >       running:
> >         drv.spec 1.10.1.12
> >         hw.addr b0:26:28:c8:85:21
> >         hw.mh_base_addr b0:26:28:c8:85:20
> >         fw 216.0.286.0
> >         fw.psid 0.0.6
> >         fw.app 216.0.251.0
> >
> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>
> Nack.
>
> Make a proper serial number for the device, the point of common
> Linux interfaces is to abstract differences between vendors. You
> basically say "Broadcom is special, we will use our own identifier".
I thought only couple of vendors support multi-host NICs, so made this
macro as vendor specific. If it will be widely used, I will make it a generic
macro.

>
> Also how is this a running version if it's supposed to be used for
> asset management.
My mistake, will fix it to fixed version.

Thanks,
Vasundhara

Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Vasundhara Volam]

On Tue, Mar 17, 2020 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote:
> > Add definition and documentation for the new generic info "drv.spec".
> > "drv.spec" specifies the version of the software interfaces between
> > driver and firmware.
> >
> > Cc: Jiri Pirko <jiri@mellanox.com>
> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > ---
> >  Documentation/networking/devlink/devlink-info.rst | 6 ++++++
> >  include/net/devlink.h                             | 3 +++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
> > index 70981dd..0765a48 100644
> > --- a/Documentation/networking/devlink/devlink-info.rst
> > +++ b/Documentation/networking/devlink/devlink-info.rst
> > @@ -59,6 +59,12 @@ board.manufacture
> >
> >  An identifier of the company or the facility which produced the part.
> >
> > +drv.spec
> > +--------
> > +
> > +Firmware interface specification version of the software interfaces between
>
> Why did you call this "drv" if the first sentence of the description
> says it's a property of the firmware?
Since it is a version of interface between driver and firmware. Both
driver and firmware
can support different versions. I intend to display the version
implemented in the driver.

I can probably add for both driver and firmware. Add it is as drv.spec
and fw.spec.

Thanks,
Vasundhara
>
> Upcoming Intel patches call this "fw.mgmt.api". Please use that name,
> it makes far more sense.
>
> > +driver and firmware. This tag displays spec version implemented by driver.
> > +
> >  fw
> >  --
> >
> > diff --git a/include/net/devlink.h b/include/net/devlink.h
> > index c9ca86b..9c4d889 100644
> > --- a/include/net/devlink.h
> > +++ b/include/net/devlink.h
> > @@ -476,6 +476,9 @@ enum devlink_param_generic_id {
> >  /* Revision of asic design */
> >  #define DEVLINK_INFO_VERSION_GENERIC_ASIC_REV        "asic.rev"
> >
> > +/* FW interface specification version implemented by driver */
> > +#define DEVLINK_INFO_VERSION_GENERIC_DRV_SPEC        "drv.spec"
> > +
> >  /* Overall FW version */
> >  #define DEVLINK_INFO_VERSION_GENERIC_FW              "fw"
> >  /* Control processor FW version */
>

Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Jakub Kicinski]

On Wed, 18 Mar 2020 09:51:29 +0530 Vasundhara Volam wrote:
> On Tue, Mar 17, 2020 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote:  
> > > Add definition and documentation for the new generic info "drv.spec".
> > > "drv.spec" specifies the version of the software interfaces between
> > > driver and firmware.
> > >
> > > Cc: Jiri Pirko <jiri@mellanox.com>
> > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > > ---
> > >  Documentation/networking/devlink/devlink-info.rst | 6 ++++++
> > >  include/net/devlink.h                             | 3 +++
> > >  2 files changed, 9 insertions(+)
> > >
> > > diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
> > > index 70981dd..0765a48 100644
> > > --- a/Documentation/networking/devlink/devlink-info.rst
> > > +++ b/Documentation/networking/devlink/devlink-info.rst
> > > @@ -59,6 +59,12 @@ board.manufacture
> > >
> > >  An identifier of the company or the facility which produced the part.
> > >
> > > +drv.spec
> > > +--------
> > > +
> > > +Firmware interface specification version of the software interfaces between  
> >
> > Why did you call this "drv" if the first sentence of the description
> > says it's a property of the firmware?  
>
> Since it is a version of interface between driver and firmware. Both
> driver and firmware
> can support different versions. I intend to display the version
> implemented in the driver.

We're just getting rid of driver versions, with significant effort,
so starting to extend devlink info with driver stuff seems risky.
How is driver information part of device info in the first place?

As you said good driver and firmware will be modular and backward
compatible, so what's the meaning of the API version?

This field is meaningless.

> I can probably add for both driver and firmware. Add it is as drv.spec
> and fw.spec.

Re: [PATCH net-next 05/11] bnxt_en: Add hw addr and multihost base hw addr to devlink info_get cb.

[Jakub Kicinski]

On Wed, 18 Mar 2020 09:46:46 +0530 Vasundhara Volam wrote:
> On Tue, Mar 17, 2020 at 11:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue, 17 Mar 2020 20:44:42 +0530 Vasundhara Volam wrote:  
> > > In most of the scenarios, device serial number is not supported. So
> > > MAC address is used for proper asset tracking by cloud customers. In
> > > case of multihost NICs, base MAC address is unique for entire NIC and
> > > this can be used for asset tracking. Add the multihost base MAC address
> > > and interface MAC address information to info_get command.
> > >
> > > Also update bnxt.rst documentation file.
> > >
> > > Example display:
> > >
> > > $ devlink dev info pci/0000:3b:00.1
> > > pci/0000:3b:00.1:
> > >   driver bnxt_en
> > >   serial_number B0-26-28-FF-FE-C8-85-20
> > >   versions:
> > >       fixed:
> > >         asic.id 1750
> > >         asic.rev 1
> > >       running:
> > >         drv.spec 1.10.1.12
> > >         hw.addr b0:26:28:c8:85:21
> > >         hw.mh_base_addr b0:26:28:c8:85:20
> > >         fw 216.0.286.0
> > >         fw.psid 0.0.6
> > >         fw.app 216.0.251.0
> > >
> > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>  
> >
> > Nack.
> >
> > Make a proper serial number for the device, the point of common
> > Linux interfaces is to abstract differences between vendors. You
> > basically say "Broadcom is special, we will use our own identifier".  
> I thought only couple of vendors support multi-host NICs, so made this
> macro as vendor specific. If it will be widely used, I will make it a generic
> macro.

There is no use for the "base address" other than to identify the
device. Which is what serial numbers are for. If the "base address"
is unique, just put it in the serial number field.

Or. You actually don't have to do that because if I look at your commit
message - it's already the exact same value. Sigh.

> > Also how is this a running version if it's supposed to be used for
> > asset management.  
> My mistake, will fix it to fixed version.

Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Jacob Keller]

On 3/18/2020 1:04 PM, Jakub Kicinski wrote:
> On Wed, 18 Mar 2020 09:51:29 +0530 Vasundhara Volam wrote:
>> On Tue, Mar 17, 2020 at 11:10 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>> On Tue, 17 Mar 2020 20:44:38 +0530 Vasundhara Volam wrote:  
>>>> Add definition and documentation for the new generic info "drv.spec".
>>>> "drv.spec" specifies the version of the software interfaces between
>>>> driver and firmware.
>>>>
>>>> Cc: Jiri Pirko <jiri@mellanox.com>
>>>> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>> ---
>>>>  Documentation/networking/devlink/devlink-info.rst | 6 ++++++
>>>>  include/net/devlink.h                             | 3 +++
>>>>  2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
>>>> index 70981dd..0765a48 100644
>>>> --- a/Documentation/networking/devlink/devlink-info.rst
>>>> +++ b/Documentation/networking/devlink/devlink-info.rst
>>>> @@ -59,6 +59,12 @@ board.manufacture
>>>>
>>>>  An identifier of the company or the facility which produced the part.
>>>>
>>>> +drv.spec
>>>> +--------
>>>> +
>>>> +Firmware interface specification version of the software interfaces between  
>>>
>>> Why did you call this "drv" if the first sentence of the description
>>> says it's a property of the firmware?  
>>
>> Since it is a version of interface between driver and firmware. Both
>> driver and firmware
>> can support different versions. I intend to display the version
>> implemented in the driver.
> 
> We're just getting rid of driver versions, with significant effort,
> so starting to extend devlink info with driver stuff seems risky.
> How is driver information part of device info in the first place?
> 
> As you said good driver and firmware will be modular and backward
> compatible, so what's the meaning of the API version?
> 
> This field is meaningless.
> 

I think I agree with Jakub here. I assume, if it's anything like what
the ice driver does, the firmware has an API field used to communicate
to the driver what it can support. This can be used by the driver to
decide if it can load.

For example, if the major API number increases, the ice driver then
assumes that it must be a very old driver which will not work at all
with that firmware. (This is mostly kept as a safety hatch in case no
other alternative can be determined).

The driver can then use this API number as a way to decide if certain
features can be enabled or not.

I suppose printing the driver's "expected" API number makes sense, but I
think the stronger approach is to make the driver able to interoperate
with any previous API version. Newer minor API numbers only mean that
new features exist which the driver might not be aware of. (for example,
if you're running an old driver).

The only reason to care would be in the case where a major breaking
increase occurred. This really shouldn't be necessary, especially if the
API between firmware and driver is designed well, but could be useful as
a last ditch exit in case of some major breaking change that must be done.

Even then, your driver *should* be able to tell and then behave
differently based on this and do the old v1 or v<whatever> that it knows
the firmware CAN support.

In practice, I'm not sure how well this is actually done, as there is
always some maintenance burden for carrying multiple variations of
support, and in the case of a really poorly designed API.. it can be
quite a nightmare.

Thanks,
Jake

Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Michael Chan]

On Wed, Mar 18, 2020 at 5:05 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> On 3/18/2020 1:04 PM, Jakub Kicinski wrote:
> > We're just getting rid of driver versions, with significant effort,
> > so starting to extend devlink info with driver stuff seems risky.
> > How is driver information part of device info in the first place?
> >
> > As you said good driver and firmware will be modular and backward
> > compatible, so what's the meaning of the API version?
> >
> > This field is meaningless.
> >
>
> I think I agree with Jakub here. I assume, if it's anything like what
> the ice driver does, the firmware has an API field used to communicate
> to the driver what it can support. This can be used by the driver to
> decide if it can load.
>
> For example, if the major API number increases, the ice driver then
> assumes that it must be a very old driver which will not work at all
> with that firmware. (This is mostly kept as a safety hatch in case no
> other alternative can be determined).
>
> The driver can then use this API number as a way to decide if certain
> features can be enabled or not.
>
> I suppose printing the driver's "expected" API number makes sense, but I
> think the stronger approach is to make the driver able to interoperate
> with any previous API version. Newer minor API numbers only mean that
> new features exist which the driver might not be aware of. (for example,
> if you're running an old driver).
>

Agreed.  Our driver is backward and forward compatible with all
production firmware for the most part.  The idea is that the effective
API version number is the minimum of the driver's API and firmware's
API.  For example, if firmware is at v1.5 and driver is at v1.4, then
the effective or operating API is v1.4.  The new features after v1.4
are unused because the driver does not understand those new features.
Similarly, a newer driver running on older firmware will have the
older firmware's API as the effective API.  The driver will not use
the new features that the firmware doesn't understand.

So if there is only one API version to report, reporting the min.
makes the most sense to the user in our case.  It is similar to a Gen4
PCIe card currently operating in a Gen3 slot.

Thanks.

Re: [PATCH net-next 01/11] devlink: add macro for "drv.spec"

[Jakub Kicinski]

On Wed, 18 Mar 2020 17:47:26 -0700 Michael Chan wrote:
> On Wed, Mar 18, 2020 at 5:05 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> > On 3/18/2020 1:04 PM, Jakub Kicinski wrote:  
> > > We're just getting rid of driver versions, with significant effort,
> > > so starting to extend devlink info with driver stuff seems risky.
> > > How is driver information part of device info in the first place?
> > >
> > > As you said good driver and firmware will be modular and backward
> > > compatible, so what's the meaning of the API version?
> > >
> > > This field is meaningless.
> >
> > I think I agree with Jakub here. I assume, if it's anything like what
> > the ice driver does, the firmware has an API field used to communicate
> > to the driver what it can support. This can be used by the driver to
> > decide if it can load.
> >
> > For example, if the major API number increases, the ice driver then
> > assumes that it must be a very old driver which will not work at all
> > with that firmware. (This is mostly kept as a safety hatch in case no
> > other alternative can be determined).
> >
> > The driver can then use this API number as a way to decide if certain
> > features can be enabled or not.
> >
> > I suppose printing the driver's "expected" API number makes sense, but I
> > think the stronger approach is to make the driver able to interoperate
> > with any previous API version. Newer minor API numbers only mean that
> > new features exist which the driver might not be aware of. (for example,
> > if you're running an old driver).
> 
> Agreed.  Our driver is backward and forward compatible with all
> production firmware for the most part.  The idea is that the effective
> API version number is the minimum of the driver's API and firmware's
> API.  For example, if firmware is at v1.5 and driver is at v1.4, then
> the effective or operating API is v1.4.  The new features after v1.4
> are unused because the driver does not understand those new features.
> Similarly, a newer driver running on older firmware will have the
> older firmware's API as the effective API.  The driver will not use
> the new features that the firmware doesn't understand.
> 
> So if there is only one API version to report, reporting the min.
> makes the most sense to the user in our case.  It is similar to a Gen4
> PCIe card currently operating in a Gen3 slot.

Sounds reasonable.