[PATCH net-next 0/4] net/smc: do some cleanups in smc module

[PATCH net-next 0/4] net/smc: do some cleanups in smc module

[Zhengchao Shao]

Do some cleanups in smc module.

Zhengchao Shao (4):
  net/smc: remove unreferenced header in smc_loopback.h file
  net/smc: remove the fallback in __smc_connect
  net/smc: remove redundant code in smc_connect_check_aclc
  net/smc: remove unused input parameters in smcr_new_buf_create

 net/smc/af_smc.c       | 8 --------
 net/smc/smc_core.c     | 4 ++--
 net/smc/smc_loopback.h | 1 -
 3 files changed, 2 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH net-next 1/4] net/smc: remove unreferenced header in smc_loopback.h file

[Zhengchao Shao]

Because linux/err.h is unreferenced in smc_loopback.h file, so
remove it.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 net/smc/smc_loopback.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/smc/smc_loopback.h b/net/smc/smc_loopback.h
index 6dd4292dae56..04dc6808d2e1 100644
--- a/net/smc/smc_loopback.h
+++ b/net/smc/smc_loopback.h
@@ -15,7 +15,6 @@
 #define _SMC_LOOPBACK_H
 
 #include <linux/device.h>
-#include <linux/err.h>
 #include <net/smc.h>
 
 #if IS_ENABLED(CONFIG_SMC_LO)
-- 
2.34.1

Re: [PATCH net-next 1/4] net/smc: remove unreferenced header in smc_loopback.h file

[Simon Horman]

On Tue, Jul 30, 2024 at 09:25:03AM +0800, Zhengchao Shao wrote:
> Because linux/err.h is unreferenced in smc_loopback.h file, so
> remove it.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>

Thanks, I agree that noting provided by err.h appears
to be used either by smc_loopback.h or files that include it.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 1/4] net/smc: remove unreferenced header in smc_loopback.h file

[D. Wythe]

On 7/30/24 9:25 AM, Zhengchao Shao wrote:
> Because linux/err.h is unreferenced in smc_loopback.h file, so
> remove it.
>
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>   net/smc/smc_loopback.h | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/net/smc/smc_loopback.h b/net/smc/smc_loopback.h
> index 6dd4292dae56..04dc6808d2e1 100644
> --- a/net/smc/smc_loopback.h
> +++ b/net/smc/smc_loopback.h
> @@ -15,7 +15,6 @@
>   #define _SMC_LOOPBACK_H
>   
>   #include <linux/device.h>
> -#include <linux/err.h>
>   #include <net/smc.h>
>   
>   #if IS_ENABLED(CONFIG_SMC_LO)

LGTM, Thanks.

Reviewed-by: D. Wythe <alibuda@linux.alibaba.com>

Re: [PATCH net-next 1/4] net/smc: remove unreferenced header in smc_loopback.h file

[Wen Gu]

On 2024/7/30 09:25, Zhengchao Shao wrote:
> Because linux/err.h is unreferenced in smc_loopback.h file, so
> remove it.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>   net/smc/smc_loopback.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/net/smc/smc_loopback.h b/net/smc/smc_loopback.h
> index 6dd4292dae56..04dc6808d2e1 100644
> --- a/net/smc/smc_loopback.h
> +++ b/net/smc/smc_loopback.h
> @@ -15,7 +15,6 @@
>   #define _SMC_LOOPBACK_H
>   
>   #include <linux/device.h>
> -#include <linux/err.h>
>   #include <net/smc.h>
>   
>   #if IS_ENABLED(CONFIG_SMC_LO)

Yeah.. I used err.h in several intermediate versions of smc loopback,
but forgot to remove it in the final version. Thanks!

[PATCH net-next 2/4] net/smc: remove the fallback in __smc_connect

[Zhengchao Shao]

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);
-- 
2.34.1

Re: [PATCH net-next 2/4] net/smc: remove the fallback in __smc_connect

[Simon Horman]

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>

Re: [PATCH net-next 2/4] net/smc: remove the fallback in __smc_connect

[Wenjia Zhang]

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

Re: [PATCH net-next 2/4] net/smc: remove the fallback in __smc_connect

[shaozhengchao]

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

Re: [PATCH net-next 2/4] net/smc: remove the fallback in __smc_connect

[Wenjia Zhang]

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

Re: [PATCH net-next 2/4] net/smc: remove the fallback in __smc_connect

[shaozhengchao]

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

[PATCH net-next 3/4] net/smc: remove redundant code in smc_connect_check_aclc

