[PATCH net-next v2 0/4] nfp: series of minor driver improvements

[PATCH net-next v2 0/4] nfp: series of minor driver improvements

[Louis Peens]

This short series bundles two unrelated but small updates to the nfp
driver.

Patch1: Add new defines for devlink string paramaters
Patch2: Make use of these two fields in the nfp driver
Patch3: Add new 'dim' profiles
Patch4: Make use of these new profiles in the nfp driver

Changes since V1:
- Move nfp local defines to devlink common code as it is quite generic.
- Add new 'dim' profile instead of using driver local overrides, as this
  allows use of the 'dim' helpers.
- This expanded 2 patches to 4, as the common code changes are split
  into seperate patches.

Fei Qin (4):
  devlink: add two info version tags
  nfp: update devlink device info output
  dim: introduce a specific dim profile for better latency
  nfp: use new dim profiles for better latency

 .../networking/devlink/devlink-info.rst        | 10 ++++++++++
 Documentation/networking/devlink/nfp.rst       |  3 +++
 .../net/ethernet/netronome/nfp/nfp_devlink.c   |  3 ++-
 .../ethernet/netronome/nfp/nfp_net_common.c    |  4 ++--
 include/linux/dim.h                            |  2 ++
 include/net/devlink.h                          |  5 +++++
 lib/dim/net_dim.c                              | 18 ++++++++++++++++++
 7 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/4] devlink: add two info version tags

[Louis Peens]

From: Fei Qin <fei.qin@corigine.com>

Add definition and documentation for the new generic
info "board.model" and "part_number".

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 Documentation/networking/devlink/devlink-info.rst | 10 ++++++++++
 include/net/devlink.h                             |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
index 1242b0e6826b..e663975a6b19 100644
--- a/Documentation/networking/devlink/devlink-info.rst
+++ b/Documentation/networking/devlink/devlink-info.rst
@@ -146,6 +146,11 @@ board.manufacture
 
 An identifier of the company or the facility which produced the part.
 
+board.model
+-----------
+
+Board design model.
+
 fw
 --
 
@@ -203,6 +208,11 @@ fw.bootloader
 
 Version of the bootloader.
 
+part_number
+-----------
+
+Part number of the entire product.
+
 Future work
 ===========
 
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 9ac394bdfbe4..edcd7a1f7068 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -605,6 +605,8 @@ enum devlink_param_generic_id {
 #define DEVLINK_INFO_VERSION_GENERIC_BOARD_REV	"board.rev"
 /* Maker of the board */
 #define DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE	"board.manufacture"
+/* Model of the board */
+#define DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL       "board.model"
 
 /* Part number, identifier of asic design */
 #define DEVLINK_INFO_VERSION_GENERIC_ASIC_ID	"asic.id"
@@ -632,6 +634,9 @@ enum devlink_param_generic_id {
 /* Bootloader */
 #define DEVLINK_INFO_VERSION_GENERIC_FW_BOOTLOADER	"fw.bootloader"
 
+/* Part number for entire product */
+#define DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER       "part_number"
+
 /**
  * struct devlink_flash_update_params - Flash Update parameters
  * @fw: pointer to the firmware data to update from
-- 
2.34.1

[PATCH net-next v2 2/4] nfp: update devlink device info output

[Louis Peens]

From: Fei Qin <fei.qin@corigine.com>

Newer NIC will introduce a new part number, now add it
into devlink device info.

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 Documentation/networking/devlink/nfp.rst         | 3 +++
 drivers/net/ethernet/netronome/nfp/nfp_devlink.c | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/devlink/nfp.rst b/Documentation/networking/devlink/nfp.rst
index a1717db0dfcc..f79d46472012 100644
--- a/Documentation/networking/devlink/nfp.rst
+++ b/Documentation/networking/devlink/nfp.rst
@@ -42,6 +42,9 @@ The ``nfp`` driver reports the following versions
    * - ``board.model``
      - fixed
      - Model name of the board design
+   * - ``part_number``
+     - fixed
+     - Part number of the entire product
    * - ``fw.bundle_id``
      - stored, running
      - Firmware bundle id
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 635d33c0d6d3..5b41338d55c4 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -159,7 +159,8 @@ static const struct nfp_devlink_versions_simple {
 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,	"assembly.partno", },
 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,	"assembly.revision", },
 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE, "assembly.vendor", },
-	{ "board.model", /* code name */		"assembly.model", },
+	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL,	"assembly.model", },
+	{ DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER,     "pn", },
 };
 
 static int
