Message-ID: <Ymf66h5dMNOLun8k@nanopsycho>
Date:   Tue, 26 Apr 2022 16:00:10 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
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

Tue, Apr 26, 2022 at 02:41:30PM CEST, kuba@...nel.org wrote:
>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 :)

I use them a lot. It is not visible in this patchset, but in the
flashing follow-up patchset.


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

The relationship-by-name sounds a bit fragile to me. The names of
components are up to the individual drivers.


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

There is no new command for that, only one nested attribute which
carries the device list added to the existing command. They are no new
objects, they are just few nested values.


>
>IMHO we should either keep lc objects simple and self contained or 
>give them a devlink instance. Creating sub-objects from them is very

Give them a devlink instance? I don't understand how. LC is not a
separate device, far from that. That does not make any sense to me.


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

Does not make sense to me at all. Line cards are detachable PHY sets in
essence, very basic functionality. They does not have buffers, health
and params, I don't think so. 


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

Is the FW version of gearbox on a line card irrelevand detail?
If so, how does the user know if/when to flash it?
If not, where would you list it if devices nest is not the correct place?


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

Yes, device 0 is "flash master" (RW). 1-3 are RO. I know it is a bit
confusing. Maybe Andy's suggestion of "shared" flag of some sort might
help.


>
>> 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 :(

UAPI is the "devices nest". It has to be list one way or another
(we may need to expose more gearboxes anyway). So what is differently
harmful with having list [0] or list [0,1,2,3] ?

Powered by blists - more mailing lists