[PATCHv2 07/17] nvme: add Clang context annotations for nvme_subsystem::lock

[Nilay Shroff]

On 6/30/26 4:42 AM, Marco Elver wrote:
> On Mon, 29 Jun 2026 at 14:47, Christoph Hellwig <hch at lst.de> wrote:
>>
>> On Fri, Jun 26, 2026 at 08:09:52PM +0530, Nilay Shroff wrote:
>>>>
>>> yes correct, but this one is tricky as the list has to be inited
>>> with non-zero value. Another way to fix this is by adding
>>> context_unsafe wrapper around INIT_LIST_HEAD(). Again overuse of
>>> those unsafe wrappers hinders readability.
>>>
>>> Would it make sense to introduce a helper, e.g. INIT_LIST_HEAD_UNSAFE(),
>>> that simply wraps INIT_LIST_HEAD() with context_unsafe()? It would document
>>> that this should only be used in cases where the caller knows the object
>>> has not yet been published and therefore no concurrent access is possible,
>>> such as during object initialization.
>>
>> I'll leave that to Marco and other core people, but I think this
>> would be a lot better than the current version.
> 
> Initialization of guarded objects has gone through a few iterations,
> so I don't want to open that can of worms again. I think the
> infrastructure we have now provides various options (the scoped guard
> machinery isn't the only way). You could just write:
> 
> /* Initializes unpublished lock-guarded variables. */
> context_unsafe(
>      INIT_LIST_HEAD(&subsys->nsheads);
>      // ... other guarded var init in same block ...
> );
> 
> context_unsafe() has a statement expression inside, so you can have
> multiple statements in one context_unsafe() block.
> And leave the mutex_init() where it was if you don't like the scoped_guard().
> 
> Although here you just have 1 INIT_LIST_HEAD() that needs that
> treatment, so "context_unsafe(INIT_LIST_HEAD(...))" would do -- it's
> just 9 chars more vs INIT_LIST_HEAD_UNSAFE(). And as soon as you have
> several guarded variable initializations in a block, you can group
> them as above.
> 
> If you introduce INIT_LIST_HEAD_UNSAFE(), that'll create more ways to
> initialize guarded variables. I'm not against it if none of the other
> ways work for you, but expanding the API surface also incurs a
> complexity cost.
> 
> The simplest option is just adding __context_unsafe(init) as an
> attribute to these init functions, although I see they do more than
> just trivial initialization, so keeping context analysis enabled might
> catch real bugs.

Makes sense. I think grouping all unpublished guarded variable initializations
under a single context_unsafe(...) block is a better approach than wrapping
each initialization separately, which can quickly clutter the code.

In fact, there are a few initialization functions in the NVMe driver where
I annotated the entire function with __context_unsafe(). I agree it would be
better to narrow the scope and annotate only the unpublished guarded variable
initializations using a single context_unsafe(...) block.

I'll update the next revision of the patchset accordingly and gather feedback
on that approach. Let's see what others think.

Thanks,
--Nilay