| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230424182119.0c34624c@kernel.org>
Date: Mon, 24 Apr 2023 18:21:19 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>
Cc: patrick@...cx.xyz, Samuel Mendoza-Jonas <sam@...dozajonas.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v1 1/1] net/ncsi: Fix the multi thread manner of NCSI
driver
On Mon, 24 Apr 2023 19:47:42 +0800 Delphine CC Chiu wrote:
> From: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.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.
Powered by blists - more mailing lists