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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Nov 2021 08:47:19 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     sundeep subbaraya <sundeep.lkml@...il.com>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Hariprasad Kelam <hkelam@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
        Rakesh Babu Saladi <rsaladi2@...vell.com>,
        Saeed Mahameed <saeed@...nel.org>,
        "anthony.l.nguyen@...el.com" <anthony.l.nguyen@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Andrew Lunn <andrew@...n.ch>, argeorge@...co.com
Subject: 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:
> > On Sun, 7 Nov 2021 11:21:17 +0200 Ido Schimmel wrote:  
> > > 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?

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

 - NPAR / Mutli-Host

so 4 known reasons.

> > 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@...my10: <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@...my10: <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.