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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jun 12, 2023 at 10:30:44AM -0300, Jason Gunthorpe wrote:
> 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.

And I would like to see sparse/smatch to be fixed. It helps to do not
oversight things.

Thanks

> 
> Jason