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

On Thu, 11 Nov 2021 16:51:51 +0200 Ido Schimmel wrote:

On Mon, Nov 08, 2021 at 07:54:50AM -0800, Jakub Kicinski wrote:

quoted

On Sun, 7 Nov 2021 11:21:17 +0200 Ido Schimmel wrote:  

quoted

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.

Hm. How do we come up with the appropriate wording here...

I meant keeping the "PHY level link" up? I think we agree that all the
cases should behave like SFP power behaves today?

The API is to control or query what is forcing the PHY link to stay up
after the netdev was set down. IOW why does the switch still see link
up if the link is down on Linux. I don't think we should report carrier
up when netdev is down?

quoted

 - NC-SI / BMC
 - SR-IOV (legacy)

 - NPAR / Mutli-Host

so 4 known reasons.

quoted

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?
  

quoted

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

Let's wait to hear a strong use case, tho.

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

"proto" in "protodown" refers to STP, right? Not sure what "proto" in
"protoup" would be.