Re: [PATCH 1/2] svcrdma: Refactor the creation of listener CMA ID

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

On 6/3/24 21:23, Chuck Lever wrote:
> On Mon, Jun 03, 2024 at 06:59:13PM +0800, Guoqing Jiang wrote:
>>
>> On 5/31/24 21:15, cel@kernel.org wrote:
>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>
>>> In a moment, I will add a second consumer of CMA ID creation in
>>> svcrdma. Refactor so this code can be reused.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>    net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
>>>    1 file changed, 40 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> index 2b1c16b9547d..fa50b7494a0a 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -65,6 +65,8 @@
>>>    static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
>>>    						 struct net *net, int node);
>>> +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
>>> +				   struct rdma_cm_event *event);
>>>    static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>>>    					struct net *net,
>>>    					struct sockaddr *sa, int salen,
>>> @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
>>>    	}
>>>    }
>>> +static struct rdma_cm_id *
>>> +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
>>> +			  void *context)
>>> +{
>>> +	struct rdma_cm_id *listen_id;
>>> +	int ret;
>>> +
>>> +	listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
>>> +				   RDMA_PS_TCP, IB_QPT_RC);
>>> +	if (IS_ERR(listen_id))
>>> +		return listen_id;
>> I am wondering if above need to return PTR_ERR(listen_id),
> PTR_ERR would convert the listen_id error to an integer, but
> svc_rdma_create_listen_id() returns a pointer or an ERR_PTR. Thus
> using PTR_ERR() would be wrong in this case.
>
>
>> and I find some
>> callers (in net/rds/, nvme etc)
>> return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return ERR_PTR(ret)
>> with ret is set to PTR_ERR(id).
> These functions use PTR_ERR only when the calling function returns
> an int.

Thanks for the explanation!

Guoqing