Message-ID: <952e8bb0-bc1e-5600-92f2-de4d6744fcb0@nvidia.com>
Date:   Sun, 14 Nov 2021 20:19:59 -0800
From:   Roopa Prabhu <roopa@...dia.com>
To:     Ido Schimmel <idosch@...sch.org>, Jakub Kicinski <kuba@...nel.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 11/14/21 12:38 AM, Ido Schimmel wrote:
> On Thu, Nov 11, 2021 at 08:47:19AM -0800, Jakub Kicinski wrote:
>> 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.
> The SFP power policy doesn't affect that. In our systems (and I believe
> many others), by default, the transceivers are transitioned to high
> power mode upon plug-in, but the link is still down when the netdev is
> down because the MAC/PHY are not operational.
>
> With SRIOV/Multi-Host, the MAC/PHY are always operational which is why
> your link partner has a carrier even when the netdev is down.
>
>> I don't think we should report carrier up when netdev is down?
> This is what happens today, but it's misleading because the carrier is
> always up with these systems. When I take the netdev down, I expect my
> link partner to lose carrier. If this doesn't happen, then I believe the
> netdev should always report IFF_UP. Alternatively, to avoid user space
> breakage, this can be reported via a new attribute such as "protoup".
>
>>>>   - 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.
> Agree
>
>>> 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 really. I believe the main use case was vrrp / mlag. The
> "protdown_reason" is just a bitmap of user enumerated reasons to keep
> the interface down. See commit 829eb208e80d ("rtnetlink: add support for
> protodown reason") for details.

correct. Its equivalent to errDisable found on most commercial switch OS'es.

Can be used for any control-plane/mgmt-plane/protocol wanting to hold 
the link down.

Other use-cases where this can be used (as also quoted by other vendors):

mismatch of link properties
Link Flapping detection and disable link
Port Security Violation
Broadcast Storms
etc


>
>> Not sure what "proto" in "protoup" would be.
> sriov/multi-host/etc ?

agree. Would be nice to re-use protodown ndo and state/reason here

Powered by blists - more mailing lists