| Message ID | 20221113010823.6436-3-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Misc changes for rtrs | expand |
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. 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? > > - switch (ev->event) { > - case RDMA_CM_EVENT_CONNECT_REQUEST: > + if (ev->event == RDMA_CM_EVENT_CONNECT_REQUEST) > /* > * In case of error cma.c will destroy cm_id, > * see cma_process_remove() > */ > return rtrs_rdma_connect(cm_id, ev->param.conn.private_data, > ev->param.conn.private_data_len); > + > + switch (ev->event) { > case RDMA_CM_EVENT_ESTABLISHED: > /* Nothing here */ > break; > -- > 2.31.1 >
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
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); - switch (ev->event) { - case RDMA_CM_EVENT_CONNECT_REQUEST: + if (ev->event == RDMA_CM_EVENT_CONNECT_REQUEST) /* * In case of error cma.c will destroy cm_id, * see cma_process_remove() */ return rtrs_rdma_connect(cm_id, ev->param.conn.private_data, ev->param.conn.private_data_len); + + switch (ev->event) { case RDMA_CM_EVENT_ESTABLISHED: /* Nothing here */ break;
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(-)