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

[Guoqing Jiang <guoqing.jiang@linux.dev>]

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