[PATCH] RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv()

[PATCH] RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv()

[Li Zhijian]

Although rdma_cm_id and ib_cm_id passing to srpt_cm_req_recv() are
exclusive currently, all other checking condition are using rdma_cm_id.
So unify the 'if' condition to make the code more clear.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index c3036aeac89e..21cbe30d526f 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2218,13 +2218,13 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 	ch->zw_cqe.done = srpt_zerolength_write_done;
 	INIT_WORK(&ch->release_work, srpt_release_channel_work);
 	ch->sport = sport;
-	if (ib_cm_id) {
-		ch->ib_cm.cm_id = ib_cm_id;
-		ib_cm_id->context = ch;
-	} else {
+	if (rdma_cm_id) {
 		ch->using_rdma_cm = true;
 		ch->rdma_cm.cm_id = rdma_cm_id;
 		rdma_cm_id->context = ch;
+	} else {
+		ch->ib_cm.cm_id = ib_cm_id;
+		ib_cm_id->context = ch;
 	}
 	/*
 	 * ch->rq_size should be at least as large as the initiator queue
-- 
1.8.3.1

Re: [PATCH] RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv()

[Bart Van Assche]

On 7/31/22 23:43, Li Zhijian wrote:
> Although rdma_cm_id and ib_cm_id passing to srpt_cm_req_recv() are
> exclusive currently, all other checking condition are using rdma_cm_id.
> So unify the 'if' condition to make the code more clear.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>   drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index c3036aeac89e..21cbe30d526f 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -2218,13 +2218,13 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
>   	ch->zw_cqe.done = srpt_zerolength_write_done;
>   	INIT_WORK(&ch->release_work, srpt_release_channel_work);
>   	ch->sport = sport;
> -	if (ib_cm_id) {
> -		ch->ib_cm.cm_id = ib_cm_id;
> -		ib_cm_id->context = ch;
> -	} else {
> +	if (rdma_cm_id) {
>   		ch->using_rdma_cm = true;
>   		ch->rdma_cm.cm_id = rdma_cm_id;
>   		rdma_cm_id->context = ch;
> +	} else {
> +		ch->ib_cm.cm_id = ib_cm_id;
> +		ib_cm_id->context = ch;
>   	}
>   	/*
>   	 * ch->rq_size should be at least as large as the initiator queue

Although the above patch looks fine to me, I'm not sure this kind of 
changes should be considered as useful or as churn?

Bart.

Re: [PATCH] RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv()

[lizhijian]

On 02/08/2022 00:46, Bart Van Assche wrote:
> On 7/31/22 23:43, Li Zhijian wrote:
>> Although rdma_cm_id and ib_cm_id passing to srpt_cm_req_recv() are
>> exclusive currently, all other checking condition are using rdma_cm_id.
>> So unify the 'if' condition to make the code more clear.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> index c3036aeac89e..21cbe30d526f 100644
>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> @@ -2218,13 +2218,13 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
>>       ch->zw_cqe.done = srpt_zerolength_write_done;
>>       INIT_WORK(&ch->release_work, srpt_release_channel_work);
>>       ch->sport = sport;
>> -    if (ib_cm_id) {
>> -        ch->ib_cm.cm_id = ib_cm_id;
>> -        ib_cm_id->context = ch;
>> -    } else {
>> +    if (rdma_cm_id) {
>>           ch->using_rdma_cm = true;
>>           ch->rdma_cm.cm_id = rdma_cm_id;
>>           rdma_cm_id->context = ch;
>> +    } else {
>> +        ch->ib_cm.cm_id = ib_cm_id;
>> +        ib_cm_id->context = ch;
>>       }
>>       /*
>>        * ch->rq_size should be at least as large as the initiator queue
>
> Although the above patch looks fine to me, I'm not sure this kind of changes should be considered as useful or as churn?

Just want to make it more clear :). you can see below cleanup path, it's checking rdma_cm_id instead.
When i first saw these conditions, i was confused until i realized rdma_cm_id and ib_cm_id are always exclusive currently after looking into its callers

2483 free_ch:
2484         if (rdma_cm_id)
2485                 rdma_cm_id->context = NULL;
2486 else
2487                 ib_cm_id->context = NULL;
2488 kfree(ch);
2489         ch = NULL;
2490
2491         WARN_ON_ONCE(ret == 0);
2492
2493 reject:
2494         pr_info("Rejecting login with reason %#x\n", be32_to_cpu(rej->reason));
...
2499
2500         if (rdma_cm_id)
2501                 rdma_reject(rdma_cm_id, rej, sizeof(*rej),
2502 IB_CM_REJ_CONSUMER_DEFINED);
2503 else
2504                 ib_send_cm_rej(ib_cm_id, IB_CM_REJ_CONSUMER_DEFINED, NULL, 0,
2505                                rej, sizeof(*rej));

Thanks
Zhijian

>
> Bart.

Re: [PATCH] RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv()

[Bart Van Assche]

On 8/1/22 20:35, lizhijian@fujitsu.com wrote:
> On 02/08/2022 00:46, Bart Van Assche wrote:
>> Although the above patch looks fine to me, I'm not sure this kind
>> of changes should be considered as useful or as churn?
> 
> Just want to make it more clear :). you can see below cleanup path,
> it's checking rdma_cm_id instead. When i first saw these conditions,
> i was confused until i realized rdma_cm_id and ib_cm_id are always
> exclusive currently after looking into its callers

Ah, that's right. Thanks for the clarification.

Bart.

Re: [PATCH] RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv()

[Bart Van Assche]

On 7/31/22 23:43, Li Zhijian wrote:
> Although rdma_cm_id and ib_cm_id passing to srpt_cm_req_recv() are
> exclusive currently, all other checking condition are using rdma_cm_id.
> So unify the 'if' condition to make the code more clear.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv()

[Jason Gunthorpe]

On Mon, Aug 01, 2022 at 06:43:46AM +0000, Li Zhijian wrote:
> Although rdma_cm_id and ib_cm_id passing to srpt_cm_req_recv() are
> exclusive currently, all other checking condition are using rdma_cm_id.
> So unify the 'if' condition to make the code more clear.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied to for-next.

Thanks,
Jason