Re: [PATCH RFC 02/12] RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler

Guoqing Jiang
2022-11-15T10:24:13Z

On 11/14/22 10:39 PM, Haris Iqbal wrote:
> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang<guoqing.jiang@linux.dev> wrote:
>> The RDMA_CM_EVENT_CONNECT_REQUEST is quite different to other types,
>> let's checking it separately at the beginning of routine, then we can
>> avoid the identation accordingly.
>>
>> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
>> ---
>> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 17 ++++++-----------
>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> index 79504aaef0cc..2cc8b423bcaa 100644
>> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> @@ -1948,24 +1948,19 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>> static int rtrs_srv_rdma_cm_handler(struct rdma_cm_id *cm_id,
>> struct rdma_cm_event *ev)
>> {
>> - struct rtrs_srv_path *srv_path = NULL;
>> - struct rtrs_path *s = NULL;
>> -
>> - if (ev->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
>> - struct rtrs_con *c = cm_id->context;
>> -
>> - s = c->path;
>> - srv_path = to_srv_path(s);
>> - }
>> + struct rtrs_con *c = cm_id->context;
>> + struct rtrs_path *s = c->path;
>> + struct rtrs_srv_path *srv_path = to_srv_path(s);
> This isn't correct for the RDMA_CM_EVENT_CONNECT_REQUEST event. At
> that moment, cm_id->context is still holding a pointer to struct
> rtrs_srv_ctx. Even though it never gets used for this event here, its
> not right IMO to dereference in this incorrect manner.

But rtrs_rdma_connect will deference the context again for connect request.

> How about we move the check for the RDMA_CM_EVENT_CONNECT_REQUEST
> event outside (just like you did), but let the pointers be NULL, and
> only dereference after the if condition for
> RDMA_CM_EVENT_CONNECT_REQUEST event?

Ok, will do it though it need more lines which I tried to avoid.

Thanks,
Guoqing