* [PATCH v1 0/1] Fix the multi thread manner of NCSI driver @ 2023-04-24 11:47 Delphine CC Chiu 2023-04-24 11:47 ` [PATCH v1 1/1] net/ncsi: " Delphine CC Chiu 0 siblings, 1 reply; 4+ messages in thread From: Delphine CC Chiu @ 2023-04-24 11:47 UTC (permalink / raw) To: patrick Cc: Delphine CC Chiu, Samuel Mendoza-Jonas, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel 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. Delphine CC Chiu (1): net/ncsi: Fix the multi thread manner of NCSI driver net/ncsi/internal.h | 1 + net/ncsi/ncsi-manage.c | 101 +++++++++++++++++++++-------------------- net/ncsi/ncsi-rsp.c | 4 +- 3 files changed, 55 insertions(+), 51 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v1 1/1] net/ncsi: Fix the multi thread manner of NCSI driver 2023-04-24 11:47 [PATCH v1 0/1] Fix the multi thread manner of NCSI driver Delphine CC Chiu @ 2023-04-24 11:47 ` Delphine CC Chiu 2023-04-25 1:21 ` Jakub Kicinski 2023-04-26 19:42 ` Simon Horman 0 siblings, 2 replies; 4+ messages in thread From: Delphine CC Chiu @ 2023-04-24 11:47 UTC (permalink / raw) To: patrick, Samuel Mendoza-Jonas, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Delphine CC Chiu, netdev, linux-kernel 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. Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com> --- net/ncsi/internal.h | 1 + net/ncsi/ncsi-manage.c | 101 +++++++++++++++++++++-------------------- net/ncsi/ncsi-rsp.c | 4 +- 3 files changed, 55 insertions(+), 51 deletions(-) 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 */ 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; 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; case ncsi_dev_state_probe_dp: ndp->pending_req_num = 1; diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c index 6447a09932f5..c3045ac974cf 100644 --- a/net/ncsi/ncsi-rsp.c +++ b/net/ncsi/ncsi-rsp.c @@ -822,12 +822,13 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr) struct ncsi_rsp_gc_pkt *rsp; struct ncsi_dev_priv *ndp = nr->ndp; struct ncsi_channel *nc; + struct ncsi_package *np; size_t size; /* Find the channel */ rsp = (struct ncsi_rsp_gc_pkt *)skb_network_header(nr->rsp); ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel, - NULL, &nc); + &np, &nc); if (!nc) return -ENODEV; @@ -862,6 +863,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr) */ nc->vlan_filter.bitmap = U64_MAX; nc->vlan_filter.n_vids = rsp->vlan_cnt; + np->ndp->max_channel = rsp->channel_cnt; return 0; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] net/ncsi: Fix the multi thread manner of NCSI driver 2023-04-24 11:47 ` [PATCH v1 1/1] net/ncsi: " Delphine CC Chiu @ 2023-04-25 1:21 ` Jakub Kicinski 2023-04-26 19:42 ` Simon Horman 1 sibling, 0 replies; 4+ messages in thread From: Jakub Kicinski @ 2023-04-25 1:21 UTC (permalink / raw) To: Delphine CC Chiu Cc: patrick, Samuel Mendoza-Jonas, David S. Miller, Eric Dumazet, Paolo Abeni, netdev 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] net/ncsi: Fix the multi thread manner of NCSI driver 2023-04-24 11:47 ` [PATCH v1 1/1] net/ncsi: " Delphine CC Chiu 2023-04-25 1:21 ` Jakub Kicinski @ 2023-04-26 19:42 ` Simon Horman 1 sibling, 0 replies; 4+ messages in thread From: Simon Horman @ 2023-04-26 19:42 UTC (permalink / raw) To: Delphine CC Chiu Cc: patrick, Samuel Mendoza-Jonas, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel On Mon, Apr 24, 2023 at 07:47:42PM +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 Hi Delphine, a minor nit from my side: s/previos/previous/ > 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. > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-04-26 19:42 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-24 11:47 [PATCH v1 0/1] Fix the multi thread manner of NCSI driver Delphine CC Chiu 2023-04-24 11:47 ` [PATCH v1 1/1] net/ncsi: " Delphine CC Chiu 2023-04-25 1:21 ` Jakub Kicinski 2023-04-26 19:42 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).