Re: [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@xxxxxx> 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