-- 
2.34.1

[PATCH net-next v2 3/4] dim: introduce a specific dim profile for better latency

[Louis Peens]

From: Fei Qin <fei.qin@corigine.com>

The current profile is not well-adaptive to NFP NICs in
terms of latency, so introduce a specific profile for better
latency.

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 include/linux/dim.h |  2 ++
 lib/dim/net_dim.c   | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/linux/dim.h b/include/linux/dim.h
index f343bc9aa2ec..edd6d7bceb28 100644
--- a/include/linux/dim.h
+++ b/include/linux/dim.h
@@ -119,11 +119,13 @@ struct dim {
  *
  * @DIM_CQ_PERIOD_MODE_START_FROM_EQE: Start counting from EQE
  * @DIM_CQ_PERIOD_MODE_START_FROM_CQE: Start counting from CQE (implies timer reset)
+ * @DIM_CQ_PERIOD_MODE_SPECIFIC_0: Specific mode to improve latency
  * @DIM_CQ_PERIOD_NUM_MODES: Number of modes
  */
 enum dim_cq_period_mode {
 	DIM_CQ_PERIOD_MODE_START_FROM_EQE = 0x0,
 	DIM_CQ_PERIOD_MODE_START_FROM_CQE = 0x1,
+	DIM_CQ_PERIOD_MODE_SPECIFIC_0 = 0x2,
 	DIM_CQ_PERIOD_NUM_MODES
 };
 
diff --git a/lib/dim/net_dim.c b/lib/dim/net_dim.c
index 4e32f7aaac86..2b5dccb6242c 100644
--- a/lib/dim/net_dim.c
+++ b/lib/dim/net_dim.c
@@ -33,6 +33,14 @@
 	{.usec = 64, .pkts = 64,}               \
 }
 
+#define NET_DIM_RX_SPECIFIC_0_PROFILES { \
+	{.usec = 0,   .pkts = 1,},   \
+	{.usec = 4,   .pkts = 32,},  \
+	{.usec = 64,  .pkts = 64,},  \
+	{.usec = 128, .pkts = 256,}, \
+	{.usec = 256, .pkts = 256,}  \
+}
+
 #define NET_DIM_TX_EQE_PROFILES { \
 	{.usec = 1,   .pkts = NET_DIM_DEFAULT_TX_CQ_PKTS_FROM_EQE,},  \
 	{.usec = 8,   .pkts = NET_DIM_DEFAULT_TX_CQ_PKTS_FROM_EQE,},  \
@@ -49,16 +57,26 @@
 	{.usec = 64, .pkts = 32,}   \
 }
 
+#define NET_DIM_TX_SPECIFIC_0_PROFILES { \
+	{.usec = 0,   .pkts = 1,},   \
+	{.usec = 4,   .pkts = 16,},  \
+	{.usec = 32,  .pkts = 64,},  \
+	{.usec = 64,  .pkts = 128,}, \
+	{.usec = 128, .pkts = 128,}  \
+}
+
 static const struct dim_cq_moder
 rx_profile[DIM_CQ_PERIOD_NUM_MODES][NET_DIM_PARAMS_NUM_PROFILES] = {
 	NET_DIM_RX_EQE_PROFILES,
 	NET_DIM_RX_CQE_PROFILES,
+	NET_DIM_RX_SPECIFIC_0_PROFILES,
 };
 
 static const struct dim_cq_moder
 tx_profile[DIM_CQ_PERIOD_NUM_MODES][NET_DIM_PARAMS_NUM_PROFILES] = {
 	NET_DIM_TX_EQE_PROFILES,
 	NET_DIM_TX_CQE_PROFILES,
+	NET_DIM_TX_SPECIFIC_0_PROFILES,
 };
 
 struct dim_cq_moder
-- 
2.34.1

[PATCH net-next v2 4/4] nfp: use new dim profiles for better latency

[Louis Peens]

From: Fei Qin <fei.qin@corigine.com>

Latency comparison between EQE profiles and SPECIFIC_0 profiles
for 5 different runs:

                                     Latency (us)
