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
