> On Jun 12, 2023, at 9:30 AM, Jason Gunthorpe <jgg@xxxxxxxxxx> 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.
It is tricky, that's why I think it's better if the code were
more obvious.
> I've just been ignoring sparse locking annotations, they don't really
> work IMHO.
Thing is that sparse has other useful warnings that are harder
to see clearly if there's a lot of noise.
No big deal. I will drop it.
--
Chuck Lever
