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@xxxxxxxxxxxxxxxxx>
>> @@ -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;