EQE profiles 	    |	132.85  136.32  131.31  131.37  133.51
SPECIFIC_0 profiles |	92.09   92.16   95.58   98.26   89.79

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index f28e769e6fda..064753b25b92 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -1232,12 +1232,12 @@ static void nfp_net_open_stack(struct nfp_net *nn)
 
 		if (r_vec->rx_ring) {
 			INIT_WORK(&r_vec->rx_dim.work, nfp_net_rx_dim_work);
-			r_vec->rx_dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_EQE;
+			r_vec->rx_dim.mode = DIM_CQ_PERIOD_MODE_SPECIFIC_0;
 		}
 
 		if (r_vec->tx_ring) {
 			INIT_WORK(&r_vec->tx_dim.work, nfp_net_tx_dim_work);
-			r_vec->tx_dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_EQE;
+			r_vec->tx_dim.mode = DIM_CQ_PERIOD_MODE_SPECIFIC_0;
 		}
 
 		napi_enable(&r_vec->napi);
-- 
2.34.1

Re: [PATCH net-next v2 1/4] devlink: add two info version tags

[Jiri Pirko]

Wed, Feb 28, 2024 at 08:51:37AM CET, louis.peens@corigine.com wrote:
>From: Fei Qin <fei.qin@corigine.com>
>
>Add definition and documentation for the new generic
>info "board.model" and "part_number".
>
>Signed-off-by: Fei Qin <fei.qin@corigine.com>
>Signed-off-by: Louis Peens <louis.peens@corigine.com>
>---
> Documentation/networking/devlink/devlink-info.rst | 10 ++++++++++
> include/net/devlink.h                             |  5 +++++
> 2 files changed, 15 insertions(+)
>
>diff --git a/Documentation/networking/devlink/devlink-info.rst b/Documentation/networking/devlink/devlink-info.rst
>index 1242b0e6826b..e663975a6b19 100644
>--- a/Documentation/networking/devlink/devlink-info.rst
>+++ b/Documentation/networking/devlink/devlink-info.rst
>@@ -146,6 +146,11 @@ board.manufacture
> 
> An identifier of the company or the facility which produced the part.
> 
>+board.model
>+-----------
>+
>+Board design model.
>+
> fw
> --
> 
>@@ -203,6 +208,11 @@ fw.bootloader
> 
> Version of the bootloader.
> 
>+part_number
>+-----------
>+
>+Part number of the entire product.
>+
> Future work
> ===========
> 
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 9ac394bdfbe4..edcd7a1f7068 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -605,6 +605,8 @@ enum devlink_param_generic_id {
> #define DEVLINK_INFO_VERSION_GENERIC_BOARD_REV	"board.rev"
> /* Maker of the board */
> #define DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE	"board.manufacture"
>+/* Model of the board */
>+#define DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL       "board.model"
> 
> /* Part number, identifier of asic design */
> #define DEVLINK_INFO_VERSION_GENERIC_ASIC_ID	"asic.id"
>@@ -632,6 +634,9 @@ enum devlink_param_generic_id {
> /* Bootloader */
> #define DEVLINK_INFO_VERSION_GENERIC_FW_BOOTLOADER	"fw.bootloader"
> 
>+/* Part number for entire product */
>+#define DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER       "part_number"

/* Part number, identifier of board design */
#define DEVLINK_INFO_VERSION_GENERIC_BOARD_ID   "board.id"

Isn't this what you are looking for?

"part_number" without domain (boards/asic/fw) does not look correct to
me. "Product" sounds very odd.

pw-bot: cr



>+
> /**
>  * struct devlink_flash_update_params - Flash Update parameters
>  * @fw: pointer to the firmware data to update from
>-- 
>2.34.1
>
>

Re: [PATCH net-next v2 2/4] nfp: update devlink device info output

[Jiri Pirko]

Wed, Feb 28, 2024 at 08:51:38AM CET, louis.peens@corigine.com wrote:
>From: Fei Qin <fei.qin@corigine.com>
>
>Newer NIC will introduce a new part number, now add it
>into devlink device info.
>
>Signed-off-by: Fei Qin <fei.qin@corigine.com>
>Signed-off-by: Louis Peens <louis.peens@corigine.com>
>---
> Documentation/networking/devlink/nfp.rst         | 3 +++
> drivers/net/ethernet/netronome/nfp/nfp_devlink.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/Documentation/networking/devlink/nfp.rst b/Documentation/networking/devlink/nfp.rst
>index a1717db0dfcc..f79d46472012 100644
>--- a/Documentation/networking/devlink/nfp.rst
>+++ b/Documentation/networking/devlink/nfp.rst
>@@ -42,6 +42,9 @@ The ``nfp`` driver reports the following versions
>    * - ``board.model``
>      - fixed
>      - Model name of the board design
>+   * - ``part_number``
>+     - fixed
>+     - Part number of the entire product
>    * - ``fw.bundle_id``
>      - stored, running
>      - Firmware bundle id
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>index 635d33c0d6d3..5b41338d55c4 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>@@ -159,7 +159,8 @@ static const struct nfp_devlink_versions_simple {
> 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,	"assembly.partno", },
> 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,	"assembly.revision", },
> 	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE, "assembly.vendor", },
>-	{ "board.model", /* code name */		"assembly.model", },
>+	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL,	"assembly.model", },
>+	{ DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER,     "pn", },

