Re: [PATCH net] net/smc: correct the reason code in smc_listen_find_device when fallback

[Guangguan Wang <guangguan.wang@linux.alibaba.com>]

On 2023/10/17 15:37, Dust Li wrote:
> On Mon, Oct 16, 2023 at 02:11:53PM +0800, Guangguan Wang wrote:
> 
> Hi guangguan,
> 
> This patch doesn't apply clearly on net because it conflict with my
> previous patch 4abbd2e3c1db(net/smc: return the right falback reason
> when prefix checks fail), pls rebase.
>

This patch sent before the patch 4abbd2e3c1db applied. I will rebase it
in the next version.
 
>> The function smc_find_ism_store_rc is not only used for ism, so it is
>> reasonable to change the function name to smc_find_device_store_rc.
>>
>> The ini->rc is used to store the last error happened when finding usable
>> ism or rdma device in smc_listen_find_device, and is set by calling smc_
>> find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
>> the value can not be overwritten anymore. So the ini-rc should be set to
>> the error reason only when an error actually occurs.
>>
>> When finding ISM/RDMA devices, device not found is not a real error, as
>> not all machine have ISM/RDMA devices. Failures after device found, when
>> initializing device or when initializing connection, is real errors, and
>> should be store in ini->rc.
>>
>> SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
>> not require same prefix.
> 
> I think it's better to seperate this patch into 2:
> - one for changing the name from smc_find_ism_store_rc to smc_find_device_store_rc.
> - one for fixing the return reason.
> 
> More comments below.
> 
OK, I will seperate it.
>>
>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>

>> @@ -2213,11 +2211,12 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
>> 		goto not_found;
>> 	ini->ism_selected = 0;
>> 	rc = smc_listen_ism_init(new_smc, ini);
>> -	if (!rc)
>> +	if (!rc) {
>> +		smc_find_device_store_rc(rc, ini);
> 
> This smc_find_device_store_rc() seems useless when rc == 0 here ?

Oh, I see. The code here should be:

rc = smc_listen_ism_init(new_smc, ini);
if (!rc)
    return;
smc_find_device_store_rc(rc, ini);

not_found:
    ini->smcd_version &= ~SMC_V1;
    ini->ism_dev[0] = NULL;
    ini->is_smcd = false;


Thanks,
Guangguan Wang.
> 
>> 		return;		/* V1 ISM device found */
>> +	}
>>
>> not_found:
>> -	smc_find_ism_store_rc(rc, ini);
>> 	ini->smcd_version &= ~SMC_V1;
>> 	ini->ism_dev[0] = NULL;
>> 	ini->is_smcd = false;