| Message ID | 20240730012506.3317978-3-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5a79575711264b98b435e08107c9e5bf129df210 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/smc: do some cleanups in smc module | expand |
On Tue, Jul 30, 2024 at 09:25:04AM +0800, Zhengchao Shao wrote: > When the SMC client begins to connect to server, smcd_version is set > to SMC_V1 + SMC_V2. If fail to get VLAN ID, only SMC_V2 information > is left in smcd_version. And smcd_version will not be changed to 0. > Therefore, remove the fallback caused by the failure to get VLAN ID. > > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> Thanks, I agree that smcd_version, which is initialised just above the code modified by this patch, cannot be 0 at the point of the check removed by this patch. Reviewed-by: Simon Horman <horms@kernel.org>
On 30.07.24 03:25, Zhengchao Shao wrote: > When the SMC client begins to connect to server, smcd_version is set > to SMC_V1 + SMC_V2. If fail to get VLAN ID, only SMC_V2 information > is left in smcd_version. And smcd_version will not be changed to 0. > Therefore, remove the fallback caused by the failure to get VLAN ID. > > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > net/smc/af_smc.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > index 73a875573e7a..83f5a1849971 100644 > --- a/net/smc/af_smc.c > +++ b/net/smc/af_smc.c > @@ -1523,10 +1523,6 @@ static int __smc_connect(struct smc_sock *smc) > ini->smcd_version &= ~SMC_V1; > ini->smcr_version = 0; > ini->smc_type_v1 = SMC_TYPE_N; > - if (!ini->smcd_version) { > - rc = SMC_CLC_DECL_GETVLANERR; > - goto fallback; > - } > } > > rc = smc_find_proposal_devices(smc, ini); Though you're right that here smcd_version never gets 0, it actually is a bug from ("42042dbbc2eb net/smc: prepare for SMC-Rv2 connection"). The purpose of the check here was to fallback at a early phase before calling smc_find_proposal_devices(). However, this change is not wrong, just I personally like adding a check for smc_ism_is_v2_capable() more. Thanks, Wenjia
Hi Wenjia Zhang:
Looks like the logic you're saying is okay. Do I need another patch
to perfect it? As below:
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 73a875573e7a..b23d15506afc 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1523,7 +1523,7 @@ static int __smc_connect(struct smc_sock *smc)
ini->smcd_version &= ~SMC_V1;
ini->smcr_version = 0;
ini->smc_type_v1 = SMC_TYPE_N;
- if (!ini->smcd_version) {
+ if (!smc_ism_is_v2_capable()) {
rc = SMC_CLC_DECL_GETVLANERR;
goto fallback;
}
Thank you
Zhengchao Shao
On 2024/7/31 23:15, Wenjia Zhang wrote:
>
>
> On 30.07.24 03:25, Zhengchao Shao wrote:
>> When the SMC client begins to connect to server, smcd_version is set
>> to SMC_V1 + SMC_V2. If fail to get VLAN ID, only SMC_V2 information
>> is left in smcd_version. And smcd_version will not be changed to 0.
>> Therefore, remove the fallback caused by the failure to get VLAN ID.
>>
>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
>> ---
>> net/smc/af_smc.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 73a875573e7a..83f5a1849971 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -1523,10 +1523,6 @@ static int __smc_connect(struct smc_sock *smc)
>> ini->smcd_version &= ~SMC_V1;
>> ini->smcr_version = 0;
>> ini->smc_type_v1 = SMC_TYPE_N;
>> - if (!ini->smcd_version) {
>> - rc = SMC_CLC_DECL_GETVLANERR;
>> - goto fallback;
>> - }
>> }
>> rc = smc_find_proposal_devices(smc, ini);
>
> Though you're right that here smcd_version never gets 0, it actually is
> a bug from ("42042dbbc2eb net/smc: prepare for SMC-Rv2 connection"). The
> purpose of the check here was to fallback at a early phase before
> calling smc_find_proposal_devices(). However, this change is not wrong,
> just I personally like adding a check for smc_ism_is_v2_capable() more.
>
> Thanks,
> Wenjia
On 01.08.24 03:22, shaozhengchao wrote: > Hi Wenjia Zhang: > Looks like the logic you're saying is okay. Do I need another patch > to perfect it? As below: > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > index 73a875573e7a..b23d15506afc 100644 > --- a/net/smc/af_smc.c > +++ b/net/smc/af_smc.c > @@ -1523,7 +1523,7 @@ static int __smc_connect(struct smc_sock *smc) > ini->smcd_version &= ~SMC_V1; > ini->smcr_version = 0; > ini->smc_type_v1 = SMC_TYPE_N; > - if (!ini->smcd_version) { > + if (!smc_ism_is_v2_capable()) { > rc = SMC_CLC_DECL_GETVLANERR; > goto fallback; > } > > > Thank you > > Zhengchao Shao > Hi Zhengchao, I see your patches series were already applied yesterday. So It's okay to let it be now. As I said, your changes are not wrong, just not clean enough IMO. Anyway, thanks for your contribution to our code! Thanks, Wenjia
On 2024/8/1 15:23, Wenjia Zhang wrote: > > > On 01.08.24 03:22, shaozhengchao wrote: >> Hi Wenjia Zhang: >> Looks like the logic you're saying is okay. Do I need another patch >> to perfect it? As below: >> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c >> index 73a875573e7a..b23d15506afc 100644 >> --- a/net/smc/af_smc.c >> +++ b/net/smc/af_smc.c >> @@ -1523,7 +1523,7 @@ static int __smc_connect(struct smc_sock *smc) >> ini->smcd_version &= ~SMC_V1; >> ini->smcr_version = 0; >> ini->smc_type_v1 = SMC_TYPE_N; >> - if (!ini->smcd_version) { >> + if (!smc_ism_is_v2_capable()) { >> rc = SMC_CLC_DECL_GETVLANERR; >> goto fallback; >> } >> >> >> Thank you >> >> Zhengchao Shao >> > Hi Wenjia: I am currently testing the SMC-R/D, also interested in the SMC module. I will continue to review SMC code. :) Thank you Zhengchao Shao > Hi Zhengchao, > > I see your patches series were already applied yesterday. So It's okay > to let it be now. As I said, your changes are not wrong, just not clean > enough IMO. Anyway, thanks for your contribution to our code! > > Thanks, > Wenjia
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index 73a875573e7a..83f5a1849971 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -1523,10 +1523,6 @@ static int __smc_connect(struct smc_sock *smc) ini->smcd_version &= ~SMC_V1; ini->smcr_version = 0; ini->smc_type_v1 = SMC_TYPE_N; - if (!ini->smcd_version) { - rc = SMC_CLC_DECL_GETVLANERR; - goto fallback; - } } rc = smc_find_proposal_devices(smc, ini);
When the SMC client begins to connect to server, smcd_version is set to SMC_V1 + SMC_V2. If fail to get VLAN ID, only SMC_V2 information is left in smcd_version. And smcd_version will not be changed to 0. Therefore, remove the fallback caused by the failure to get VLAN ID. Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- net/smc/af_smc.c | 4 ---- 1 file changed, 4 deletions(-)