Could this be 2 patches? One logical change per patch please.


> };
> 
> static int
>-- 
>2.34.1
>
>

Re: [PATCH net-next v2 1/4] devlink: add two info version tags

[Jakub Kicinski]

On Wed, 28 Feb 2024 13:14:43 +0100 Jiri Pirko wrote:
> >+/* Part number for entire product */
> >+#define DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER       "part_number"  
> 
> /* Part number, identifier of board design */
> #define DEVLINK_INFO_VERSION_GENERIC_BOARD_ID   "board.id"
> 
> Isn't this what you are looking for?

My memory is fading but AFAIR when I added the other IDs, back in my
Netronome days, the expectation was that they would be combined
together to form the part number.

Not sure why they need a separate one now, maybe they lost the docs,
maybe requirements changed. Would be good to know... :)

> "part_number" without domain (boards/asic/fw) does not look correct to
> me. "Product" sounds very odd.

I believe Part Number is what PCI VPD calls it.

In addition to Jiri's questions:

> +/* Model of the board */
> +#define DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL       "board.model"

What's the difference between this and:

 board.id
 --------
 
 Unique identifier of the board design.

? One is AMDA the other one is code name?
You gotta provide more guidance how the two differ...

Re: [PATCH net-next v2 2/4] nfp: update devlink device info output

[Jakub Kicinski]

On Wed, 28 Feb 2024 09:51:38 +0200 Louis Peens wrote:
> +   * - ``part_number``
> +     - fixed
> +     - Part number of the entire product

Belongs in the previous patch..

>     * - ``fw.bundle_id``
>       - stored, running
>       - Firmware bundle id
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> index 635d33c0d6d3..5b41338d55c4 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> @@ -159,7 +159,8 @@ static const struct nfp_devlink_versions_simple {
>  	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,	"assembly.partno", },
>  	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,	"assembly.revision", },
>  	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE, "assembly.vendor", },
> -	{ "board.model", /* code name */		"assembly.model", },
> +	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL,	"assembly.model", },

Ah, it is the code name. I don't understand why you're trying to make
this generic. I never seen other vendors report code names for boards.

Re: [PATCH net-next v2 1/4] devlink: add two info version tags

[Louis Peens]

On Wed, Feb 28, 2024 at 08:32:35PM -0800, Jakub Kicinski wrote:
> On Wed, 28 Feb 2024 13:14:43 +0100 Jiri Pirko wrote:
> > >+/* Part number for entire product */
> > >+#define DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER       "part_number"  
> > 
> > /* Part number, identifier of board design */
> > #define DEVLINK_INFO_VERSION_GENERIC_BOARD_ID   "board.id"
> > 
> > Isn't this what you are looking for?
> 
> My memory is fading but AFAIR when I added the other IDs, back in my
> Netronome days, the expectation was that they would be combined
> together to form the part number.
> 
> Not sure why they need a separate one now, maybe they lost the docs,
> maybe requirements changed. Would be good to know... :)
Hi Jiri, Jakub, sorry for the delay in coming back to this. It did
indeed trigger a bit of an internal discussion about which number is
where and for what purpose. More detail at the end.
> 
> > "part_number" without domain (boards/asic/fw) does not look correct to
> > me. "Product" sounds very odd.
> 
> I believe Part Number is what PCI VPD calls it.
This is indeed where the decision to use "part_number" is coming from.
> 
> In addition to Jiri's questions:
> 
> > +/* Model of the board */
> > +#define DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL       "board.model"
> 
> What's the difference between this and:
> 
>  board.id
>  --------
>  
>  Unique identifier of the board design.
> 
> ? One is AMDA the other one is code name?
> You gotta provide more guidance how the two differ...
Carefully looking at this again revealed that "board.model" is indeed
the code name, and it probably shouldn't be a generic field. Or if it is
it should named better, and be called something like
'DEVLINK_INFO_VERSION_GENERIC_BOARD_CODENAME' instead. I do not know why
this was added in the first place, maybe it was a requirement at some
point, and I'm hesitant to just remove the user visible field now. But I
am happy to keep it local to the driver - it was moved based on Jiri's
suggestion - should have provided a better explanation then.

