Re: [PATCH rdma-next v1 2/5] IB/cm: Simplify ib_cancel_mad() and ib_modify_mad() calls

[Jason Gunthorpe <jgg@nvidia.com>]

On Sun, Apr 11, 2021 at 03:21:49PM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markzhang@nvidia.com>
> 
> The mad_agent parameter is redundant since the struct ib_mad_send_buf
> already has a pointer of it.
> 
> Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>  drivers/infiniband/core/cm.c       | 101 ++++++++++++++++++-----------
>  drivers/infiniband/core/mad.c      |  17 ++---
>  drivers/infiniband/core/sa_query.c |   4 +-
>  include/rdma/ib_mad.h              |  27 ++++----
>  4 files changed, 84 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index e33b730107b4..f7f094861f79 100644
> +++ b/drivers/infiniband/core/cm.c
> @@ -1023,7 +1023,7 @@ static void cm_destroy_id(struct ib_cm_id *cm_id, int err)
>  		break;
>  	case IB_CM_SIDR_REQ_SENT:
>  		cm_id->state = IB_CM_IDLE;
> -		ib_cancel_mad(cm_id_priv->av.port->mad_agent, cm_id_priv->msg);
> +		ib_cancel_mad(cm_id_priv->msg);
>  		break;
>  	case IB_CM_SIDR_REQ_RCVD:
>  		cm_send_sidr_rep_locked(cm_id_priv,
> @@ -1034,7 +1034,7 @@ static void cm_destroy_id(struct ib_cm_id *cm_id, int err)
>  		break;
>  	case IB_CM_REQ_SENT:
>  	case IB_CM_MRA_REQ_RCVD:
> -		ib_cancel_mad(cm_id_priv->av.port->mad_agent, cm_id_priv->msg);
> +		ib_cancel_mad(cm_id_priv->msg);
>  		cm_send_rej_locked(cm_id_priv, IB_CM_REJ_TIMEOUT,
>  				   &cm_id_priv->id.device->node_guid,
>  				   sizeof(cm_id_priv->id.device->node_guid),
> @@ -1052,7 +1052,7 @@ static void cm_destroy_id(struct ib_cm_id *cm_id, int err)
>  		break;
>  	case IB_CM_REP_SENT:
>  	case IB_CM_MRA_REP_RCVD:
> -		ib_cancel_mad(cm_id_priv->av.port->mad_agent, cm_id_priv->msg);
> +		ib_cancel_mad(cm_id_priv->msg);
>  		cm_send_rej_locked(cm_id_priv, IB_CM_REJ_CONSUMER_DEFINED, NULL,
>  				   0, NULL, 0);
>  		goto retest;
> @@ -1070,7 +1070,7 @@ static void cm_destroy_id(struct ib_cm_id *cm_id, int err)
>  		cm_send_dreq_locked(cm_id_priv, NULL, 0);
>  		goto retest;
>  	case IB_CM_DREQ_SENT:
> -		ib_cancel_mad(cm_id_priv->av.port->mad_agent, cm_id_priv->msg);
> +		ib_cancel_mad(cm_id_priv->msg);
>  		cm_enter_timewait(cm_id_priv);
>  		goto retest;
>  	case IB_CM_DREQ_RCVD:
> @@ -1473,6 +1473,8 @@ int ib_send_cm_req(struct ib_cm_id *cm_id,
>  		if (ret)
>  			goto out;
>  	}
> +
> +	spin_lock_irqsave(&cm_id_priv->lock, flags);
>  	cm_id->service_id = param->service_id;
>  	cm_id->service_mask = ~cpu_to_be64(0);
>  	cm_id_priv->timeout_ms = cm_convert_to_ms(
> @@ -1489,7 +1491,7 @@ int ib_send_cm_req(struct ib_cm_id *cm_id,
>  
>  	ret = cm_alloc_msg(cm_id_priv, &cm_id_priv->msg);
>  	if (ret)
> -		goto out;
> +		goto error_alloc;
>  
>  	req_msg = (struct cm_req_msg *) cm_id_priv->msg->mad;
>  	cm_format_req(req_msg, cm_id_priv, param);
> @@ -1501,19 +1503,21 @@ int ib_send_cm_req(struct ib_cm_id *cm_id,
>  	cm_id_priv->rq_psn = cpu_to_be32(IBA_GET(CM_REQ_STARTING_PSN, req_msg));
>  
>  	trace_icm_send_req(&cm_id_priv->id);
> -	spin_lock_irqsave(&cm_id_priv->lock, flags);
>  	ret = ib_post_send_mad(cm_id_priv->msg, NULL);
> -	if (ret) {
> -		spin_unlock_irqrestore(&cm_id_priv->lock, flags);
> -		goto error2;
> -	}
> -	BUG_ON(cm_id->state != IB_CM_IDLE);
> +	if (ret)
> +		goto error_post_send_mad;
> +
>  	cm_id->state = IB_CM_REQ_SENT;
>  	spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>  	return 0;
>  
> -error2:	cm_free_msg(cm_id_priv->msg);
> -out:	return ret;
> +error_post_send_mad:
> +	cm_free_msg(cm_id_priv->msg);
> +	cm_id_priv->msg = NULL;

No, please use the patch series I made instead of this

https://lore.kernel.org/linux-rdma/20210329124101.GA887238@nvidia.com/

Jason