Date:   Tue, 26 Apr 2022 05:41:30 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Ido Schimmel <idosch@...sch.org>, Ido Schimmel <idosch@...dia.com>,
        netdev@...r.kernel.org, davem@...emloft.net, pabeni@...hat.com,
        jiri@...dia.com, petrm@...dia.com, dsahern@...il.com,
        andrew@...n.ch, mlxsw@...dia.com
Subject: Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices
 and info

On Tue, 26 Apr 2022 08:57:15 +0200 Jiri Pirko wrote:
> >> 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..  
> 
> *
> Sure, that is why I re-used components :)

Well, right, I guess you did reuse them a little :)

> But you have to somehow link the component to the particular gearbox on
> particular line card. Say, you need to flash GB on line card 8. This is
> basically providing a way to expose this relationship to user.
> 
> Also, the "lc info" shows the FW version for gearboxes. As Ido
> mentioned, the GB versions could be listed in "devlink dev info" in
> theory. But then, you need to somehow expose the relationship with
> line card as well.

Why would the automation which comes to update the firmware care 
at all where the component is? Humans can see what the component 
is by looking at the name.

If we do need to know (*if*!) you can list FW components as a lc
attribute, no need for new commands and objects.

IMHO we should either keep lc objects simple and self contained or 
give them a devlink instance. Creating sub-objects from them is very
worrying. If there is _any_ chance we'll need per-lc health reporters 
or sbs or params(🤢) etc. etc. - let's bite the bullet _now_ and create
full devlink sub-instances!

> I don't see any simpler iface than this.

Based on the assumptions you've made, maybe, but the uAPI should
abstract away the irrelevant details. I'm questioning the assumptions.

> >> 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 4 gearboxes on the line card. They share the same flash. So
> if you flash gearbox 0, the rest will use the same FW.

o_0 so the FW component is called lcX_dev0 and yet it applies to _all_
devices, not just dev0?! Looking at the output above I thought other
devices simply don't have FW ("flashable false").

> I'm exposing them for the sake of completeness. Also, the interface
> needs to be designed as a list anyway, as different line cards may
> have separate flash per gearbox.
> 
> What's is the harm in exposing devices 1-3? If you insist, we can hide
> them.

Well, they are unnecessary (this is uAPI), and coming from the outside
I misinterpreted what the information reported means, so yeah, I'd
classify it as harmful :(

Powered by blists - more mailing lists