"board.id" is not the same thing as "part_number", or at least not closely
related anymore. Perhaps it was previously, but I can't find any
information on this.

"board.id" is the AMDA number, something like AMDA2001-1003, and it gets
combined with a few other fields to generate the product_id, exposed as
the devlink serial_number field. The AMDA number that is provided in the
"board.id" field is still used to identify firmware names from the
driver side.

"part_number" looks like this: CGX11-A2PSNM. This is a number that is
printed on the board, and also a field that can get exposed over BMC by
products that supports this.

While Fei was preparing the patch there was discussion about using
"board.id" instead for the part_number as they do seem quite similar in
purpose. The reason why we proposed a new field instead was that the
information in "board.id" can still be helpful for identification. If
there are some scripts out there that uses the "board.id" field, for
example to build up a firmware pathname, replacing it with the
"part_number" will break those.

V1 of the series did also have the "part_number" in the driver only,
Jiri requested moving it to devlink itself.

Proposal for V3 - Move both fields back to driver-only, and greatly
improve the commit message to explain the difference. Does this sound
reasonable?

Re: [PATCH net-next v2 2/4] nfp: update devlink device info output

[Louis Peens]

On Wed, Feb 28, 2024 at 08:34:58PM -0800, Jakub Kicinski wrote:
> On Wed, 28 Feb 2024 09:51:38 +0200 Louis Peens wrote:
> > +   * - ``part_number``
> > +     - fixed
> > +     - Part number of the entire product
> 
> Belongs in the previous patch..
> 
> >     * - ``fw.bundle_id``
> >       - stored, running
> >       - Firmware bundle id
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > index 635d33c0d6d3..5b41338d55c4 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > @@ -159,7 +159,8 @@ static const struct nfp_devlink_versions_simple {
> >  	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,	"assembly.partno", },
> >  	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_REV,	"assembly.revision", },
> >  	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MANUFACTURE, "assembly.vendor", },
> > -	{ "board.model", /* code name */		"assembly.model", },
> > +	{ DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL,	"assembly.model", },
> 
> Ah, it is the code name. I don't understand why you're trying to make
> this generic. I never seen other vendors report code names for boards.
> 
Mostly expanded on this in my previous reply of patch 1, but yes, I
agree. Should have pushed back on review of V1 where moving it to
VERSION_GENERIC was suggested. The one alternative is to still make it
generic, but give it a better name to make it clear that it is a
codename. Still, I don't know if that is generally helpful, happy to
keep it driver only.

Re: [PATCH net-next v2 1/4] devlink: add two info version tags

[Jiri Pirko]

