| Message ID | 1659336226-2-1-git-send-email-lizhijian@fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/ib_srpt: unify checking rdma_cm_id condition in srpt_cm_req_recv() | expand |
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.
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.
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.
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>
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
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 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(-)