* [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel
@ 2020-01-10 22:02 Johnathan Mantey
2020-01-11 23:31 ` David Miller
2020-01-14 0:43 ` samjonas
0 siblings, 2 replies; 6+ messages in thread
From: Johnathan Mantey @ 2020-01-10 22:02 UTC (permalink / raw)
To: netdev; +Cc: sam, davem
From 76d99782ec897b010ba507895d60d27dca8dca44 Mon Sep 17 00:00:00 2001
From: Johnathan Mantey <johnathanx.mantey@intel.com>
Date: Fri, 10 Jan 2020 12:46:17 -0800
Subject: [PATCH] Propagate NCSI channel carrier loss/gain events to the
kernel
Problem statement:
Insertion or removal of a network cable attached to a NCSI controlled
network channel does not notify the kernel of the loss/gain of the
network link.
The expectation is that /sys/class/net/eth(x)/carrier will change
state after a pull/insertion event. In addition the carrier_up_count
and carrier_down_count files should increment.
Change statement:
Use the NCSI Asynchronous Event Notification handler to detect a
change in a NCSI link.
Add code to propagate carrier on/off state to the network interface.
The on/off state is only modified after the existing code identifies
if the network device HAD or HAS a link state change.
Test procedure:
Connected a L2 switch with only two ports connected.
One port was a DHCP corporate net, the other port attached to the NCSI
controlled NIC.
Starting with the L2 switch with DC on, check to make sure the NCSI
link is operating.
cat /sys/class/net/eth1/carrier
1
cat /sys/class/net/eth1/carrier_up_count
0
cat /sys/class/net/eth1/carrier_down_count
0
Remove DC from the L2 switch, and check link state
cat /sys/class/net/eth1/carrier
0
cat /sys/class/net/eth1/carrier_up_count
0
cat /sys/class/net/eth1/carrier_down_count
1
Restore DC to the L2 switch, and check link state
cat /sys/class/net/eth1/carrier
1
cat /sys/class/net/eth1/carrier_up_count
1
cat /sys/class/net/eth1/carrier_down_count
1
Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
---
net/ncsi/ncsi-aen.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
index b635c194f0a8..274c415dcead 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -89,6 +89,12 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv
*ndp,
if ((had_link == has_link) || chained)
return 0;
+ if (had_link) {
+ netif_carrier_off(ndp->ndev.dev);
+ } else {
+ netif_carrier_on(ndp->ndev.dev);
+ }
+
if (!ndp->multi_package && !nc->package->multi_channel) {
if (had_link) {
ndp->flags |= NCSI_DEV_RESHUFFLE;
--
2.24.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel
2020-01-10 22:02 [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel Johnathan Mantey
@ 2020-01-11 23:31 ` David Miller
2020-01-14 0:43 ` samjonas
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2020-01-11 23:31 UTC (permalink / raw)
To: johnathanx.mantey; +Cc: netdev, sam
From: Johnathan Mantey <johnathanx.mantey@intel.com>
Date: Fri, 10 Jan 2020 14:02:23 -0800
Please format your Subject line as:
Subject: $SUBSYSTEM_PREFIX: Summary.
Here, $SUBSYSTEM_PREFIX would be "ncsi: "
> Problem statement:
> Insertion or removal of a network cable attached to a NCSI controlled
> network channel does not notify the kernel of the loss/gain of the
> network link.
>
> The expectation is that /sys/class/net/eth(x)/carrier will change
> state after a pull/insertion event. In addition the carrier_up_count
> and carrier_down_count files should increment.
>
> Change statement:
> Use the NCSI Asynchronous Event Notification handler to detect a
> change in a NCSI link.
> Add code to propagate carrier on/off state to the network interface.
> The on/off state is only modified after the existing code identifies
> if the network device HAD or HAS a link state change.
Please remove this "Problem statement:" and "Change statement:", we know
what you are talking about.
> @@ -89,6 +89,12 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv
> *ndp,
> if ((had_link == has_link) || chained)
> return 0;
>
> + if (had_link) {
> + netif_carrier_off(ndp->ndev.dev);
> + } else {
> + netif_carrier_on(ndp->ndev.dev);
> + }
As per coding style, single line basic blocks should not get curly
braces around them in this situation.
Thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel
2020-01-10 22:02 [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel Johnathan Mantey
2020-01-11 23:31 ` David Miller
@ 2020-01-14 0:43 ` samjonas
2020-01-14 16:47 ` Johnathan Mantey
1 sibling, 1 reply; 6+ messages in thread
From: samjonas @ 2020-01-14 0:43 UTC (permalink / raw)
To: Johnathan Mantey, netdev; +Cc: davem
On Fri, 2020-01-10 at 14:02 -0800, Johnathan Mantey wrote:
> From 76d99782ec897b010ba507895d60d27dca8dca44 Mon Sep 17 00:00:00
> 2001
> From: Johnathan Mantey <johnathanx.mantey@intel.com>
> Date: Fri, 10 Jan 2020 12:46:17 -0800
> Subject: [PATCH] Propagate NCSI channel carrier loss/gain events to
> the
> kernel
>
> Problem statement:
> Insertion or removal of a network cable attached to a NCSI controlled
> network channel does not notify the kernel of the loss/gain of the
> network link.
>
> The expectation is that /sys/class/net/eth(x)/carrier will change
> state after a pull/insertion event. In addition the carrier_up_count
> and carrier_down_count files should increment.
>
> Change statement:
> Use the NCSI Asynchronous Event Notification handler to detect a
> change in a NCSI link.
> Add code to propagate carrier on/off state to the network interface.
> The on/off state is only modified after the existing code identifies
> if the network device HAD or HAS a link state change.
If we set the carrier state off until we successfully configured a
channel could we avoid this limitation?
Cheers,
Sam
>
> Test procedure:
> Connected a L2 switch with only two ports connected.
> One port was a DHCP corporate net, the other port attached to the
> NCSI
> controlled NIC.
>
> Starting with the L2 switch with DC on, check to make sure the NCSI
> link is operating.
> cat /sys/class/net/eth1/carrier
> 1
> cat /sys/class/net/eth1/carrier_up_count
> 0
> cat /sys/class/net/eth1/carrier_down_count
> 0
>
> Remove DC from the L2 switch, and check link state
> cat /sys/class/net/eth1/carrier
> 0
> cat /sys/class/net/eth1/carrier_up_count
> 0
> cat /sys/class/net/eth1/carrier_down_count
> 1
>
> Restore DC to the L2 switch, and check link state
> cat /sys/class/net/eth1/carrier
> 1
> cat /sys/class/net/eth1/carrier_up_count
> 1
> cat /sys/class/net/eth1/carrier_down_count
> 1
>
> Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
> ---
> net/ncsi/ncsi-aen.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index b635c194f0a8..274c415dcead 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -89,6 +89,12 @@ static int ncsi_aen_handler_lsc(struct
> ncsi_dev_priv
> *ndp,
> if ((had_link == has_link) || chained)
> return 0;
>
> + if (had_link) {
> + netif_carrier_off(ndp->ndev.dev);
> + } else {
> + netif_carrier_on(ndp->ndev.dev);
> + }
> +
> if (!ndp->multi_package && !nc->package->multi_channel) {
> if (had_link) {
> ndp->flags |= NCSI_DEV_RESHUFFLE;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel
2020-01-14 0:43 ` samjonas
@ 2020-01-14 16:47 ` Johnathan Mantey
2020-01-15 1:24 ` samjonas
0 siblings, 1 reply; 6+ messages in thread
From: Johnathan Mantey @ 2020-01-14 16:47 UTC (permalink / raw)
To: samjonas, netdev; +Cc: davem
Sam,
This code is working on a channel that is completely configured. This
code is covering a situation where the RJ45 cable is intentionally or
mistakenly removed from a system. In the event that the network cable is
removed/damaged/slips, the kernel needs to change state to show that
network interface no longer has a link. For my employers use case the
loss of carrier is then added to a log of system state changes. Thus for
each internal channel there needs to be a way to report that the channel
has/does not have a link carrier.
On 1/13/20 4:43 PM, samjonas wrote:
> On Fri, 2020-01-10 at 14:02 -0800, Johnathan Mantey wrote:
>> From 76d99782ec897b010ba507895d60d27dca8dca44 Mon Sep 17 00:00:00
>> 2001
>> From: Johnathan Mantey <johnathanx.mantey@intel.com>
>> Date: Fri, 10 Jan 2020 12:46:17 -0800
>> Subject: [PATCH] Propagate NCSI channel carrier loss/gain events to
>> the
>> kernel
>>
>> Problem statement:
>> Insertion or removal of a network cable attached to a NCSI controlled
>> network channel does not notify the kernel of the loss/gain of the
>> network link.
>>
>> The expectation is that /sys/class/net/eth(x)/carrier will change
>> state after a pull/insertion event. In addition the carrier_up_count
>> and carrier_down_count files should increment.
>>
>> Change statement:
>> Use the NCSI Asynchronous Event Notification handler to detect a
>> change in a NCSI link.
>> Add code to propagate carrier on/off state to the network interface.
>> The on/off state is only modified after the existing code identifies
>> if the network device HAD or HAS a link state change.
>
> If we set the carrier state off until we successfully configured a
> channel could we avoid this limitation?
>
> Cheers,
> Sam
>
>>
>> Test procedure:
>> Connected a L2 switch with only two ports connected.
>> One port was a DHCP corporate net, the other port attached to the
>> NCSI
>> controlled NIC.
>>
>> Starting with the L2 switch with DC on, check to make sure the NCSI
>> link is operating.
>> cat /sys/class/net/eth1/carrier
>> 1
>> cat /sys/class/net/eth1/carrier_up_count
>> 0
>> cat /sys/class/net/eth1/carrier_down_count
>> 0
>>
>> Remove DC from the L2 switch, and check link state
>> cat /sys/class/net/eth1/carrier
>> 0
>> cat /sys/class/net/eth1/carrier_up_count
>> 0
>> cat /sys/class/net/eth1/carrier_down_count
>> 1
>>
>> Restore DC to the L2 switch, and check link state
>> cat /sys/class/net/eth1/carrier
>> 1
>> cat /sys/class/net/eth1/carrier_up_count
>> 1
>> cat /sys/class/net/eth1/carrier_down_count
>> 1
>>
>> Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
>> ---
>> net/ncsi/ncsi-aen.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
>> index b635c194f0a8..274c415dcead 100644
>> --- a/net/ncsi/ncsi-aen.c
>> +++ b/net/ncsi/ncsi-aen.c
>> @@ -89,6 +89,12 @@ static int ncsi_aen_handler_lsc(struct
>> ncsi_dev_priv
>> *ndp,
>> if ((had_link == has_link) || chained)
>> return 0;
>>
>> + if (had_link) {
>> + netif_carrier_off(ndp->ndev.dev);
>> + } else {
>> + netif_carrier_on(ndp->ndev.dev);
>> + }
>> +
>> if (!ndp->multi_package && !nc->package->multi_channel) {
>> if (had_link) {
>> ndp->flags |= NCSI_DEV_RESHUFFLE;
>
--
Johnathan Mantey
Senior Software Engineer
*azad technology partners*
Contributing to Technology Innovation since 1992
Phone: (503) 712-6764
Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel
2020-01-14 16:47 ` Johnathan Mantey
@ 2020-01-15 1:24 ` samjonas
2020-01-15 1:40 ` samjonas
0 siblings, 1 reply; 6+ messages in thread
From: samjonas @ 2020-01-15 1:24 UTC (permalink / raw)
To: Johnathan Mantey, netdev; +Cc: davem
On Tue, 2020-01-14 at 08:47 -0800, Johnathan Mantey wrote:
> Sam,
>
> This code is working on a channel that is completely
> configured. This
> code is covering a situation where the RJ45 cable is intentionally or
> mistakenly removed from a system. In the event that the network cable
> is
> removed/damaged/slips, the kernel needs to change state to show that
> network interface no longer has a link. For my employers use case
> the
> loss of carrier is then added to a log of system state changes. Thus
> for
> each internal channel there needs to be a way to report that the
> channel
> has/does not have a link carrier.
Hi Johnathan,
Right, updating the carrier status for NC-SI enabled interfaces is
helpful for a number of use cases (DHCP, etc). At the moment this will
only update the status after a LSC AEN appears, so on init the carrier
status will always be "up" regardless of what's plugged into the
channels. Does NC-SI still behave as expected if the carrier status is
initially set to "down"?
(However if we did do that we would also need a change to set the
carrier status on on configuration, or for packages that don't support
AENs).
Cheers,
Sam
>
> On 1/13/20 4:43 PM, samjonas wrote:
> > On Fri, 2020-01-10 at 14:02 -0800, Johnathan Mantey wrote:
> > > From 76d99782ec897b010ba507895d60d27dca8dca44 Mon Sep 17 00:00:00
> > > 2001
> > > From: Johnathan Mantey <johnathanx.mantey@intel.com>
> > > Date: Fri, 10 Jan 2020 12:46:17 -0800
> > > Subject: [PATCH] Propagate NCSI channel carrier loss/gain events
> > > to
> > > the
> > > kernel
> > >
> > > Problem statement:
> > > Insertion or removal of a network cable attached to a NCSI
> > > controlled
> > > network channel does not notify the kernel of the loss/gain of
> > > the
> > > network link.
> > >
> > > The expectation is that /sys/class/net/eth(x)/carrier will change
> > > state after a pull/insertion event. In addition the
> > > carrier_up_count
> > > and carrier_down_count files should increment.
> > >
> > > Change statement:
> > > Use the NCSI Asynchronous Event Notification handler to detect a
> > > change in a NCSI link.
> > > Add code to propagate carrier on/off state to the network
> > > interface.
> > > The on/off state is only modified after the existing code
> > > identifies
> > > if the network device HAD or HAS a link state change.
> >
> > If we set the carrier state off until we successfully configured a
> > channel could we avoid this limitation?
> >
> > Cheers,
> > Sam
> >
> > >
> > > Test procedure:
> > > Connected a L2 switch with only two ports connected.
> > > One port was a DHCP corporate net, the other port attached to the
> > > NCSI
> > > controlled NIC.
> > >
> > > Starting with the L2 switch with DC on, check to make sure the
> > > NCSI
> > > link is operating.
> > > cat /sys/class/net/eth1/carrier
> > > 1
> > > cat /sys/class/net/eth1/carrier_up_count
> > > 0
> > > cat /sys/class/net/eth1/carrier_down_count
> > > 0
> > >
> > > Remove DC from the L2 switch, and check link state
> > > cat /sys/class/net/eth1/carrier
> > > 0
> > > cat /sys/class/net/eth1/carrier_up_count
> > > 0
> > > cat /sys/class/net/eth1/carrier_down_count
> > > 1
> > >
> > > Restore DC to the L2 switch, and check link state
> > > cat /sys/class/net/eth1/carrier
> > > 1
> > > cat /sys/class/net/eth1/carrier_up_count
> > > 1
> > > cat /sys/class/net/eth1/carrier_down_count
> > > 1
> > >
> > > Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
> > > ---
> > > net/ncsi/ncsi-aen.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> > > index b635c194f0a8..274c415dcead 100644
> > > --- a/net/ncsi/ncsi-aen.c
> > > +++ b/net/ncsi/ncsi-aen.c
> > > @@ -89,6 +89,12 @@ static int ncsi_aen_handler_lsc(struct
> > > ncsi_dev_priv
> > > *ndp,
> > > if ((had_link == has_link) || chained)
> > > return 0;
> > >
> > > + if (had_link) {
> > > + netif_carrier_off(ndp->ndev.dev);
> > > + } else {
> > > + netif_carrier_on(ndp->ndev.dev);
> > > + }
> > > +
> > > if (!ndp->multi_package && !nc->package->multi_channel) {
> > > if (had_link) {
> > > ndp->flags |= NCSI_DEV_RESHUFFLE;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel
2020-01-15 1:24 ` samjonas
@ 2020-01-15 1:40 ` samjonas
0 siblings, 0 replies; 6+ messages in thread
From: samjonas @ 2020-01-15 1:40 UTC (permalink / raw)
To: Johnathan Mantey, netdev; +Cc: davem
On Tue, 2020-01-14 at 17:24 -0800, samjonas wrote:
> On Tue, 2020-01-14 at 08:47 -0800, Johnathan Mantey wrote:
> > Sam,
> >
> > This code is working on a channel that is completely
> > configured. This
> > code is covering a situation where the RJ45 cable is intentionally
> > or
> > mistakenly removed from a system. In the event that the network
> > cable
> > is
> > removed/damaged/slips, the kernel needs to change state to show
> > that
> > network interface no longer has a link. For my employers use case
> > the
> > loss of carrier is then added to a log of system state changes.
> > Thus
> > for
> > each internal channel there needs to be a way to report that the
> > channel
> > has/does not have a link carrier.
>
> Hi Johnathan,
>
> Right, updating the carrier status for NC-SI enabled interfaces is
> helpful for a number of use cases (DHCP, etc). At the moment this
> will
> only update the status after a LSC AEN appears, so on init the
> carrier
> status will always be "up" regardless of what's plugged into the
> channels. Does NC-SI still behave as expected if the carrier status
> is
> initially set to "down"?
Ah I think I remember the issue with this; AFAIK netif_carrier_off()
will stop scheduling packets for the interface. The change here should
work for this specific case because the remote NIC will fire the LSC
asynchronously, but it probably has the nasty side effect of stopping
xmit for the entire local interface - ie, all packages and channels on
this interface.
With this change can you confirm that normal failover still works for
example, or otherwise try to configure the interface after a LSC/down
event?
Thanks,
Sam
> (However if we did do that we would also need a change to set the
> carrier status on on configuration, or for packages that don't
> support
> AENs).
>
> Cheers,
> Sam
>
> >
> > On 1/13/20 4:43 PM, samjonas wrote:
> > > On Fri, 2020-01-10 at 14:02 -0800, Johnathan Mantey wrote:
> > > > From 76d99782ec897b010ba507895d60d27dca8dca44 Mon Sep 17
> > > > 00:00:00
> > > > 2001
> > > > From: Johnathan Mantey <johnathanx.mantey@intel.com>
> > > > Date: Fri, 10 Jan 2020 12:46:17 -0800
> > > > Subject: [PATCH] Propagate NCSI channel carrier loss/gain
> > > > events
> > > > to
> > > > the
> > > > kernel
> > > >
> > > > Problem statement:
> > > > Insertion or removal of a network cable attached to a NCSI
> > > > controlled
> > > > network channel does not notify the kernel of the loss/gain of
> > > > the
> > > > network link.
> > > >
> > > > The expectation is that /sys/class/net/eth(x)/carrier will
> > > > change
> > > > state after a pull/insertion event. In addition the
> > > > carrier_up_count
> > > > and carrier_down_count files should increment.
> > > >
> > > > Change statement:
> > > > Use the NCSI Asynchronous Event Notification handler to detect
> > > > a
> > > > change in a NCSI link.
> > > > Add code to propagate carrier on/off state to the network
> > > > interface.
> > > > The on/off state is only modified after the existing code
> > > > identifies
> > > > if the network device HAD or HAS a link state change.
> > >
> > > If we set the carrier state off until we successfully configured
> > > a
> > > channel could we avoid this limitation?
> > >
> > > Cheers,
> > > Sam
> > >
> > > >
> > > > Test procedure:
> > > > Connected a L2 switch with only two ports connected.
> > > > One port was a DHCP corporate net, the other port attached to
> > > > the
> > > > NCSI
> > > > controlled NIC.
> > > >
> > > > Starting with the L2 switch with DC on, check to make sure the
> > > > NCSI
> > > > link is operating.
> > > > cat /sys/class/net/eth1/carrier
> > > > 1
> > > > cat /sys/class/net/eth1/carrier_up_count
> > > > 0
> > > > cat /sys/class/net/eth1/carrier_down_count
> > > > 0
> > > >
> > > > Remove DC from the L2 switch, and check link state
> > > > cat /sys/class/net/eth1/carrier
> > > > 0
> > > > cat /sys/class/net/eth1/carrier_up_count
> > > > 0
> > > > cat /sys/class/net/eth1/carrier_down_count
> > > > 1
> > > >
> > > > Restore DC to the L2 switch, and check link state
> > > > cat /sys/class/net/eth1/carrier
> > > > 1
> > > > cat /sys/class/net/eth1/carrier_up_count
> > > > 1
> > > > cat /sys/class/net/eth1/carrier_down_count
> > > > 1
> > > >
> > > > Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
> > > > ---
> > > > net/ncsi/ncsi-aen.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> > > > index b635c194f0a8..274c415dcead 100644
> > > > --- a/net/ncsi/ncsi-aen.c
> > > > +++ b/net/ncsi/ncsi-aen.c
> > > > @@ -89,6 +89,12 @@ static int ncsi_aen_handler_lsc(struct
> > > > ncsi_dev_priv
> > > > *ndp,
> > > > if ((had_link == has_link) || chained)
> > > > return 0;
> > > >
> > > > + if (had_link) {
> > > > + netif_carrier_off(ndp->ndev.dev);
> > > > + } else {
> > > > + netif_carrier_on(ndp->ndev.dev);
> > > > + }
> > > > +
> > > > if (!ndp->multi_package && !nc->package->multi_channel) {
> > > > if (had_link) {
> > > > ndp->flags |= NCSI_DEV_RESHUFFLE;
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-15 1:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10 22:02 [PATCH] Propagate NCSI channel carrier loss/gain events to the kernel Johnathan Mantey
2020-01-11 23:31 ` David Miller
2020-01-14 0:43 ` samjonas
2020-01-14 16:47 ` Johnathan Mantey
2020-01-15 1:24 ` samjonas
2020-01-15 1:40 ` samjonas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).