Date:   Tue, 14 Jan 2020 17:24:19 -0800
From:   samjonas <sam@...dozajonas.com>
To:     Johnathan Mantey <johnathanx.mantey@...el.com>,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net
Subject: Re: [PATCH] Propagate NCSI channel carrier loss/gain events to the
 kernel

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@...el.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@...el.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;
> 
> 

Powered by blists - more mailing lists