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

From: Jiri Pirko <jiri@resnulli.us>
To: Ido Schimmel <idosch@idosch.org>
Cc: Jakub Kicinski <kuba@kernel.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, 26 Apr 2022 08:47:37 +0200	[thread overview]
Message-ID: <YmeViVZ1XhCBCFLN@nanopsycho> (raw)
In-Reply-To: <Ymb5DQonnrnIBG3c@shredder>

Mon, Apr 25, 2022 at 09:39:57PM CEST, idosch@idosch.org wrote:
>On Mon, Apr 25, 2022 at 09:00:21AM -0700, Jakub Kicinski wrote:
>> On Mon, 25 Apr 2022 06:44:20 +0300 Ido Schimmel wrote:
>> > This patchset is extending the line card model by three items:
>> > 1) line card devices
>> > 2) line card info
>> > 3) line card device info
>> > 
>> > First three patches are introducing the necessary changes in devlink
>> > core.
>> > 
>> > Then, all three extensions are implemented in mlxsw alongside with
>> > selftest.
>> 
>> :/ 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.
>
>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
>
>Registering a separate devlink instance for these devices sounds like an

It is not a separate devlink device, not removetely. A devlink device is
attached to some bus on the host, it contains entities like netdevices,
etc.

Line card devices, on contrary, are accessible over ASIC FW interface,
they reside on line cards. ASIC FW is using build-in SDK to communicate
with them. There is really nothing to expose, except for the face they
are there, with some FW version and later on (follow-up patchset) to be
able to flash FW on them.

It's a gearbox. I found it odd to name it gearbox as in theory, there
might be some other single purpose device on the line card of other type
than gearbox. Therefore, "device". I admit it is a bit misleading. Idea
for a better name?

>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

We would need to add all the line card hierarchy into info_get. That
would look a bit odd to me.

>the current approach is cleaner.
>
>> Would you mind if I revert this?

Let's see what you need to change and change it in place, if possible.

>
>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.