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

[Jiri Pirko <jiri@resnulli.us>]

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.