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

From: Jakub Kicinski <kuba@kernel.org>
To: Ido Schimmel <idosch@idosch.org>
Cc: 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: Mon, 25 Apr 2022 12:52:18 -0700	[thread overview]
Message-ID: <20220425125218.7caa473f@kernel.org> (raw)
In-Reply-To: <Ymb5DQonnrnIBG3c@shredder>

On Mon, 25 Apr 2022 22:39:57 +0300 Ido Schimmel wrote:
> > :/ what is a line card device? You must provide document what you're
> > doing, this:
> > 
> >  .../networking/devlink/devlink-linecard.rst   |   4 +
> > 
> > is not enough.
> > 
> > How many operations and attributes are you going to copy&paste?
> > 
> > Is linking devlink instances into a hierarchy a better approach?  
> 
> In this particular case, these devices are gearboxes. They are running
> their own firmware and we want user space to be able to query and update
> the running firmware version.

Nothing too special, then, we don't create "devices" for every
component of the system which can have a separate FW. That's where
"components" are intended to be used..

> The idea (implemented in the next patchset) is to let these devices
> expose their own "component name", which can then be plugged into the
> existing flash command:
> 
>     $ devlink lc show pci/0000:01:00.0 lc 8
>     pci/0000:01:00.0:
>       lc 8 state active type 16x100G
>         supported_types:
>            16x100G
>         devices:
>           device 0 flashable true component lc8_dev0
>           device 1 flashable false
>           device 2 flashable false
>           device 3 flashable false
>     $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0

IDK if it's just me or this assumes deep knowledge of the system.
I don't understand why we need to list devices 1-3 at all. And they
don't even have names. No information is exposed. 

There are many components on any networking device, including plenty
40G-R4 -> 25G-R1 gearboxes out there.

> Registering a separate devlink instance for these devices sounds like an
> overkill to me. If you are not OK with a separate command (e.g.,
> DEVLINK_CMD_LINECARD_INFO_GET), then extending DEVLINK_CMD_INFO_GET is
> also an option. We discussed this during internal review, but felt that
> the current approach is cleaner.

I don't know what you have queued, so if you don't need a full devlink
instance (IOW line cards won't need more individual config) that's fine.
For just FW flashing you can list many devices and update the
components... no need to introduce new objects or uAPI.

> > Would you mind if I revert this?  
> 
> I can't stop you, but keep in mind that it's already late here and that
> tomorrow I'm AFK (reserve duty) and won't be able to tag it. Jiri should
> be available to continue this discussion tomorrow morning, so probably
> best to wait for his feedback.

Sure, no rush.