On Wed, Oct 27, 2021 at 06:43:00PM +0000, Sunil Kovvuri Goutham wrote:
quoted
________________________________
From: Ido Schimmel <redacted>
Sent: Wednesday, October 27, 2021 11:41 PM
To: Jakub Kicinski <kuba@kernel.org>
Cc: sundeep subbaraya <redacted>; David Miller <davem@davemloft.net>; netdev@vger.kernel.org <redacted>; Hariprasad Kelam <redacted>; Geethasowjanya Akula <gakula@marvell.com>; Sunil Kovvuri Goutham <sgoutham@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Rakesh Babu Saladi <redacted>; Saeed Mahameed <saeed@kernel.org>; anthony.l.nguyen@intel.com <anthony.l.nguyen@intel.com>; Jesse Brandeburg <redacted>; Andrew Lunn <andrew@lunn.ch>
Subject: Re: [EXT] Re: [net-next PATCH 1/2] octeontx2-pf: Add devlink param to init and de-init serdes
On Wed, Oct 27, 2021 at 10:08:57AM -0700, Jakub Kicinski wrote:
quoted
On Wed, 27 Oct 2021 22:13:32 +0530 sundeep subbaraya wrote:
quoted
quoted
On Wed, 27 Oct 2021 16:01:14 +0530 Subbaraya Sundeep wrote:
quoted
From: Rakesh Babu <redacted>
The physical/SerDes link of an netdev interface is not
toggled on interface bring up and bring down. This is
because the same link is shared between PFs and its VFs.
This patch adds devlink param to toggle physical link so
that it is useful in cases where a physical link needs to
be re-initialized.
So it's a reset? Or are there cases where user wants the link
to stay down?
There are cases where the user wants the link to stay down and debug.
We are adding this to help customers to debug issues wrt physical links.
Intel has a similar thing, they keep adding a ethtool priv flag called
"link-down-on-close" to all their drivers.
Maybe others do this, too. It's time we added a standard API for
this.
The new parameter sounds like a reset, but it can also be achieved by:
# ethtool --set-priv-flags eth0 link-down-on-close on
# ip link set dev eth0 down
# ip link set dev eth0 up
Where the first command is replaced by a more standard ethtool API.
The intention here is provide an option to the user to toggle the serdes configuration
as and when he wants to.
But why? What is the motivation? The commit message basically says that
you are adding a param to toggle the physical link because it is useful
to toggle the physical link.
There is no dependency with logical interface's status.
But there is and the commit message explains why you are not doing it as
part of ndo_{stop,open}(): "because the same link is shared between PFs
and its VFs"
Such constraints also apply to other drivers and you can see that in the
"link-down-on-close" private flag. I'm also aware of propriety tools to
toggle device bits which prevent the physical link from going down upon
ndo_stop().
Having a standard API to select bringing down physical interface upon logical interface's close call
is a good idea. But this patch is not for that.
IIUC, your default behavior is not to take the physical link down upon
ndo_stop() and now you want to toggle the link. If you have a standard
API to change the default behavior, then the commands I showed will
toggle the link, no?
Hi Ido,
On Thu, Oct 28, 2021 at 3:55 AM Ido Schimmel [off-list ref] wrote:
On Wed, Oct 27, 2021 at 06:43:00PM +0000, Sunil Kovvuri Goutham wrote:
quoted
quoted
________________________________
From: Ido Schimmel <redacted>
Sent: Wednesday, October 27, 2021 11:41 PM
To: Jakub Kicinski <kuba@kernel.org>
Cc: sundeep subbaraya <redacted>; David Miller <davem@davemloft.net>; netdev@vger.kernel.org <redacted>; Hariprasad Kelam <redacted>; Geethasowjanya Akula <gakula@marvell.com>; Sunil Kovvuri Goutham <sgoutham@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Rakesh Babu Saladi <redacted>; Saeed Mahameed <saeed@kernel.org>; anthony.l.nguyen@intel.com <anthony.l.nguyen@intel.com>; Jesse Brandeburg <redacted>; Andrew Lunn <andrew@lunn.ch>
Subject: Re: [EXT] Re: [net-next PATCH 1/2] octeontx2-pf: Add devlink param to init and de-init serdes
On Wed, Oct 27, 2021 at 10:08:57AM -0700, Jakub Kicinski wrote:
quoted
On Wed, 27 Oct 2021 22:13:32 +0530 sundeep subbaraya wrote:
quoted
quoted
On Wed, 27 Oct 2021 16:01:14 +0530 Subbaraya Sundeep wrote:
quoted
From: Rakesh Babu <redacted>
The physical/SerDes link of an netdev interface is not
toggled on interface bring up and bring down. This is
because the same link is shared between PFs and its VFs.
This patch adds devlink param to toggle physical link so
that it is useful in cases where a physical link needs to
be re-initialized.
So it's a reset? Or are there cases where user wants the link
to stay down?
There are cases where the user wants the link to stay down and debug.
We are adding this to help customers to debug issues wrt physical links.
Intel has a similar thing, they keep adding a ethtool priv flag called
"link-down-on-close" to all their drivers.
Maybe others do this, too. It's time we added a standard API for
this.
The new parameter sounds like a reset, but it can also be achieved by:
# ethtool --set-priv-flags eth0 link-down-on-close on
# ip link set dev eth0 down
# ip link set dev eth0 up
Where the first command is replaced by a more standard ethtool API.
The intention here is provide an option to the user to toggle the serdes configuration
as and when he wants to.
But why? What is the motivation? The commit message basically says that
you are adding a param to toggle the physical link because it is useful
to toggle the physical link.
quoted
There is no dependency with logical interface's status.
But there is and the commit message explains why you are not doing it as
part of ndo_{stop,open}(): "because the same link is shared between PFs
and its VFs"
Such constraints also apply to other drivers and you can see that in the
"link-down-on-close" private flag. I'm also aware of propriety tools to
toggle device bits which prevent the physical link from going down upon
ndo_stop().
quoted
Having a standard API to select bringing down physical interface upon logical interface's close call
is a good idea. But this patch is not for that.
IIUC, your default behavior is not to take the physical link down upon
ndo_stop() and now you want to toggle the link. If you have a standard
API to change the default behavior, then the commands I showed will
toggle the link, no?
Actually we also need a case where debugging is required when the
logical link is
up (so that packets flow from kernel to SerDes continuously) but the
physical link
is down. We will change the commit description since it is giving the
wrong impression.
A command to change physical link up/down with no relation to ifconfig
is needed.
Thanks,
Sundeep
On Thu, Oct 28, 2021 at 05:48:02PM +0530, sundeep subbaraya wrote:
Actually we also need a case where debugging is required when the
logical link is
up (so that packets flow from kernel to SerDes continuously) but the
physical link
is down.
Can you explain the motivation for that? In the past we discussed use
cases for forcing the operational state to down while the administrative
state is up and couldn't find any.
We will change the commit description since it is giving the
wrong impression.
A command to change physical link up/down with no relation to ifconfig
is needed.
So it is obvious that some drivers default to not shutting down the
physical link upon admin down, but that some users want to change that.
In addition, we have your use case to control the physical link without
relation to the logical link. I wonder if it can all be solved with a
new ethtool attribute (part of LINKINFO_{SET,GET} ?) that describes the
physical link policy and has the following values:
* auto: Physical link state is derived from logical link state
* up: Physical link state is always up
* down: Physical link state is always down
IIUC, it should solve your problem and that of the "link-down-on-close"
private flag. It also has the added benefit of allowing user space to
query the default policy. The expectation is that it would be "auto",
but in some scenarios it is "up".
Hi Ido,
On Thu, Oct 28, 2021 at 7:21 PM Ido Schimmel [off-list ref] wrote:
On Thu, Oct 28, 2021 at 05:48:02PM +0530, sundeep subbaraya wrote:
quoted
Actually we also need a case where debugging is required when the
logical link is
up (so that packets flow from kernel to SerDes continuously) but the
physical link
is down.
Can you explain the motivation for that? In the past we discussed use
cases for forcing the operational state to down while the administrative
state is up and couldn't find any.
To be honest we got this request from a customer to provide a command to modify
physical link without tying it to a logical link. I have asked for
more details on how
they use it.
quoted
We will change the commit description since it is giving the
wrong impression.
A command to change physical link up/down with no relation to ifconfig
is needed.
So it is obvious that some drivers default to not shutting down the
physical link upon admin down, but that some users want to change that.
In addition, we have your use case to control the physical link without
relation to the logical link. I wonder if it can all be solved with a
new ethtool attribute (part of LINKINFO_{SET,GET} ?) that describes the
physical link policy and has the following values:
* auto: Physical link state is derived from logical link state
* up: Physical link state is always up
* down: Physical link state is always down
IIUC, it should solve your problem and that of the "link-down-on-close"
private flag. It also has the added benefit of allowing user space to
query the default policy. The expectation is that it would be "auto",
but in some scenarios it is "up".
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
Thanks,
Sundeep
On Sat, Oct 30, 2021 at 12:55:47PM +0530, sundeep subbaraya wrote:
Hi Ido,
On Thu, Oct 28, 2021 at 7:21 PM Ido Schimmel [off-list ref] wrote:
quoted
On Thu, Oct 28, 2021 at 05:48:02PM +0530, sundeep subbaraya wrote:
quoted
Actually we also need a case where debugging is required when the
logical link is
up (so that packets flow from kernel to SerDes continuously) but the
physical link
is down.
Can you explain the motivation for that? In the past we discussed use
cases for forcing the operational state to down while the administrative
state is up and couldn't find any.
To be honest we got this request from a customer to provide a command to modify
physical link without tying it to a logical link. I have asked for
more details on how
they use it.
Thanks
quoted
quoted
We will change the commit description since it is giving the
wrong impression.
A command to change physical link up/down with no relation to ifconfig
is needed.
So it is obvious that some drivers default to not shutting down the
physical link upon admin down, but that some users want to change that.
In addition, we have your use case to control the physical link without
relation to the logical link. I wonder if it can all be solved with a
new ethtool attribute (part of LINKINFO_{SET,GET} ?) that describes the
physical link policy and has the following values:
* auto: Physical link state is derived from logical link state
* up: Physical link state is always up
* down: Physical link state is always down
IIUC, it should solve your problem and that of the "link-down-on-close"
private flag. It also has the added benefit of allowing user space to
query the default policy. The expectation is that it would be "auto",
but in some scenarios it is "up".
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. 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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-11-08 15:54:53
On Sun, 7 Nov 2021 11:21:17 +0200 Ido Schimmel wrote:
quoted
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
- 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.
What are your thoughts on the SFP policy? We can still reshuffle it.
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:
quoted
quoted
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?
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
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").
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-11-11 16:47:23
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.
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:
quoted
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.
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".
quoted
quoted
- NC-SI / BMC
- SR-IOV (legacy)
- NPAR / Mutli-Host
so 4 known reasons.
quoted
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.
Agree
quoted
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.
On Thu, Nov 11, 2021 at 08:47:19AM -0800, Jakub Kicinski wrote:
quoted
On Thu, 11 Nov 2021 16:51:51 +0200 Ido Schimmel wrote:
quoted
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.
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.
quoted
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".
quoted
quoted
quoted
- NC-SI / BMC
- SR-IOV (legacy)
- NPAR / Mutli-Host
so 4 known reasons.
quoted
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.
Agree
quoted
quoted
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
quoted
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-11-15 15:12:29
On Sun, 14 Nov 2021 20:19:59 -0800 Roopa Prabhu wrote:
On 11/14/21 12:38 AM, Ido Schimmel wrote:
quoted
On Thu, Nov 11, 2021 at 08:47:19AM -0800, Jakub Kicinski wrote:
quoted
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.
Ah, GTK!
quoted
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 see, I think you're talking about something like IFLA_VF_LINK_STATE_*
but for the PF. That could make sense, although I don't think it was
ever requested.
quoted
quoted
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".
Sounds sensible.
quoted
quoted
"proto" in "protodown" refers to STP, right?
Not really. I believe the main use case was vrrp / mlag.
VRRP is a proto, mlag maybe a little less clear-cut.
quoted
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
What link properties?
Link Flapping detection and disable link
Port Security Violation
Port security as established by a .. protocol like 802.1X ?
Broadcast Storms
etc
Why not take the entire interface down for bcast storm?
quoted
quoted
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
You are the experts so correct me please but the point of protodown
is that the the link is held down for general traffic but you can
still exchange protocol messages on it. STP, VRRP, LAG, 802.1X etc.
For anything that does not require special message exchange the link
can be just brought down completely.
In my head link held up is a completely different beast, the local host
does not participate or otherwise pay attention to any communication on
the link. It's about what other entities do with the link.
But if you prefer "protoup" strongly that's fine, I guess.
Hi,
On Mon, Nov 15, 2021 at 8:41 PM Jakub Kicinski [off-list ref] wrote:
On Sun, 14 Nov 2021 20:19:59 -0800 Roopa Prabhu wrote:
quoted
On 11/14/21 12:38 AM, Ido Schimmel wrote:
quoted
On Thu, Nov 11, 2021 at 08:47:19AM -0800, Jakub Kicinski wrote:
quoted
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.
Ah, GTK!
quoted
quoted
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 see, I think you're talking about something like IFLA_VF_LINK_STATE_*
but for the PF. That could make sense, although I don't think it was
ever requested.
quoted
quoted
quoted
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".
Sounds sensible.
quoted
quoted
quoted
"proto" in "protodown" refers to STP, right?
Not really. I believe the main use case was vrrp / mlag.
VRRP is a proto, mlag maybe a little less clear-cut.
quoted
quoted
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
What link properties?
quoted
Link Flapping detection and disable link
Port Security Violation
Port security as established by a .. protocol like 802.1X ?
quoted
Broadcast Storms
etc
Why not take the entire interface down for bcast storm?
quoted
quoted
quoted
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
You are the experts so correct me please but the point of protodown
is that the the link is held down for general traffic but you can
still exchange protocol messages on it. STP, VRRP, LAG, 802.1X etc.
For anything that does not require special message exchange the link
can be just brought down completely.
In my head link held up is a completely different beast, the local host
does not participate or otherwise pay attention to any communication on
the link. It's about what other entities do with the link.
But if you prefer "protoup" strongly that's fine, I guess.
As said by Ido, ndo_change_proto_down with proto_down as
on and off is sufficient for our requirement right now. We will use
ndo_change_proto_down
instead of devlink. Thanks everyone for pitching in.
Sundeep
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-11-19 14:10:01
On Fri, 19 Nov 2021 16:17:53 +0530 sundeep subbaraya wrote:
As said by Ido, ndo_change_proto_down with proto_down as
on and off is sufficient for our requirement right now. We will use
ndo_change_proto_down instead of devlink. Thanks everyone for
pitching in.
ndo_change_proto_down is for software devices. Make sure you explain
your use case well, otherwise it's going to be a nack.
On Fri, Nov 19, 2021 at 7:40 PM Jakub Kicinski [off-list ref] wrote:
On Fri, 19 Nov 2021 16:17:53 +0530 sundeep subbaraya wrote:
quoted
As said by Ido, ndo_change_proto_down with proto_down as
on and off is sufficient for our requirement right now. We will use
ndo_change_proto_down instead of devlink. Thanks everyone for
pitching in.
ndo_change_proto_down is for software devices. Make sure you explain
your use case well, otherwise it's going to be a nack.
Sorry new to networking stuff here. Where does the below imply it is
for software devices?
* 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.
I will find out the use case (pinged customer again)
Thanks,
Sundeep
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-11-19 14:49:23
On Fri, 19 Nov 2021 19:56:51 +0530 sundeep subbaraya wrote:
On Fri, Nov 19, 2021 at 7:40 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Fri, 19 Nov 2021 16:17:53 +0530 sundeep subbaraya wrote:
quoted
As said by Ido, ndo_change_proto_down with proto_down as
on and off is sufficient for our requirement right now. We will use
ndo_change_proto_down instead of devlink. Thanks everyone for
pitching in.
ndo_change_proto_down is for software devices. Make sure you explain
your use case well, otherwise it's going to be a nack.
Sorry new to networking stuff here. Where does the below imply it is
for software devices?
* 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.
I will find out the use case (pinged customer again)
Don't trust comments or documentation when working on Linux.
Code and git history are the sources of truth.
But you're right in a sense, the software devices which use this
callback today look like pretty fake users to allow out-of-tree
code to do things.
Will anyone who does not work on Cumulus Linus scream if we do this?
----->8---------------
From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 19 Nov 2021 06:43:58 -0800
Subject: [PATCH] net: remove .ndo_change_proto_down
.ndo_change_proto_down was added seemingly to enable out-of-tree
implementations. Over 2.5yrs later we still have no real users
upstream. Stub this out for now, we can revert once real users
materialize. (rocker is a test vehicle, not a user.)
We need to drop the optimization on the sysfs side, because
unlike ndos priv_flags will be changed at runtime, so we'd
need READ_ONCE/WRITE_ONCE everywhere..
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/rocker/rocker_main.c | 12 -----------
drivers/net/macvlan.c | 3 +--
drivers/net/vxlan.c | 3 +--
include/linux/netdevice.h | 11 ++--------
net/core/dev.c | 26 ++++-------------------
net/core/net-sysfs.c | 6 ------
net/core/rtnetlink.c | 3 +--
7 files changed, 9 insertions(+), 55 deletions(-)
@@ -490,12 +490,6 @@ static ssize_t proto_down_store(struct device *dev,{structnet_device*netdev=to_net_dev(dev);-/* The check is also done in change_proto_down; this helps returning-*earlywithouthittingthetrylock/restartinnetdev_store.-*/-if(!netdev->netdev_ops->ndo_change_proto_down)-return-EOPNOTSUPP;-returnnetdev_store(dev,attr,buf,len,change_proto_down);}NETDEVICE_SHOW_RW(proto_down,fmt_dec);
@@ -2539,13 +2539,12 @@ static int do_set_proto_down(struct net_device *dev,structnetlink_ext_ack*extack){structnlattr*pdreason[IFLA_PROTO_DOWN_REASON_MAX+1];-conststructnet_device_ops*ops=dev->netdev_ops;unsignedlongmask=0;u32value;boolproto_down;interr;-if(!ops->ndo_change_proto_down){+if(!(dev->priv_flags&IFF_CHANGE_PROTO_DOWN)){NL_SET_ERR_MSG(extack,"Protodown not supported by device");return-EOPNOTSUPP;}