Thu, Mar 07, 2024 at 09:17:27AM CET, louis.peens@corigine.com wrote:
>On Wed, Feb 28, 2024 at 08:32:35PM -0800, Jakub Kicinski wrote:
>> On Wed, 28 Feb 2024 13:14:43 +0100 Jiri Pirko wrote:
>> > >+/* Part number for entire product */
>> > >+#define DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER       "part_number"  
>> > 
>> > /* Part number, identifier of board design */
>> > #define DEVLINK_INFO_VERSION_GENERIC_BOARD_ID   "board.id"
>> > 
>> > Isn't this what you are looking for?
>> 
>> My memory is fading but AFAIR when I added the other IDs, back in my
>> Netronome days, the expectation was that they would be combined
>> together to form the part number.
>> 
>> Not sure why they need a separate one now, maybe they lost the docs,
>> maybe requirements changed. Would be good to know... :)
>Hi Jiri, Jakub, sorry for the delay in coming back to this. It did
>indeed trigger a bit of an internal discussion about which number is
>where and for what purpose. More detail at the end.
>> 
>> > "part_number" without domain (boards/asic/fw) does not look correct to
>> > me. "Product" sounds very odd.
>> 
>> I believe Part Number is what PCI VPD calls it.
>This is indeed where the decision to use "part_number" is coming from.
>> 
>> In addition to Jiri's questions:
>> 
>> > +/* Model of the board */
>> > +#define DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL       "board.model"
>> 
>> What's the difference between this and:
>> 
>>  board.id
>>  --------
>>  
>>  Unique identifier of the board design.
>> 
>> ? One is AMDA the other one is code name?
>> You gotta provide more guidance how the two differ...
>Carefully looking at this again revealed that "board.model" is indeed
>the code name, and it probably shouldn't be a generic field. Or if it is
>it should named better, and be called something like
>'DEVLINK_INFO_VERSION_GENERIC_BOARD_CODENAME' instead. I do not know why
>this was added in the first place, maybe it was a requirement at some
>point, and I'm hesitant to just remove the user visible field now. But I
>am happy to keep it local to the driver - it was moved based on Jiri's
>suggestion - should have provided a better explanation then.
>
>"board.id" is not the same thing as "part_number", or at least not closely
>related anymore. Perhaps it was previously, but I can't find any
>information on this.
>
>"board.id" is the AMDA number, something like AMDA2001-1003, and it gets
>combined with a few other fields to generate the product_id, exposed as
>the devlink serial_number field. The AMDA number that is provided in the
>"board.id" field is still used to identify firmware names from the
>driver side.
>
>"part_number" looks like this: CGX11-A2PSNM. This is a number that is
>printed on the board, and also a field that can get exposed over BMC by
>products that supports this.
>
>While Fei was preparing the patch there was discussion about using
>"board.id" instead for the part_number as they do seem quite similar in
>purpose. The reason why we proposed a new field instead was that the
>information in "board.id" can still be helpful for identification. If
>there are some scripts out there that uses the "board.id" field, for
>example to build up a firmware pathname, replacing it with the
>"part_number" will break those.
>
>V1 of the series did also have the "part_number" in the driver only,
>Jiri requested moving it to devlink itself.
>
>Proposal for V3 - Move both fields back to driver-only, and greatly
>improve the commit message to explain the difference. Does this sound
>reasonable?

Why the "part_number" is specific to you? You say it is a board
attribute, why don't you have:

#define DEVLINK_INFO_VERSION_GENERIC_BOARD_PART_NUMBER       "board.part_number"
?

Re: [PATCH net-next v2 1/4] devlink: add two info version tags

[Louis Peens]