[Zhengchao Shao]

When the SMC client perform CLC handshake, it will check whether
the clc header type is correct in receiving SMC_CLC_ACCEPT packet.
The specific invoking path is as follows:
__smc_connect
  smc_connect_clc
    smc_clc_wait_msg
      smc_clc_msg_hdr_valid
        smc_clc_msg_acc_conf_valid
Therefore, the smc_connect_check_aclc interface invoked by
__smc_connect does not need to check type again.

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 83f5a1849971..6f82e4d8fda4 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1466,10 +1466,6 @@ static int smc_connect_ism(struct smc_sock *smc,
 static int smc_connect_check_aclc(struct smc_init_info *ini,
 				  struct smc_clc_msg_accept_confirm *aclc)
 {
-	if (aclc->hdr.typev1 != SMC_TYPE_R &&
-	    aclc->hdr.typev1 != SMC_TYPE_D)
-		return SMC_CLC_DECL_MODEUNSUPP;
-
 	if (aclc->hdr.version >= SMC_V2) {
 		if ((aclc->hdr.typev1 == SMC_TYPE_R &&
 		     !smcr_indicated(ini->smc_type_v2)) ||
-- 
2.34.1

Re: [PATCH net-next 3/4] net/smc: remove redundant code in smc_connect_check_aclc

[Simon Horman]

On Tue, Jul 30, 2024 at 09:25:05AM +0800, Zhengchao Shao wrote:
> When the SMC client perform CLC handshake, it will check whether
> the clc header type is correct in receiving SMC_CLC_ACCEPT packet.
> The specific invoking path is as follows:
> __smc_connect
>   smc_connect_clc
>     smc_clc_wait_msg
>       smc_clc_msg_hdr_valid
>         smc_clc_msg_acc_conf_valid
> Therefore, the smc_connect_check_aclc interface invoked by
> __smc_connect does not need to check type again.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>

Thanks, I agree that in the case of a SMC_CLC_ACCEPT packet,
which is the case here, this check is unnecessary as it
has already been performed by smc_clc_msg_acc_conf_valid().

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 4/4] net/smc: remove unused input parameters in smcr_new_buf_create

[Zhengchao Shao]

The input parameter "is_rmb" of the smcr_new_buf_create function
has never been used, remove it.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 net/smc/smc_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 3b95828d9976..71fb334d8234 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2250,7 +2250,7 @@ int smcr_buf_reg_lgr(struct smc_link *lnk)
 }
 
 static struct smc_buf_desc *smcr_new_buf_create(struct smc_link_group *lgr,
-						bool is_rmb, int bufsize)
+						int bufsize)
 {
 	struct smc_buf_desc *buf_desc;
 
@@ -2398,7 +2398,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 		if (is_smcd)
 			buf_desc = smcd_new_buf_create(lgr, is_rmb, bufsize);
 		else
-			buf_desc = smcr_new_buf_create(lgr, is_rmb, bufsize);
+			buf_desc = smcr_new_buf_create(lgr, bufsize);
 
 		if (PTR_ERR(buf_desc) == -ENOMEM)
 			break;
-- 
2.34.1

Re: [PATCH net-next 4/4] net/smc: remove unused input parameters in smcr_new_buf_create

[Simon Horman]

On Tue, Jul 30, 2024 at 09:25:06AM +0800, Zhengchao Shao wrote:
> The input parameter "is_rmb" of the smcr_new_buf_create function
> has never been used, remove it.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 0/4] net/smc: do some cleanups in smc module

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 30 Jul 2024 09:25:02 +0800 you wrote:
> Do some cleanups in smc module.
> 
> Zhengchao Shao (4):
>   net/smc: remove unreferenced header in smc_loopback.h file
>   net/smc: remove the fallback in __smc_connect
>   net/smc: remove redundant code in smc_connect_check_aclc
>   net/smc: remove unused input parameters in smcr_new_buf_create
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] net/smc: remove unreferenced header in smc_loopback.h file
    https://git.kernel.org/netdev/net-next/c/1018825a9539
  - [net-next,2/4] net/smc: remove the fallback in __smc_connect
    https://git.kernel.org/netdev/net-next/c/5a7957571126
  - [net-next,3/4] net/smc: remove redundant code in smc_connect_check_aclc
    https://git.kernel.org/netdev/net-next/c/d37307eaac13
  - [net-next,4/4] net/smc: remove unused input parameters in smcr_new_buf_create
    https://git.kernel.org/netdev/net-next/c/0908503ade5f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html