* [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).