On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>
