Message-ID: <20220426075133.53562a2e@kernel.org>
Date:   Tue, 26 Apr 2022 07:51:33 -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 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.
You're not designing an interface for SDK users but for end users.

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

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

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.

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.

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

> 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

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

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