Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   <Justin.Lee1@Dell.com>
To:     <sam@mendozajonas.com>, <netdev@vger.kernel.org>
CC:     <davem@davemloft.net>, <linux-kernel@vger.kernel.org>,
        <openbmc@lists.ozlabs.org>
Subject: RE: [PATCH net-next v3 6/6] net/ncsi: Configure multi-package,
 multi-channel modes with failover
Thread-Topic: [PATCH net-next v3 6/6] net/ncsi: Configure multi-package,
 multi-channel modes with failover
Thread-Index: AQHUdw3T7sTeKH7RI0a85rjfghKYaaVGRmNg
Date:   Thu, 8 Nov 2018 22:48:09 +0000
Message-ID: <f9d0309dafe248a6aac8198f96f9bc4b@AUSX13MPS306.AMER.DELL.COM>
References: <20181108024909.9897-1-sam@mendozajonas.com>
 <20181108024909.9897-7-sam@mendozajonas.com>
In-Reply-To: <20181108024909.9897-7-sam@mendozajonas.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Samuel,

For multi-package and multi-channel case, channel seems to be select correc=
tly. Expect that,
I still see the timing issue for back-to-back netlink command. Due to that,=
 channel might be
set to invisible state. Please refer to ncsi0 and ncsi2 below. The channel =
state is set to 3.

cat /sys/kernel/debug/ncsi_protocol/ncsi_device_status
IFIDX IFNAME NAME   PID CID RX TX MP MC WP WC PC CS PS LS RU CR NQ HA
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
  2   eth2   ncsi0  000 000 1  1  1  1  1  0  0  3  0  1  1  1  0  1
  2   eth2   ncsi1  000 001 0  0  1  1  1  0  0  1  0  1  1  1  0  1
  2   eth2   ncsi2  001 000 1  0  1  1  1  1  0  3  0  1  1  1  0  1
  2   eth2   ncsi3  001 001 1  0  1  1  1  1  0  2  1  1  1  1  0  1
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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

The timing issue is not only happening in application. If I use using the f=
ollowing way
to send the request, I can see the issue as well.=20

ncsi_netlink -l 2 -a 0x01 -m; ncsi_netlink -l 2 -p 0 -b 0x03 -m; ncsi_netli=
nk -l 2 -p 1 -b 0x00 -m;
ncsi_netlink -l 2 -a 0x03 -m; ncsi_netlink -l 2 -p 0 -b 0x00 -m; ncsi_netli=
nk -l 2 -p 1 -b 0x03 -m;


Also, there is one issue below for non-multi-package/non-multi-channel case=
.

Thanks,
Justin


> @@ -1008,32 +1164,49 @@ static int ncsi_choose_active_channel(struct ncsi=
_dev_priv *ndp)
> =20
>  			ncm =3D &nc->modes[NCSI_MODE_LINK];
>  			if (ncm->data[2] & 0x1) {
> -				spin_unlock_irqrestore(&nc->lock, flags);
>  				found =3D nc;
> -				goto out;
> +				with_link =3D true;
>  			}
> =20
> -			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) {
> +				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;

The line needs to change to "goto found". If not, all channels with link wi=
ll be added
even if the multi-channel is not enabled for that package. The ncsi1 below =
is enabled.
There is no netlink command sent to enable multi-package or multi-channel.

IFIDX IFNAME NAME   PID CID RX TX MP MC WP WC PC CS PS LS RU CR NQ HA
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
  2   eth2   ncsi0  000 000 1  1  0  0  1  1  0  2  1  1  1  1  0  1
  2   eth2   ncsi1  000 001 1  0  0  0  1  1  0  2  1  1  1  1  0  1
  2   eth2   ncsi2  001 000 0  0  0  0  1  1  0  1  0  1  1  1  0  1
  2   eth2   ncsi3  001 001 0  0  0  0  1  1  0  1  0  1  1  1  0  1
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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

>  		}
> +		if (with_link && !ndp->multi_package)
> +			break;
>  	}

found:

After applying this change, I notice that if there is no link available to =
BMC when BMC
starts, NC-SI can't properly configure channel once I plug in the Ethernet =
cable.=20

npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_aen_handler_lsc() - pkg 0 ch 0 st=
ate up
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_aen_handler_lsc() - had_link 0, h=
as_link 1, chained 0
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_stop_channel_monitor() - pkg 0 ch=
 0
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_process_next_channel()
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_process_next_channel() - pkg 0 ch=
 0 INVISIBLE
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_process_next_channel() - suspendi=
ng pkg 0 ch 0
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_suspend_channel() - pkg 0 ch 0 st=
ate 0400 select
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_dev_work()
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_suspend_channel() - pkg 0 ch 0 st=
ate 0403 dc
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_dev_work()
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_suspend_channel() - pkg 0 ch 0 st=
ate 0404 deselect
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_dev_work()
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_suspend_channel() - pkg 0 ch 0 st=
ate 0405 done
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_rsp_handler_dp() - pkg 0 ch 0 INA=
CTIVE
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_rsp_handler_dp() - pkg 0 ch 1 INA=
CTIVE
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_dev_work()
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_suspend_channel() - pkg 0 ch 0 st=
ate 0406 deselect
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_suspend_channel() - pkg 0 ch 0 IN=
ACTIVE
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_process_next_channel()
npcm7xx-emc f0825000.eth eth2: NCSI: ncsi_process_next_channel() - No more =
channels to process
npcm7xx-emc f0825000.eth eth2: NCSI interface down

> =20
> -	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;
>  	}
> =20
> -	ncm =3D &found->modes[NCSI_MODE_LINK];
> -	netdev_dbg(ndp->ndev.dev,
> -		   "NCSI: Channel %u added to queue (link %s)\n",
> -		   found->id, ncm->data[2] & 0x1 ? "up" : "down");
> -
> -out:
> -	spin_lock_irqsave(&ndp->lock, flags);
> -	list_add_tail_rcu(&found->link, &ndp->channel_queue);
> -	spin_unlock_irqrestore(&ndp->lock, flags);
> -
>  	return ncsi_process_next_channel(ndp);
>  }