Message-ID: <YmjyRgYYRU/ZaF9X@nanopsycho>
Date:   Wed, 27 Apr 2022 09:35:34 +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 04:51:33PM CEST, kuba@...nel.org wrote:
>On Tue, 26 Apr 2022 16:00:10 +0200 Jiri Pirko wrote:
>> >> 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.
>
>I asked you how the automation will operate. You must answer questions
>if you want to have a discussion. Automation is the relevant part.

Automation, not sure. It would probably just see type of gearbox and
flash it. Not sure I understand the question, perhaps you could explain?
Plus, the possibility is to auto-flash the GB from driver directly.


>You're not designing an interface for SDK users but for end users.

Sure, that is the aim of this API. Human end user. That is why I wanted
the user to see the relationships between devlink dev, line cards and
the gearboxes on them. If you want to limit the visibility, sure, just
tell me how.


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

Okay, that is not only to expose devices. That is also to expose info
about linecards, like HW revision, INI version etc. Where else to put
it? I can perhaps embed it into devlink dev info, but I thought separate
command would be more suitable. object cmd, object info cmd. It is
more clear I believe.


>
>> >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.
>
>You can put a name of another devlink instance as an attribute of a lc.
>See below.
>
>> >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 guess the definition of a "line card" has become somewhat murky over
>the years, since the olden days of serial lines.
>
>Perhaps David and others can enlighten us but what I'm used to hearing
>about as a line card these days in a chassis system is a full-on switch.
>Chassis being effectively a Clos network in a box, the main difference
>being the line cards talk cells to the backplane, not full packets.

That is a different device model comparing what we have.
These are like "nested switches". If they are separate devices with
everything, they can be modeled as separate devlink device instances.
No problem. Different case though.


>
>Back in my Netronome days we called those detachable front panel gear
>boxes "phy mods". Those had nowhere near the complexity of a real line
>card. Sounds like that's more aligned with what you have.

Yep.


>
>To summarize, since your definition of a line card is a bit special,
>the less uAPI we add to fit your definition we add the better.

Tell me where to cut it so it still makes sense.


>
>> >> 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?
>
>Not what I said.

That is why I'm asking :)


>
>> 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?
>
>Let me mock up what I had in mind for you since it did not come thru 
>in the explanation:
>
>$ devlink dev info show pci/0000:01:00.0
>    versions:
>        fixed:
>          hw.revision 0
>          lc2.hw.revision a
>          lc8.hw.revision b
>        running:
>          ini.version 4
>          lc2.gearbox 1.1.3
>          lc8.gearbox 1.2.3

Would be rather:

          lc2.gearbox0 1.1.3
          lc2.gearbox1 1.2.4
          lc8.gearbox0 1.2.3

Okay, I see. So instead of having clear api with relationships and
clear human+machine readability we have squahed indexes into strings.
I fail to see the benefit, other than no-api-extension :/ On contrary.


>
>$ devlink lc show pci/0000:01:00.0 lc 8
>pci/0000:01:00.0:
>  lc 8 state active type 16x100G
>    supported_types:
>      16x100G
>    versions: 
>      lc8.hw.revision (a) 
>      lc8.gearbox (1.2.3)
>
>Where the data in the brackets is optionally fetched thru the existing
>"dev info" API, but rendered together by the user space.

Quite odd. I find it questionable to say at least to mix multiple
command netlink outputs into one output. The processing of it would
be a small nightmare considering the way how the netlink message
processing works in iproute2 :/


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