Re: [PATCH 11/15] nvme: add Clang context annotations for nvme_queue::sq_lock

[Nilay Shroff]

On 6/10/26 10:03 PM, Bart Van Assche wrote:
> On 6/10/26 7:27 AM, Nilay Shroff wrote:
> >   static void nvme_free_queue(struct nvme_queue *nvmeq)
> > +    __context_unsafe(/* frees queue which is no longer in use */)
> >   {
> >       dma_free_coherent(nvmeq->dev->dev, CQ_SIZE(nvmeq),
> >                   (void *)nvmeq->cqes, nvmeq->cq_dma_addr);
> > @@ -2176,6 +2182,7 @@ static int queue_request_irq(struct nvme_queue *nvmeq)
> >   }
> >   static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
> > +    __context_unsafe(/* safe to init queue without any protection */)
> >   {
> >       struct nvme_dev *dev = nvmeq->dev;
>
> __context_unsafe() is a big hammer that disables context analysis
> for the entire function body. Has it been considered to use
> guard(..._init)(...) instead?
Yeah, I considered using guard(..._init), but it is a bit tricky in
this case. The lock (nvmeq->sq_lock) that protects nvmeq->sq_tail and
nvmeq->last_sq_tail is initialized in nvme_alloc_queue(), while those
fields are initialized later in nvme_init_queue(). Because the lock
initialization and the protected-field initialization happen in different
functions, I don't see a straightforward way to use guard() here to
create the synthetic acquire/release pattern around the accesses that
Clang complains about.

That said, I agree that __context_unsafe() is a fairly big hammer since
it disables context analysis for the entire function body. One alternative
would be to wrap the individual field initializations with context_unsafe(),
but that would require annotating several initialization/freeing operations
throughout these functions, which would add a fair amount of noise and make
the code less readable. Given that these functions operate on objects that
are still being initialized (or are already being torn down), using
__context_unsafe() seemed like the cleaner tradeoff.

Thanks,
--Nilay