| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <209ba91aac614acb9dc74df18ff3ed45@AUSX13MPS306.AMER.DELL.COM>
Date: Tue, 6 Nov 2018 17:27:09 +0000
From: <Justin.Lee1@...l.com>
To: <sam@...dozajonas.com>, <netdev@...r.kernel.org>
CC: <davem@...emloft.net>, <linux-kernel@...r.kernel.org>,
<openbmc@...ts.ozlabs.org>
Subject: RE: [PATCH net-next 5/6] net/ncsi: Reset channel state in
ncsi_start_dev()
> On Mon, 2018-11-05 at 18:01 +0000, Justin.Lee1@...l.com wrote:
> > > On Tue, 2018-10-30 at 21:26 +0000, Justin.Lee1@...l.com wrote:
> > > > > +int ncsi_reset_dev(struct ncsi_dev *nd)
> > > > > +{
> > > > > + struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
> > > > > + struct ncsi_channel *nc, *active;
> > > > > + struct ncsi_package *np;
> > > > > + unsigned long flags;
> > > > > + bool enabled;
> > > > > + int state;
> > > > > +
> > > > > + active = NULL;
> > > > > + NCSI_FOR_EACH_PACKAGE(ndp, np) {
> > > > > + NCSI_FOR_EACH_CHANNEL(np, nc) {
> > > > > + spin_lock_irqsave(&nc->lock, flags);
> > > > > + enabled = nc->monitor.enabled;
> > > > > + state = nc->state;
> > > > > + spin_unlock_irqrestore(&nc->lock, flags);
> > > > > +
> > > > > + if (enabled)
> > > > > + ncsi_stop_channel_monitor(nc);
> > > > > + if (state == NCSI_CHANNEL_ACTIVE) {
> > > > > + active = nc;
> > > > > + break;
> > > >
> > > > Is the original intention to process the channel one by one?
> > > > If it is the case, there are two loops and we might need to use
> > > > "goto found" instead.
> > >
> > > Yes we'll need to break out of the package loop here as well.
> > >
> > > > > + }
> > > > > + }
> > > > > + }
> > > > > +
> > > >
> > > > found: ?
> > > >
> > > > > + if (!active) {
> > > > > + /* Done */
> > > > > + spin_lock_irqsave(&ndp->lock, flags);
> > > > > + ndp->flags &= ~NCSI_DEV_RESET;
> > > > > + spin_unlock_irqrestore(&ndp->lock, flags);
> > > > > + return ncsi_choose_active_channel(ndp);
> > > > > + }
> > > > > +
> > > > > + spin_lock_irqsave(&ndp->lock, flags);
> > > > > + ndp->flags |= NCSI_DEV_RESET;
> > > > > + ndp->active_channel = active;
> > > > > + ndp->active_package = active->package;
> > > > > + spin_unlock_irqrestore(&ndp->lock, flags);
> > > > > +
> > > > > + nd->state = ncsi_dev_state_suspend;
> > > > > + schedule_work(&ndp->work);
> > > > > + return 0;
> > > > > +}
> > > >
> > > > Also similar issue in ncsi_choose_active_channel() function below.
> > > >
> > > > > @@ -916,32 +1045,49 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> > > > >
> > > > > ncm = &nc->modes[NCSI_MODE_LINK];
> > > > > if (ncm->data[2] & 0x1) {
> > > > > - spin_unlock_irqrestore(&nc->lock, flags);
> > > > > found = nc;
> > > > > - goto out;
> > > > > + with_link = true;
> > > > > }
> > > > >
> > > > > - spin_unlock_irqrestore(&nc->lock, flags);
> > > > > + /* If multi_channel is enabled configure all valid
> > > > > + * channels whether or not they currently have link
> > > > > + * so they will have AENs enabled.
> > > > > + */
> > > > > + if (with_link || np->multi_channel) {
> > > >
> > > > I notice that there is a case that we will misconfigure the interface.
> > > > For example below, multi-channel is not enable for package 1.
> > > > But we enable the channel for ncsi2 below (package 1 channel 0) as that interface is the first
> > > > channel for that package with link.
> > >
> > > I don't think I see the issue here; multi-channel is not set on package
> > > 1, but both channels are in the channel whitelist. Channel 0 is
> > > configured since it's the first found on package 1, and channel 1 is not
> > > since channel 0 is already found. Are you expecting something different?
> > >
> >
> > The setting is that multi-package is enable for both package 0 and 1.
> > Multi-channel is only enabled for package 0.
> >
> > > > cat /sys/kernel/debug/ncsi_protocol/ncsi_device_
> > > > IFIDX IFNAME NAME PID CID RX TX MP MC WP WC PC CS PS LS RU CR NQ HA
> > > > =====================================================================
> > > > 2 eth2 ncsi0 000 000 1 1 1 1 1 1 0 2 1 1 1 1 0 1
> > > > 2 eth2 ncsi1 000 001 1 0 1 1 1 1 0 2 1 1 1 1 0 1
> > > > 2 eth2 ncsi2 001 000 1 0 1 0 1 1 0 2 1 1 1 1 0 1
> >
> > I was replying to the wrong old email and it might cause a bit confusion.
> > The first 1 meaning channel is enabled for package 1 channel 0 (ncsi2).
> > For eth2, we already has ncsi0 as the active channel with TX enable.
> > I would think that package doesn't have the multi-channel enabled and
> > we should not enable the channel for ncsi2. The problem is that package 1 doesn't
> > enable the multi-channel and it believes it needs to enable one channel for its package
> > but it doesn't aware that the other package already has one active channel.
>
> Ah, maybe the confusion here is that multi_channel is a per-package
> setting; it determines what a package does with its own channels.
>
> So you have package 0 with multi-channel enabled so it enables channels 0
> & 1.
> Then you have package 1 without multi-channel so it enables only channel
> 0.
> There is still only one Tx channel (package 0, channel 0).
>
> Does that sound right, or have I missed something?
Yes, you are right. There is only one TX enabled.
If we can hold off a few seconds before applying, then we will not see
these configuration changes in between the back to back netlink commands.
Thanks,
Justin
Powered by blists - more mailing lists