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
