Re: [PATCH v1 1/1] net/ncsi: Fix the multi thread manner of NCSI driver

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 24 Apr 2023 19:47:42 +0800 Delphine CC Chiu wrote:
> From: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> 
> Currently NCSI driver will send several NCSI commands back
> to back without waiting the response of previous NCSI command
> or timeout in some state when NIC have multi channel. This
> operation against the single thread manner defined by NCSI
> SPEC(section 6.3.2.3 in DSP0222_1.1.1).
> 
> 1. Fix the problem of NCSI driver that sending command back
> to back without waiting the response of previos NCSI command
> or timeout to meet the single thread manner.
> 2. According to NCSI SPEC(section 6.2.13.1 in DSP0222_1.1.1),
> we should probe one channel at a time by sending NCSI commands
> (Clear initial state, Get version ID, Get capabilities...), than
> repeat this steps until the max number of channels which we got
> from NCSI command (Get capabilities) has been probed.

Sounds like this is a fix? Could you add a Fixes tag to point to
the commit where the problem was first present?

> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 03757e76bb6b..6701ac7d4249 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -337,6 +337,7 @@ struct ncsi_dev_priv {
>  #define NCSI_MAX_VLAN_VIDS	15
>  	struct list_head    vlan_vids;       /* List of active VLAN IDs */
>  
> +	unsigned char       max_channel;     /* Num of channels to probe   */

"max" usually equals "count - 1", so let's stick to count naming.
Standard calls this "Channel count", correct?

>  	bool                multi_package;   /* Enable multiple packages   */
>  	bool                mlx_multi_host;  /* Enable multi host Mellanox */
>  	u32                 package_whitelist; /* Packages to configure    */
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index d9da942ad53d..c31b9bf7d099 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -471,6 +471,7 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>  	struct ncsi_channel *nc, *tmp;
>  	struct ncsi_cmd_arg nca;
>  	unsigned long flags;
> +	static unsigned char channel_index;

Why static? We can't have function state in the kernel, there maybe
multiple NC-SI devices present. Should it be stored in ndp?

>  	int ret;
>  
>  	np = ndp->active_package;
> @@ -510,17 +511,21 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>  
>  		break;
>  	case ncsi_dev_state_suspend_gls:
> -		ndp->pending_req_num = np->channel_num;
> +		ndp->pending_req_num = 1;
>  
>  		nca.type = NCSI_PKT_CMD_GLS;
>  		nca.package = np->id;
>  
>  		nd->state = ncsi_dev_state_suspend_dcnt;
> -		NCSI_FOR_EACH_CHANNEL(np, nc) {
> -			nca.channel = nc->id;
> -			ret = ncsi_xmit_cmd(&nca);
> -			if (ret)
> -				goto error;
> +		nca.channel = channel_index;
> +		ret = ncsi_xmit_cmd(&nca);
> +		if (ret)
> +			goto error;
> +		channel_index++;
> +
> +		if (channel_index == ndp->max_channel) {
> +			channel_index = 0;
> +			nd->state = ncsi_dev_state_suspend_dcnt;
>  		}
>  
>  		break;
> @@ -1350,9 +1355,9 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
>  {
>  	struct ncsi_dev *nd = &ndp->ndev;
>  	struct ncsi_package *np;
> -	struct ncsi_channel *nc;
>  	struct ncsi_cmd_arg nca;
> -	unsigned char index;
> +	unsigned char package_index;
> +	static unsigned char channel_index;
>  	int ret;
>  
>  	nca.ndp = ndp;
> @@ -1367,8 +1372,8 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
>  		/* Deselect all possible packages */
>  		nca.type = NCSI_PKT_CMD_DP;
>  		nca.channel = NCSI_RESERVED_CHANNEL;
> -		for (index = 0; index < 8; index++) {
> -			nca.package = index;
> +		for (package_index = 0; package_index < 8; package_index++) {
> +			nca.package = package_index;
>  			ret = ncsi_xmit_cmd(&nca);
>  			if (ret)
>  				goto error;
> @@ -1431,21 +1436,46 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
>  		break;
>  #endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
>  	case ncsi_dev_state_probe_cis:
> -		ndp->pending_req_num = NCSI_RESERVED_CHANNEL;
> +	case ncsi_dev_state_probe_gvi:
> +	case ncsi_dev_state_probe_gc:
> +	case ncsi_dev_state_probe_gls:
> +		np = ndp->active_package;
> +		ndp->pending_req_num = 1;
>  
>  		/* Clear initial state */
> -		nca.type = NCSI_PKT_CMD_CIS;
> -		nca.package = ndp->active_package->id;
> -		for (index = 0; index < NCSI_RESERVED_CHANNEL; index++) {
> -			nca.channel = index;
> -			ret = ncsi_xmit_cmd(&nca);
> -			if (ret)
> -				goto error;
> +		if (nd->state == ncsi_dev_state_probe_cis)
> +			nca.type = NCSI_PKT_CMD_CIS;
> +		/* Retrieve version, capability or link status */
> +		else if (nd->state == ncsi_dev_state_probe_gvi)
> +			nca.type = NCSI_PKT_CMD_GVI;
> +		else if (nd->state == ncsi_dev_state_probe_gc)
> +			nca.type = NCSI_PKT_CMD_GC;
> +		else
> +			nca.type = NCSI_PKT_CMD_GLS;
> +
> +		nca.package = np->id;
> +		nca.channel = channel_index;
> +		ret = ncsi_xmit_cmd(&nca);
> +		if (ret)
> +			goto error;
> +
> +		if (nd->state == ncsi_dev_state_probe_cis) {
> +			nd->state = ncsi_dev_state_probe_gvi;
> +		} else if (nd->state == ncsi_dev_state_probe_gvi) {
> +			nd->state = ncsi_dev_state_probe_gc;
> +		} else if (nd->state == ncsi_dev_state_probe_gc) {
> +			nd->state = ncsi_dev_state_probe_gls;
> +		} else {
> +			nd->state = ncsi_dev_state_probe_cis;
> +			channel_index++;
>  		}
>  
> -		nd->state = ncsi_dev_state_probe_gvi;
> -		if (IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY))
> -			nd->state = ncsi_dev_state_probe_keep_phy;
> +		if (channel_index == ndp->max_channel) {
> +			channel_index = 0;
> +			nd->state = ncsi_dev_state_probe_dp;
> +			if (IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY))
> +				nd->state = ncsi_dev_state_probe_keep_phy;
> +		}
>  		break;
>  #if IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY)
>  	case ncsi_dev_state_probe_keep_phy:
> @@ -1461,35 +1491,6 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
>  		nd->state = ncsi_dev_state_probe_gvi;
>  		break;
>  #endif /* CONFIG_NCSI_OEM_CMD_KEEP_PHY */
> -	case ncsi_dev_state_probe_gvi:
> -	case ncsi_dev_state_probe_gc:
> -	case ncsi_dev_state_probe_gls:
> -		np = ndp->active_package;
> -		ndp->pending_req_num = np->channel_num;
> -
> -		/* Retrieve version, capability or link status */
> -		if (nd->state == ncsi_dev_state_probe_gvi)
> -			nca.type = NCSI_PKT_CMD_GVI;
> -		else if (nd->state == ncsi_dev_state_probe_gc)
> -			nca.type = NCSI_PKT_CMD_GC;
> -		else
> -			nca.type = NCSI_PKT_CMD_GLS;
> -
> -		nca.package = np->id;
> -		NCSI_FOR_EACH_CHANNEL(np, nc) {
> -			nca.channel = nc->id;
> -			ret = ncsi_xmit_cmd(&nca);
> -			if (ret)
> -				goto error;
> -		}
> -
> -		if (nd->state == ncsi_dev_state_probe_gvi)
> -			nd->state = ncsi_dev_state_probe_gc;
> -		else if (nd->state == ncsi_dev_state_probe_gc)
> -			nd->state = ncsi_dev_state_probe_gls;
> -		else
> -			nd->state = ncsi_dev_state_probe_dp;
> -		break;

Could you make the code move a separate patch (making the submission 
a 2 patch series) for ease of review? Otherwise it's tricky to see what
actually changed here.