Message-ID: <Zemfzkv4/FevHHfS@LouisNoVo>
Date: Thu, 7 Mar 2024 13:06:54 +0200
From: Louis Peens <louis.peens@...igine.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>,
	Paolo Abeni <pabeni@...hat.com>, Fei Qin <fei.qin@...igine.com>,
	netdev@...r.kernel.org, oss-drivers@...igine.com
Subject: Re: [PATCH net-next v2 1/4] devlink: add two info version tags

On Thu, Mar 07, 2024 at 09:40:51AM +0100, Jiri Pirko wrote:
> Thu, Mar 07, 2024 at 09:17:27AM CET, louis.peens@...igine.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?

Powered by blists - more mailing lists