Re: [PATCH v2 03/12] loop: Add lock context annotations

On 8/3/26 11:13 PM, Bart Van Assche wrote:
> On 8/3/26 6:11 AM, Nilay Shroff wrote:
>> On 7/31/26 1:28 AM, Bart Van Assche wrote:
>>> Add lock context annotations that are compatible with Clang. Swap the
>>> loop_ctl_mutex and loop_index_idr declarations such that __guarded_by()
>>> can be used.
>>>
>>> Cc: Nilay Shroff <[email protected]>
>>> Signed-off-by: Bart Van Assche <[email protected]>
>>> ---
>>>   drivers/block/loop.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>>> index a71fe763c933..e2ac19de11f4 100644
>>> --- a/drivers/block/loop.c
>>> +++ b/drivers/block/loop.c
>>> @@ -90,8 +90,8 @@ struct loop_cmd {
>>>   #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
>>>   #define LOOP_DEFAULT_HW_Q_DEPTH 128
>>> -static DEFINE_IDR(loop_index_idr);
>>>   static DEFINE_MUTEX(loop_ctl_mutex);
>>> +static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);
>>
>> Can we define helper macro here, which is more readable?
>>
>> # define DEFINE_IDR_GUARDED(name, lock)\
>>          __guarded_by(&(lock)) DEFINE_IDR(name);
>>
>> This is similar to what we have defined for guarded LIST_HEAD here:
>> https://lore.kernel.org/all/[email protected]/
> 
> Maybe I missed something but it's not clear to me how such a short macro
> helps? It doesn't seem to improve code readability to me?
> 
I think the helper makes the relationship between the IDR and its protecting
lock a little more explicit, so this is mostly a matter of readability/style.

I'm fine either way. Let's see if others have a preference.

Thanks,
--Nilay