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