On Thu, Mar 07, 2024 at 09:40:51AM +0100, Jiri Pirko wrote:
> Thu, Mar 07, 2024 at 09:17:27AM CET, louis.peens@corigine.com wrote:
> >On Wed, Feb 28, 2024 at 08:32:35PM -0800, Jakub Kicinski wrote:
> >> On Wed, 28 Feb 2024 13:14:43 +0100 Jiri Pirko wrote:
> >> > >+/* Part number for entire product */
> >> > >+#define DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER       "part_number"  
> >> > 
> >> > /* Part number, identifier of board design */
> >> > #define DEVLINK_INFO_VERSION_GENERIC_BOARD_ID   "board.id"
> >> > 
> >> > Isn't this what you are looking for?
> >> 
> >> My memory is fading but AFAIR when I added the other IDs, back in my
> >> Netronome days, the expectation was that they would be combined
> >> together to form the part number.
> >> 
> >> Not sure why they need a separate one now, maybe they lost the docs,
> >> maybe requirements changed. Would be good to know... :)
> >Hi Jiri, Jakub, sorry for the delay in coming back to this. It did
> >indeed trigger a bit of an internal discussion about which number is
> >where and for what purpose. More detail at the end.
> >> 
> >> > "part_number" without domain (boards/asic/fw) does not look correct to
> >> > me. "Product" sounds very odd.
> >> 
> >> I believe Part Number is what PCI VPD calls it.
> >This is indeed where the decision to use "part_number" is coming from.
> >> 
> >> In addition to Jiri's questions:
> >> 
> >> > +/* Model of the board */
> >> > +#define DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL       "board.model"
> >> 
> >> What's the difference between this and:
> >> 
> >>  board.id
> >>  --------
> >>  
> >>  Unique identifier of the board design.
> >> 
> >> ? One is AMDA the other one is code name?
> >> You gotta provide more guidance how the two differ...
> >Carefully looking at this again revealed that "board.model" is indeed
> >the code name, and it probably shouldn't be a generic field. Or if it is
> >it should named better, and be called something like
> >'DEVLINK_INFO_VERSION_GENERIC_BOARD_CODENAME' instead. I do not know why
> >this was added in the first place, maybe it was a requirement at some
> >point, and I'm hesitant to just remove the user visible field now. But I
> >am happy to keep it local to the driver - it was moved based on Jiri's
> >suggestion - should have provided a better explanation then.
> >
> >"board.id" is not the same thing as "part_number", or at least not closely
> >related anymore. Perhaps it was previously, but I can't find any
> >information on this.
> >
> >"board.id" is the AMDA number, something like AMDA2001-1003, and it gets
> >combined with a few other fields to generate the product_id, exposed as
> >the devlink serial_number field. The AMDA number that is provided in the
> >"board.id" field is still used to identify firmware names from the
> >driver side.
> >
> >"part_number" looks like this: CGX11-A2PSNM. This is a number that is
> >printed on the board, and also a field that can get exposed over BMC by
> >products that supports this.
> >
> >While Fei was preparing the patch there was discussion about using
> >"board.id" instead for the part_number as they do seem quite similar in
> >purpose. The reason why we proposed a new field instead was that the
> >information in "board.id" can still be helpful for identification. If
> >there are some scripts out there that uses the "board.id" field, for
> >example to build up a firmware pathname, replacing it with the
> >"part_number" will break those.
> >
> >V1 of the series did also have the "part_number" in the driver only,
> >Jiri requested moving it to devlink itself.
> >
> >Proposal for V3 - Move both fields back to driver-only, and greatly
> >improve the commit message to explain the difference. Does this sound
> >reasonable?
> 
> Why the "part_number" is specific to you? You say it is a board
> attribute, why don't you have:
> 
> #define DEVLINK_INFO_VERSION_GENERIC_BOARD_PART_NUMBER       "board.part_number"
> ?
I don't know if it is specific to us, that's the thing, maybe it is,
maybe it isn't. What I do know in our case is that "part_number" and
"board.id" is not the same thing, so we would prefer to have both visible.
I cannot comment if that is the case for others, if the concensus is
that others will find this helpful we don't mind adding it to devlink,
as we've done from v1 to v2 - exact naming disussion aside.

Updated proposal after this comment, V3 would then be:
1) Keep "board.model" (the codename) local like it currently exist
in-tree.
2) Do still add "part_number" to devlink, but call it "board.part_number".
Looking at the existing options it probably does fit the closest with
board, it's not "fw", and I don't think it's "asic" either.

Does this sound like the right track?

Re: [PATCH net-next v2 1/4] devlink: add two info version tags

[Jiri Pirko]

