Re: [PATCH rdma-next v1 1/2] IB/mad: Add state machine to MAD layer

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 07, 2025 at 04:02:12PM +0200, Leon Romanovsky wrote:
> +static void handle_send_state(struct ib_mad_send_wr_private *mad_send_wr,
> +		       struct ib_mad_agent_private *mad_agent_priv)
> +{
> +	if (!mad_send_wr->state) {

What is this doing? state is an enum, what is !state supposed to be? 0
is not a valid value in the enum.

> @@ -1118,15 +1209,12 @@ int ib_post_send_mad(struct ib_mad_send_buf *send_buf,
>  		mad_send_wr->max_retries = send_buf->retries;
>  		mad_send_wr->retries_left = send_buf->retries;
>  		send_buf->retries = 0;
> -		/* Reference for work request to QP + response */
> -		mad_send_wr->refcount = 1 + (mad_send_wr->timeout > 0);
> -		mad_send_wr->status = IB_WC_SUCCESS;
> +		mad_send_wr->state = 0;

Same, enums should not be assigned to constants. If you want another
state you need another IB_MAD_STATE value and use it here and above.

> +#define EXPECT_MAD_STATE(mad_send_wr, expected_state)                  \
> +	{                                                              \
> +		if (IS_ENABLED(CONFIG_LOCKDEP))                        \
> +			WARN_ON(mad_send_wr->state != expected_state); \
> +	}
> +#define EXPECT_MAD_STATE3(mad_send_wr, expected_state1, expected_state2, \
> +			  expected_state3)                               \
> +	{                                                                \
> +		if (IS_ENABLED(CONFIG_LOCKDEP))                          \
> +			WARN_ON(mad_send_wr->state != expected_state1 && \
> +				mad_send_wr->state != expected_state2 && \
> +				mad_send_wr->state != expected_state3);  \
> +	}
> +#define NOT_EXPECT_MAD_STATE(mad_send_wr, wrong_state)              \
> +	{                                                           \
> +		if (IS_ENABLED(CONFIG_LOCKDEP))                     \
> +			WARN_ON(mad_send_wr->state == wrong_state); \
> +	}

These could all be static inlines, otherwise at least
mad_send_wr->state needs brackets (mad_send_wr)->state

Jason