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

[Leon Romanovsky <leon@kernel.org>]

On Tue, Jan 14, 2025 at 03:42:08PM -0400, Jason Gunthorpe wrote:
> 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.

Sure, will change.

> 
> > +#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

I don't think that it is worth to have functions here.

Thanks

> 
> Jason