Re: [PATCH 1/2] IB/mad: pass ib_mad_send_buf explicitly to the recv_handler

[Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 1/4/2016 8:15 AM, Christoph Hellwig wrote:
> Stop abusing wr_id and just pass the parameter explicitly.
> 
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

Reviewed-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

> ---
>  drivers/infiniband/core/cm.c          |  1 +
>  drivers/infiniband/core/mad.c         | 18 ++++++++++--------
>  drivers/infiniband/core/sa_query.c    |  7 ++++---
>  drivers/infiniband/core/user_mad.c    |  1 +
>  drivers/infiniband/ulp/srpt/ib_srpt.c |  1 +
>  include/rdma/ib_mad.h                 |  2 ++
>  6 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index e3a95d1..ad3726d 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -3503,6 +3503,7 @@ int ib_cm_notify(struct ib_cm_id *cm_id, enum ib_event_type event)
>  EXPORT_SYMBOL(ib_cm_notify);
>  
>  static void cm_recv_handler(struct ib_mad_agent *mad_agent,
> +			    struct ib_mad_send_buf *send_buf,
>  			    struct ib_mad_recv_wc *mad_recv_wc)
>  {
>  	struct cm_port *port = mad_agent->context;
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
> index d4d2a61..cbe232a 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -693,7 +693,7 @@ static void snoop_recv(struct ib_mad_qp_info *qp_info,
>  
>  		atomic_inc(&mad_snoop_priv->refcount);
>  		spin_unlock_irqrestore(&qp_info->snoop_lock, flags);
> -		mad_snoop_priv->agent.recv_handler(&mad_snoop_priv->agent,
> +		mad_snoop_priv->agent.recv_handler(&mad_snoop_priv->agent, NULL,
>  						   mad_recv_wc);
>  		deref_snoop_agent(mad_snoop_priv);
>  		spin_lock_irqsave(&qp_info->snoop_lock, flags);
> @@ -1994,9 +1994,9 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
>  				/* user rmpp is in effect
>  				 * and this is an active RMPP MAD
>  				 */
> -				mad_recv_wc->wc->wr_id = 0;
> -				mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> -								   mad_recv_wc);
> +				mad_agent_priv->agent.recv_handler(
> +						&mad_agent_priv->agent, NULL,
> +						mad_recv_wc);
>  				atomic_dec(&mad_agent_priv->refcount);
>  			} else {
>  				/* not user rmpp, revert to normal behavior and
> @@ -2010,9 +2010,10 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
>  			spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
>  
>  			/* Defined behavior is to complete response before request */
> -			mad_recv_wc->wc->wr_id = (unsigned long) &mad_send_wr->send_buf;
> -			mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> -							   mad_recv_wc);
> +			mad_agent_priv->agent.recv_handler(
> +					&mad_agent_priv->agent,
> +					&mad_send_wr->send_buf,
> +					mad_recv_wc);
>  			atomic_dec(&mad_agent_priv->refcount);
>  
>  			mad_send_wc.status = IB_WC_SUCCESS;
> @@ -2021,7 +2022,7 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
>  			ib_mad_complete_send_wr(mad_send_wr, &mad_send_wc);
>  		}
>  	} else {
> -		mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> +		mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent, NULL,
>  						   mad_recv_wc);
>  		deref_mad_agent(mad_agent_priv);
>  	}
> @@ -2762,6 +2763,7 @@ static void local_completions(struct work_struct *work)
>  					   IB_MAD_SNOOP_RECVS);
>  			recv_mad_agent->agent.recv_handler(
>  						&recv_mad_agent->agent,
> +						&local->mad_send_wr->send_buf,
>  						&local->mad_priv->header.recv_wc);
>  			spin_lock_irqsave(&recv_mad_agent->lock, flags);
>  			atomic_dec(&recv_mad_agent->refcount);
> diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
> index e364a42..1f91b6e 100644
> --- a/drivers/infiniband/core/sa_query.c
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -1669,14 +1669,15 @@ static void send_handler(struct ib_mad_agent *agent,
>  }
>  
>  static void recv_handler(struct ib_mad_agent *mad_agent,
> +			 struct ib_mad_send_buf *send_buf,
>  			 struct ib_mad_recv_wc *mad_recv_wc)
>  {
>  	struct ib_sa_query *query;
> -	struct ib_mad_send_buf *mad_buf;
>  
> -	mad_buf = (void *) (unsigned long) mad_recv_wc->wc->wr_id;
> -	query = mad_buf->context[0];
> +	if (!send_buf)
> +		return;
>  
> +	query = send_buf->context[0];
>  	if (query->callback) {
>  		if (mad_recv_wc->wc->status == IB_WC_SUCCESS)
>  			query->callback(query,
> diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> index 57f281f..415a318 100644
> --- a/drivers/infiniband/core/user_mad.c
> +++ b/drivers/infiniband/core/user_mad.c
> @@ -210,6 +210,7 @@ static void send_handler(struct ib_mad_agent *agent,
>  }
>  
>  static void recv_handler(struct ib_mad_agent *agent,
> +			 struct ib_mad_send_buf *send_buf,
>  			 struct ib_mad_recv_wc *mad_recv_wc)
>  {
>  	struct ib_umad_file *file = agent->context;
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 105512d..1d78de1 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -455,6 +455,7 @@ static void srpt_mad_send_handler(struct ib_mad_agent *mad_agent,
>   * srpt_mad_recv_handler() - MAD reception callback function.
>   */
>  static void srpt_mad_recv_handler(struct ib_mad_agent *mad_agent,
> +				  struct ib_mad_send_buf *send_buf,
>  				  struct ib_mad_recv_wc *mad_wc)
>  {
>  	struct srpt_port *sport = (struct srpt_port *)mad_agent->context;
> diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h
> index ec9b44d..61c5baa 100644
> --- a/include/rdma/ib_mad.h
> +++ b/include/rdma/ib_mad.h
> @@ -438,6 +438,7 @@ typedef void (*ib_mad_snoop_handler)(struct ib_mad_agent *mad_agent,
>  /**
>   * ib_mad_recv_handler - callback handler for a received MAD.
>   * @mad_agent: MAD agent requesting the received MAD.
> + * @send_buf: Send buffer if found, else NULL
>   * @mad_recv_wc: Received work completion information on the received MAD.
>   *
>   * MADs received in response to a send request operation will be handed to
> @@ -447,6 +448,7 @@ typedef void (*ib_mad_snoop_handler)(struct ib_mad_agent *mad_agent,
>   * modify the data referenced by @mad_recv_wc.
>   */
>  typedef void (*ib_mad_recv_handler)(struct ib_mad_agent *mad_agent,
> +			 	    struct ib_mad_send_buf *send_buf,
>  				    struct ib_mad_recv_wc *mad_recv_wc);

Nit: remove the following text from ib_mad_recv_wc which is no longer valid:

 * For received response, the wr_id contains a pointer to the ib_mad_send_buf
 *   for the corresponding send request.

although there's one vestige of this left in local_completions where build_smp_wc is called which is removed in your subsequent patch "IB/mad: use CQ abstraction".

>  
>  /**
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html