Re: [PATCH v2 2/9] ata: libata: Pass the ATA port argument directly to __ata_scsi_queuecmd()

[Niklas Cassel <cassel@kernel.org>]

On Tue, May 26, 2026 at 02:46:01PM -0700, Bart Van Assche wrote:
> On 5/26/26 8:07 AM, Niklas Cassel wrote:
> > Hello Bart,
> > 
> > On Thu, May 21, 2026 at 10:33:30AM -0700, Bart Van Assche wrote:
> > > Prepare for adding lock context annotations that refer to the ATA port
> > > argument (ap). No functionality has been changed.
> > 
> > For this and the other preparation patches:
> > Please add some additional text explaining why this change is required for
> > us to add lock context annotations.
> > 
> > I guess it is not possible to simply add __must_hold(dev->link->ap->lock)
> > annotation to __ata_scsi_queuecmd() ?
> 
> Context annotations with complex expressions are supported by Clang.
> However, annotating __ata_scsi_queuecmd() with
> __must_hold(dev->link->ap->lock) would require annotating many
> __ata_scsi_queuecmd() callers with __assume_ctx_lock(dev->link->ap->lock) to
> tell the compiler that dev->link->ap->lock is the same
> synchronization object as ap->lock. If the "ap" argument is added then
> these __assume_ctx_lock() annotations aren't necessary in the
> __ata_scsi_queuecmd() callers.

I do agree that the __assume_ctx_lock() annotations are quite ugly, and that
we would like to avoid them, and as much as possible only have __must_hold()
annotations.

While it makes the code slightly more ugly to provide an ap argument if
strictly not needed, the advantage of enabling clang context analysis
would justify it.

I just want to understand how it works a bit better...

It obviously cannot detect that the lock taken by ata_scsi_queuecmd()
(ap->lock) is the same as dev->link->ap->lock in __ata_scsi_queuecmd().

Too bad that there isn't a more clearer way to tell clang that the two
objects are the same, by having something like an
__objects_are_equal(dev->link->ap->lock, ap->lock) annotation in
__ata_scsi_queuecmd(). (Needing to have an annotation in the caller, is
quite ugly... and __assume_ctx_lock(), which takes a single argument, seems
way more error prone than an annotation that would take two arguments.)


> 
> Do you really want me to repeat this explanation in every patch that
> adds the "ap" argument to a function?

Yes, please.
Even if the motivation might be more or less the same in all the prep patches,
I think the motivation should be in all the prep patches.


> 
> > But at the same time, you do add __must_hold(dev->link->ap->lock) to e.g.
> > ata_scsi_translate().
> > 
> > Looking at the C-file, I can see that patch 9/9 adds:
> > 
> > +	/* Tell the compiler that dev->link->ap == ap. */
> > +	__assume_ctx_lock(dev->link->ap->lock);
> > 
> > to __ata_scsi_queuecmd().
> > 
> > but, that annotation is using dev->link->ap and not ap directly.
> 
> I will look into adding "ap" to the ata_scsi_translate() arguments and
> removing the __assume_ctx_lock() mentioned above.

Sounds good.


Overall comment, for the places where we have something like:
/* Tell the compiler that dev->link->ap == ap. */
__assume_ctx_lock(dev->link->ap->lock);

Since this is in the calling function, I can see that you try to put it
as close to the function that is being called (the callee), but would it
make sense to also mention the name of the callee in the comment?


> 
> > Patch 9/9 also adds a __must_hold(ap->lock) annotation to the declaration
> > of __ata_scsi_queuecmd(), i.e. in the header file.
> > 
> > Personally, I think that it makes more sense to have the annotation in the
> > definition (C-file), since that is what we most often read.
> > If clang requires us to also add the annotation to the declaration, then
> > perhaps we can have the annotation both in the C-file and the header file?
> > (Especially since you do annotate the function definition for those functions
> > that do not have a declaration in the header file.)
> 
> I will look into adding annotations to both the header and the C files.

Sounds good.


> 
> > Not strictly needed, but assuming that we still need to grow an ap parameter
> > to many functions, would it perhaps be possible to restructure the series like:
> > 1) Pass the ATA port argument directly to __ata_scsi_queuecmd()
> > 2) Add annotations to __ata_scsi_queuecmd()
> > 3) Pass the ATA port argument directly to ata_qc_schedule_eh()
> > 4) Add annotations to ata_qc_schedule_eh()
> > 5) Pass the ATA port argument directly to ata_qc_complete()
> > 6) Add annotations to ata_qc_complete()
> > ...
> > 
> > That should be possible, right?
> > (I guess you might need to reorder some patches.)
> 
> Hmm ... there is no way to check that the intermediate state will be
> consistent because with this approach CONTEXT_ANALYSIS := y would have
> to be moved to the end of the patch series. Once CONTEXT_ANALYSIS := y
> is moved to the end of the patch series, how to let the compiler verify
> that any intermediate annotations are consistent?

I was thinking to move CONTEXT_ANALYSIS := y
to the first patch that enables context analysis,
which in my suggestion above would be:
2) Add annotations to __ata_scsi_queuecmd()

My thinking was e.g. for patch 2) to e.g. only add annotations in:
__ata_scsi_queuecmd() and all functions calling it / above it.

I would assume that there is no requirement to add annotations to
all functions below it. (As you have told us that we can add annotations
to additional functions later.)

Thus enabling annotations for functions below __ata_scsi_queuecmd(), e.g.
ata_scsi_translate() could be done in a patch later in the series.

E.g.:
1) Pass the ATA port argument directly to __ata_scsi_queuecmd()
2) Add annotations to __ata_scsi_queuecmd()
3) Pass the ATA port argument directly to ata_scsi_translate()
4) Add annotations to ata_scsi_translate()

But perhaps I am misunderstanding something.


Kind regards,
Niklas