Re: [EXT] Re: [net-next PATCH 1/2] octeontx2-pf: Add devlink param to init and de-init serdes

[Ido Schimmel <idosch@idosch.org>]

On Mon, Nov 08, 2021 at 07:54:50AM -0800, Jakub Kicinski wrote:
> On Sun, 7 Nov 2021 11:21:17 +0200 Ido Schimmel wrote:
> > > This looks good. Please note that we need the behavior such that after changing
> > > the flag a subsequent ifconfig command is not needed by the user.
> > > 
> > > auto : in ndo_open, ndo_close check the physical link flag is auto and
> > > send command
> > >           to firmware for bringing physical link up/down.
> > > up: send command to firmware instantaneously for physical link UP
> > > down: send command to firmware instantaneously for physical link DOWN  
> > 
> > TBH, I'm not that happy with my ethtool suggestion. It is not very clear
> > which hardware entities the attribute controls.
> 
> Last week I heard a request to also be able to model NC-SI disruption.
> Control if the NIC should be reset and newly flashed FW activated when
> host is rebooted (vs full server power cycle).
> 
> That adds another dimension to the problem, even though that particular
> use case may be better answered thru the devlink flashing/reset APIs.
> 
> Trying to organize the requirements we have 3 entities which may hold
> the link up:
>  - SFP power policy

The SFP power policy does not keep the link up. In fact, we specifically
removed the "low" policy to make sure that whatever policy you configure
("auto"/"high") does not affect your carrier.

>  - NC-SI / BMC
>  - SR-IOV (legacy)
> 
> I'd think auto/up as possible options still make sense, although in
> case of NC-SI many NICs may not allow overriding the "up". And the
> policy may change without notification if BMC selects / activates 
> a port - it may go from auto to up with no notification.
> 
> Presumably we want to track "who's holding the link up" per consumer.
> Just a bitset with 1s for every consumer holding "up"? 
> 
> Or do we expect there will be "more to it" and should create bespoke
> nests?
> 
> > Maybe it's better to
> > implement it as a rtnetlink attribute that controls the carrier (e.g.,
> > "carrier_policy")? Note that we already have ndo_change_carrier(), but
> > the kdoc comment explicitly mentions that it shouldn't be used by
> > physical devices:
> >
> >  * int (*ndo_change_carrier)(struct net_device *dev, bool new_carrier);
> >  *	Called to change device carrier. Soft-devices (like dummy, team, etc)
> >  *	which do not represent real hardware may define this to allow their
> >  *	userspace components to manage their virtual carrier state. Devices
> >  *	that determine carrier state from physical hardware properties (eg
> >  *	network cables) or protocol-dependent mechanisms (eg
> >  *	USB_CDC_NOTIFY_NETWORK_CONNECTION) should NOT implement this function.
> 
> New NDO seems reasonable. 

Spent a bit more time on that and I'm not sure a new ndo is needed. See:

 * void (*ndo_change_proto_down)(struct net_device *dev,
 *				 bool proto_down);
 *	This function is used to pass protocol port error state information
 *	to the switch driver. The switch driver can react to the proto_down
 *      by doing a phys down on the associated switch port.

So what this patch is trying to achieve can be achieved by implementing
support for this ndo:

$ ip link show dev macvlan10
20: macvlan10@dummy10: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 3e:d6:1a:97:ba:5e brd ff:ff:ff:ff:ff:ff

# ip link set dev macvlan10 protodown on

$ ip link show dev macvlan10
20: macvlan10@dummy10: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
    link/ether 3e:d6:1a:97:ba:5e brd ff:ff:ff:ff:ff:ff protodown on

Currently, user space has no visibility into the fact that by default
the carrier is on, but I imagine this can be resolved by adding
"protoup" and defaulting the driver to report "on". The "who's holding
the link up" issue can be resolved via "protoup_reason" (same as
"protodown_reason").