Re: [PATCH] RDMA/cma: Address sparse warnings

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jun 12, 2023 at 01:27:23PM +0000, Chuck Lever III wrote:

> > I think this change will solve it.
> > 
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 93a1c48d0c32..435ac3c93c1f 100644
> > --- a/drivers/infiniband/core/cma.c
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -2043,7 +2043,7 @@ static void _destroy_id(struct rdma_id_private *id_priv,
> >  * handlers can start running concurrently.
> >  */
> > static void destroy_id_handler_unlock(struct rdma_id_private *id_priv)
> > - 	__releases(&idprv->handler_mutex)
> > + 	__releases(&id_prv->handler_mutex)
> 
> The argument of __releases() is still mis-spelled: s/id_prv/id_priv/
> 
> I can't say I like this solution. It adds clutter but doesn't improve
> the documentation of the lock ordering.
> 
> Instead, I'd pull the mutex_unlock() out of destroy_id_handler_unlock(),
> and then make each of the call sites do the unlock. For instance:
> 
>  void rdma_destroy_id(struct rdma_cm_id *id)
>  {
>         struct rdma_id_private *id_priv =
>                 container_of(id, struct rdma_id_private, id);
> +       enum rdma_cm_state state;
> 
>         mutex_lock(&id_priv->handler_mutex);
> -       destroy_id_handler_unlock(id_priv);
> +       state = destroy_id_handler(id_priv);
> +       mutex_unlock(&id_priv->handler_mutex);
> +       _destroy_id(id_priv, state);
>  }
>  EXPORT_SYMBOL(rdma_destroy_id);
> 
> That way, no annotation is necessary, and both a human being and
> sparse can easily agree that the locking is correct.

I don't like it, there are a lot of call sites and this is tricky
stuff.

I've just been ignoring sparse locking annotations, they don't really
work IMHO.

Jason