> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@xxxxxxxxx>
> Sent: Thursday, October 26, 2023 8:46 AM
> To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>; jgg@xxxxxxxx; leon@xxxxxxxxxx
> Cc: linux-rdma@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH 16/19] RDMA/siw: Remove
> siw_sk_assign_cm_upcalls
>
>
>
> On 10/25/23 21:19, Bernard Metzler wrote:
> >> -----Original Message-----
> >> From: Guoqing Jiang<guoqing.jiang@xxxxxxxxx>
> >> Sent: Monday, October 9, 2023 9:18 AM
> >> To: Bernard Metzler<BMT@xxxxxxxxxxxxxx>;jgg@xxxxxxxx;leon@xxxxxxxxxx
> >> Cc:linux-rdma@xxxxxxxxxxxxxxx
> >> Subject: [EXTERNAL] [PATCH 16/19] RDMA/siw: Remove
> siw_sk_assign_cm_upcalls
> >>
> >> Let's move it into siw_sk_save_upcalls, then we only need to
> >> get sk_callback_lock once. Also rename siw_sk_save_upcalls
> >> to better align with the new code.
> >>
> >> Signed-off-by: Guoqing Jiang<guoqing.jiang@xxxxxxxxx>
> >> ---
> >> drivers/infiniband/sw/siw/siw_cm.c | 19 ++++++-------------
> >> 1 file changed, 6 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
> >> b/drivers/infiniband/sw/siw/siw_cm.c
> >> index c3aa5533e75d..6866ec80473c 100644
> >> --- a/drivers/infiniband/sw/siw/siw_cm.c
> >> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> >> @@ -39,17 +39,7 @@ static void siw_cm_llp_error_report(struct sock *s);
> >> static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type
> >> reason,
> >> int status);
> >>
> >> -static void siw_sk_assign_cm_upcalls(struct sock *sk)
> >> -{
> >> - write_lock_bh(&sk->sk_callback_lock);
> >> - sk->sk_state_change = siw_cm_llp_state_change;
> >> - sk->sk_data_ready = siw_cm_llp_data_ready;
> >> - sk->sk_write_space = siw_cm_llp_write_space;
> >> - sk->sk_error_report = siw_cm_llp_error_report;
> >> - write_unlock_bh(&sk->sk_callback_lock);
> >> -}
> >> -
> >> -static void siw_sk_save_upcalls(struct sock *sk)
> > To simplify, I'd suggest doing it the other way around,
> > so having siw_sk_assign_cm_upcalls() including the
> > functionality of siw_sk_save_upcalls() first.
>
> I guess you mean below. If so, I will update it in v3.
right. assigning the cm upcalls is the first step and
needs the original socket callbacks saved. Later, maybe
we have to reassign the upcalls to execute the additional
RTS handshake.
>
> static void siw_sk_assign_cm_upcalls(struct sock *sk)
> -{
> - write_lock_bh(&sk->sk_callback_lock);
> - sk->sk_state_change = siw_cm_llp_state_change;
> - sk->sk_data_ready = siw_cm_llp_data_ready;
> - sk->sk_write_space = siw_cm_llp_write_space;
> - sk->sk_error_report = siw_cm_llp_error_report;
> - write_unlock_bh(&sk->sk_callback_lock);
> -}
> -
> -static void siw_sk_save_upcalls(struct sock *sk)
> {
> struct siw_cep *cep = sk_to_cep(sk);
>
> @@ -58,6 +48,11 @@ static void siw_sk_save_upcalls(struct sock *sk)
> cep->sk_data_ready = sk->sk_data_ready;
> cep->sk_write_space = sk->sk_write_space;
> cep->sk_error_report = sk->sk_error_report;
> +
> + sk->sk_state_change = siw_cm_llp_state_change;
> + sk->sk_data_ready = siw_cm_llp_data_ready;
> + sk->sk_write_space = siw_cm_llp_write_space;
> + sk->sk_error_report = siw_cm_llp_error_report;
> write_unlock_bh(&sk->sk_callback_lock);
> }
>
> > There is another function siw_sk_assign_rtr_upcalls(),
> > which re-assigns the upcalls for special handling of
> > an explicit RTR->RTS handshake if requested later during
> > connection setup.
>
> Ah, one is assign rtr upcall and another is assign cm upcall.
> Thanks for the explanation.
>
> Guoqing
