RE: [PATCH net-next 5/6] net/ncsi: Reset channel state in ncsi_start_dev()

[Justin.Lee1]

> +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.

> +			}
> +		}
> +	}
> +

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.

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.

>  		}
> +		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?

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;
}