Message-ID: <20210810120030.5092ec22@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 10 Aug 2021 12:00:30 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        davem@...emloft.net, mkubecek@...e.cz, pali@...nel.org,
        vadimp@...dia.com, mlxsw@...dia.com,
        Ido Schimmel <idosch@...dia.com>
Subject: Re: [RFC PATCH net-next 2/8] ethtool: Add ability to reset
 transceiver modules

On Tue, 10 Aug 2021 21:15:45 +0300 Ido Schimmel wrote:
> On Tue, Aug 10, 2021 at 06:54:23AM -0700, Jakub Kicinski wrote:
> > On Tue, 10 Aug 2021 16:05:07 +0300 Ido Schimmel wrote:  
> > > Takes about 1.5ms to get an ACK on the reset request and another few
> > > seconds to ensure module is in a valid operational state (will remove
> > > RTNL in next version).  
> > 
> > Hm. RTNL-lock-less ethtool ops are a little concerning. The devlink
> > locking was much complicated by the unclear locking rules. Can we keep
> > ethtool simple and put this functionality in a different API or make
> > the reset async?  
> 
> I thought there are already RTNL-lock-less ethtool ops, but maybe I
> imagined it. Given that we also want to support firmware update on
> modules and that user space might want to update several modules
> simultaneously, do you have a suggestion on how to handle it from
> locking perspective?

Hm, flashing is harder than reset. We can't unbind the driver while
it's in progress. I don't have any ready solution in mind, but I'd 
like to make sure the locking is clear and hard to get wrong. Maybe 
we could have a mix of ops, one called for "preparing" the flashing
called under rtnl and another for "commit" with "unlocked" in the name.
Drivers which don't want to deal with dropping rtnl lock can just do
everything in the first stage? Perhaps Andrew has better ideas, I'm
just spit-balling. Presumably there are already locks at play, locks
we would have to take in the case where Linux manages the PHY. Maybe
they dictate an architecture?

> The ethtool netlink backend has parallel ops, but
> RTNL is a problem. Firmware flashing is currently synchronous in both
> ethtool and devlink, but the latter does not hold RTNL.

Yeah, drivers dropping rtnl_lock mid way thru the ethtool flashing op
was one of my main motivations for moving it into devlink.

> > > We can have multiple ports (split) using the same module and in CMIS
> > > each data path is controlled by a different state machine. Given the
> > > complexity of these modules and possible faults, it is possible to
> > > imagine a situation in which a few ports are fine and the rest are
> > > unable to obtain a carrier.
> > > 
> > > Resetting the module on ifup of swp1s0 is not intuitive and it shouldn't
> > > affect other split ports (e.g., swp1s1). With the dedicated reset
> > > command we have the ability to enforce all the required restrictions
> > > from the start instead of changing the behavior of existing commands.  
> > 
> > Sounds similar to what ethtool --reset was trying to address (upper
> > 16 bits). Could we reuse that? Whether its a SFP or other part of the
> > port being reset may not be entirely important to the user, so perhaps
> > it's not a bad idea to abstract that away and stick to "reset levels"?  
> 
> Wasn't aware of this API. Looks like it is only implemented by a few
> drivers, but man page says "phy    Transceiver/PHY", so I think we can
> reuse it.
> 
> What do you mean by "reset levels"? The split between shared /
> dedicated?

Indeed.

> Just to make sure I understand, you suggest the following semantics?
> 
> # ethtool --reset swp1s0 phy
> Error since module is used by several ports (split)
> 
> # ethtool --reset swp1s0 phy-shared
> OK
> 
> # ethtool --reset swp1 phy
> OK (no split)
> 
> # ethtool --reset swp1 phy-shared
> OK

Exactly.

Powered by blists - more mailing lists