Thu, Mar 07, 2024 at 12:06:54PM CET, louis.peens@corigine.com wrote:
>On Thu, Mar 07, 2024 at 09:40:51AM +0100, Jiri Pirko wrote:
>> Thu, Mar 07, 2024 at 09:17:27AM CET, louis.peens@corigine.com wrote:
>> >On Wed, Feb 28, 2024 at 08:32:35PM -0800, Jakub Kicinski wrote:
>> >> On Wed, 28 Feb 2024 13:14:43 +0100 Jiri Pirko wrote:
>> >> > >+/* Part number for entire product */
>> >> > >+#define DEVLINK_INFO_VERSION_GENERIC_PART_NUMBER       "part_number"  
>> >> > 
>> >> > /* Part number, identifier of board design */
>> >> > #define DEVLINK_INFO_VERSION_GENERIC_BOARD_ID   "board.id"
>> >> > 
>> >> > Isn't this what you are looking for?
>> >> 
>> >> My memory is fading but AFAIR when I added the other IDs, back in my
>> >> Netronome days, the expectation was that they would be combined
>> >> together to form the part number.
>> >> 
>> >> Not sure why they need a separate one now, maybe they lost the docs,
>> >> maybe requirements changed. Would be good to know... :)
>> >Hi Jiri, Jakub, sorry for the delay in coming back to this. It did
>> >indeed trigger a bit of an internal discussion about which number is
>> >where and for what purpose. More detail at the end.
>> >> 
>> >> > "part_number" without domain (boards/asic/fw) does not look correct to
>> >> > me. "Product" sounds very odd.
>> >> 
>> >> I believe Part Number is what PCI VPD calls it.
>> >This is indeed where the decision to use "part_number" is coming from.
>> >> 
>> >> In addition to Jiri's questions:
>> >> 
>> >> > +/* Model of the board */
>> >> > +#define DEVLINK_INFO_VERSION_GENERIC_BOARD_MODEL       "board.model"
>> >> 
>> >> What's the difference between this and:
>> >> 
>> >>  board.id
>> >>  --------
>> >>  
>> >>  Unique identifier of the board design.
>> >> 
>> >> ? One is AMDA the other one is code name?
>> >> You gotta provide more guidance how the two differ...
>> >Carefully looking at this again revealed that "board.model" is indeed
>> >the code name, and it probably shouldn't be a generic field. Or if it is
>> >it should named better, and be called something like
>> >'DEVLINK_INFO_VERSION_GENERIC_BOARD_CODENAME' instead. I do not know why
>> >this was added in the first place, maybe it was a requirement at some
>> >point, and I'm hesitant to just remove the user visible field now. But I
>> >am happy to keep it local to the driver - it was moved based on Jiri's
>> >suggestion - should have provided a better explanation then.
>> >
>> >"board.id" is not the same thing as "part_number", or at least not closely
>> >related anymore. Perhaps it was previously, but I can't find any
>> >information on this.
>> >
>> >"board.id" is the AMDA number, something like AMDA2001-1003, and it gets
>> >combined with a few other fields to generate the product_id, exposed as
>> >the devlink serial_number field. The AMDA number that is provided in the
>> >"board.id" field is still used to identify firmware names from the
>> >driver side.
>> >
>> >"part_number" looks like this: CGX11-A2PSNM. This is a number that is
>> >printed on the board, and also a field that can get exposed over BMC by
>> >products that supports this.
>> >
>> >While Fei was preparing the patch there was discussion about using
>> >"board.id" instead for the part_number as they do seem quite similar in
>> >purpose. The reason why we proposed a new field instead was that the
>> >information in "board.id" can still be helpful for identification. If
>> >there are some scripts out there that uses the "board.id" field, for
>> >example to build up a firmware pathname, replacing it with the
>> >"part_number" will break those.
>> >
>> >V1 of the series did also have the "part_number" in the driver only,
>> >Jiri requested moving it to devlink itself.
>> >
>> >Proposal for V3 - Move both fields back to driver-only, and greatly
>> >improve the commit message to explain the difference. Does this sound
>> >reasonable?
>> 
>> Why the "part_number" is specific to you? You say it is a board
>> attribute, why don't you have:
>> 
>> #define DEVLINK_INFO_VERSION_GENERIC_BOARD_PART_NUMBER       "board.part_number"
>> ?
>I don't know if it is specific to us, that's the thing, maybe it is,
>maybe it isn't. What I do know in our case is that "part_number" and
>"board.id" is not the same thing, so we would prefer to have both visible.
>I cannot comment if that is the case for others, if the concensus is
>that others will find this helpful we don't mind adding it to devlink,
>as we've done from v1 to v2 - exact naming disussion aside.
>
>Updated proposal after this comment, V3 would then be:
>1) Keep "board.model" (the codename) local like it currently exist
>in-tree.
>2) Do still add "part_number" to devlink, but call it "board.part_number".
>Looking at the existing options it probably does fit the closest with
>board, it's not "fw", and I don't think it's "asic" either.
>
>Does this sound like the right track?

Okay.

Re: [PATCH net-next v2 1/4] devlink: add two info version tags

[Jakub Kicinski]

On Thu, 7 Mar 2024 12:32:48 +0100 Jiri Pirko wrote:
> >I don't know if it is specific to us, that's the thing, maybe it is,
> >maybe it isn't. What I do know in our case is that "part_number" and
> >"board.id" is not the same thing, so we would prefer to have both visible.
> >I cannot comment if that is the case for others, if the concensus is
> >that others will find this helpful we don't mind adding it to devlink,
> >as we've done from v1 to v2 - exact naming disussion aside.
> >
> >Updated proposal after this comment, V3 would then be:
> >1) Keep "board.model" (the codename) local like it currently exist
> >in-tree.
> >2) Do still add "part_number" to devlink, but call it "board.part_number".
> >Looking at the existing options it probably does fit the closest with
> >board, it's not "fw", and I don't think it's "asic" either.
> >
> >Does this sound like the right track?  
> 
> Okay.

+1