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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>> ---
>> 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.
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 <[email protected]>
On 8/1/22 20:35, [email protected] 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.
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 <[email protected]>
> Reviewed-by: Bart Van Assche <[email protected]>
> ---
> drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Applied to for-next.
Thanks,
Jason