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

[Jiri Pirko <jiri@resnulli.us>]

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"
?