Messages in this thread Patch in this message | | | Subject | Re: [PATCH net-next 5/6] net/ncsi: Reset channel state in ncsi_start_dev() | From | Samuel Mendoza-Jonas <> | Date | Fri, 02 Nov 2018 09:53:03 +1100 |
| |
On Tue, 2018-10-30 at 21:26 +0000, Justin.Lee1@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? > > 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 > 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.
> > } > > + 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.
> > 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; > } > >
| |