> On Tue, 2018-10-30 at 21:26 +0000, justin.l...@dell.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.
> > 2 eth2 ncsi3 001 001 0 0 1 0 1 1 0 1 0 1 1 1 0 1
> > =====================================================================
> > MP: Multi-mode Package WP: Whitelist Package
> > MC: Multi-mode Channel WC: Whitelist Channel
> > PC: Primary Channel CS: Channel State IA/A/IV 1/2/3
> > PS: Poll Status LS: Link Status
> > RU: Running CR: Carrier OK
> > NQ: Queue Stopped HA: Hardware Arbitration
> >
> > I temporally change to the following to avoid that.
> > if ((with_link &&
> > !np->multi_channel &&
> > list_empty(&ndp->channel_queue)) ||
> > np->multi_channel) {
> >
> > > + spin_lock_irqsave(&ndp->lock, flags);
> > > + list_add_tail_rcu(&nc->link,
> > > + &ndp->channel_queue);
> > > + spin_unlock_irqrestore(&ndp->lock, flags);
> > > +
> > > + netdev_dbg(ndp->ndev.dev,
> > > + "NCSI: Channel %u added to queue
> > > (link %s)\n",
> > > + nc->id,
> > > + ncm->data[2] & 0x1 ? "up" : "down");
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&nc->lock, cflags);
> > > +
> > > + if (with_link && !np->multi_channel)
> > > + break;
> >
> > Similar issue here. As we are using break, so each package will configure
> > one active TX.
> >
>
> I believe this is handled properly in ncsi_channel_is_tx() in the most
> recent revision.
I saw this issue with the last revision. I was using the wrong email to reply.
>
> > > }
> > > + if (with_link && !ndp->multi_package)
> > > + break;
> > > }
> > >
> > > - if (!found) {
> > > + if (list_empty(&ndp->channel_queue) && found) {
> > > + netdev_info(ndp->ndev.dev,
> > > + "NCSI: No channel with link found, configuring
> > > channel %u\n",
> > > + found->id);
> > > + spin_lock_irqsave(&ndp->lock, flags);
> > > + list_add_tail_rcu(&found->link, &ndp->channel_queue);
> > > + spin_unlock_irqrestore(&ndp->lock, flags);
> > > + } else if (!found) {
> > > netdev_warn(ndp->ndev.dev,
> > > - "NCSI: No channel found with link\n");
> > > + "NCSI: No channel found to configure!\n");
> > > ncsi_report_link(ndp, true);
> > > return -ENODEV;
> > > }
> >
> > Also, for deselect package handler function, do we want to set to inactive
> > here?
> > If we just change the state, the cached data still keeps the old value. If
> > the new
> > ncsi_reset_dev() function is handling one by one, can we skip this part?
>
> Technically yes we could skip the state change here since
> ncsi_reset_dev() will have already done it. However if we send a DP
> command via some other means then it is probably best to ensure we treat
> all channels on that package as inactive.
When I tested, if I didn't comment out the state change in response handler,
ncsi_reset_dev() function will not handle properly and some channels got into
invisible state and at the end we lost those selectable channels.
>
> >
> > static int ncsi_rsp_handler_dp(struct ncsi_request *nr)
> > {
> > struct ncsi_rsp_pkt *rsp;
> > struct ncsi_dev_priv *ndp = nr->ndp;
> > struct ncsi_package *np;
> > struct ncsi_channel *nc;
> > unsigned long flags;
> >
> > /* Find the package */
> > rsp = (struct ncsi_rsp_pkt *)skb_network_header(nr->rsp);
> > ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
> > &np, NULL);
> > if (!np)
> > return -ENODEV;
> >
> > /* Change state of all channels attached to the package */
> > NCSI_FOR_EACH_CHANNEL(np, nc) {
> > spin_lock_irqsave(&nc->lock, flags);
> > nc->state = NCSI_CHANNEL_INACTIVE;
> >
> > spin_unlock_irqrestore(&nc->lock, flags);
> > }
> >
> > return 0;
> > }
> >
> >
