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

[Louis Peens <louis.peens@corigine.com>]

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?