Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

From: Jakub Kicinski <kuba@kernel.org>
To: Jiri Pirko <jiri@resnulli.us>
Cc: Ido Schimmel <idosch@idosch.org>,
	Ido Schimmel <idosch@nvidia.com>,
	netdev@vger.kernel.org, davem@davemloft.net, pabeni@redhat.com,
	jiri@nvidia.com, petrm@nvidia.com, dsahern@gmail.com,
	andrew@lunn.ch, mlxsw@nvidia.com
Subject: Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info
Date: Tue, 31 May 2022 09:08:52 -0700	[thread overview]
Message-ID: <20220531090852.2b10c344@kernel.org> (raw)
In-Reply-To: <YpY5iKHR073DNF7D@nanopsycho>

On Tue, 31 May 2022 17:51:36 +0200 Jiri Pirko wrote:
> Tue, May 31, 2022 at 05:05:55PM CEST, kuba@kernel.org wrote:
> >> Group of what? Could you provide me example what you mean?  
> >
> >Group of components. As explained component has an existing meaning,
> >we can't reuse the term with a different one now.  
> 
> I still don't follow. I don't want to reuse it.
> Really, could you be more specific and show examples, please?

What you had in your previous examples, just don't call it components
but come up with a new term:

$ devlink dev info
pci/0000:01:00.0:
  driver mlxsw_spectrum2
  versions:
      fixed:
        hw.revision A0
        fw.psid MT_0000000199
      running:
        fw.version 29.2010.2302
        fw 29.2010.2302
  groups? sections? subordinates? :                         <= here
    lc1:
      versions:
        fixed:
          hw.revision 0
          fw.psid MT_0000000111
        running:
          fw 19.2010.1310
          ini.version 4

Note that lc1 is not a nest at netlink level but user space can group
the attrs pretty trivially.

> >> But to be consistent with the output, we would have to change "devlink   
> >> dev info" to something like:                                             
> >> pci/0000:01:00.0:                                                        
> >>   versions:                                                              
> >>       running:                                                           
> >>         fw 1.2.3                                                         
> >>         fw.mgmt 10.20.30                                                 
> >>         lc 2 fw 19.2010.1310                                               
> >
> >Yup.  
> 
> Set, you say "yup" but below you say it should be in a separate nest.
> That is confusing me.

Ah, sorry. I hope the above is clear.

> >> But that would break the existing JSON output, because "running" is an array:
> >>                 "running": {                                             
> >>                     "fw": "1.2.3",                                       
> >>                     "fw.mgmt": "10.20.30"                                
> >>                 },                                                         
> >
> >No, the lc versions should be in separate nests. Since they are not
> >updated when flashing main FW mixing them into existing versions would
> >break uAPI.  
> 
> Could you please draw it out for me exacly as you envision it? We are
> dancing around it, I can't really understand what exactly do you mean.

Why would I prototype your feature for you? I prefer a separate dl
instance. If you want to explore other options the "drawing out" is
on you :/

> >> So probably better to stick to "lcx.y" notation in both devlink dev info
> >> and flash and split/squash to attributes internally. What do you think?  
> >
> >BTW how do you intend to activate the new FW? Extend the reload command?  
> 
> Not sure now. Extending reload is an option. Have to think about it.

😑