Message-ID: <770869e375d49c2d5ced183e1f0466644c2be783.camel@mendozajonas.com>
Date:   Tue, 14 Jan 2020 17:40:21 -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 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@...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