[PATCH v2 0/9] ata: libata-core: Enable context analysis

[PATCH v2 0/9] ata: libata-core: Enable context analysis

[Bart Van Assche]

Hi Niklas,

This patch series enables context analysis for the ATA subsystem. Please
consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v1:
 - Expanded a single patch into a series of nine patches.
 - Included a bug fix for ata_exec_internal() and several refactoring patches.
 - Added a patch for annotating the code that uses the host lock.

Bart Van Assche (9):
  ata: libata: Fix ata_exec_internal()
  ata: libata: Pass the ATA port argument directly to
    __ata_scsi_queuecmd()
  ata: libata: Pass the ATA port argument directly to
    ata_qc_schedule_eh()
  ata: libata: Pass the ATA port argument directly to ata_qc_complete()
  ata: libata: Pass the ATA port argument directly to ata_qc_issue()
  ata: libata: Pass the ATA port argument directly to
    __ata_qc_complete()
  ata: libata: Pass the ATA port argument directly to ata_link_abort()
  ata: libata: Enable context analysis
  ata: Annotate the code that uses the host lock

 drivers/ata/Makefile          |  2 ++
 drivers/ata/ahci.h            |  6 +++--
 drivers/ata/ahci_imx.c        |  1 +
 drivers/ata/ahci_xgene.c      |  1 +
 drivers/ata/libahci.c         | 11 +++++++-
 drivers/ata/libata-core.c     | 51 ++++++++++++++++++++++++-----------
 drivers/ata/libata-eh.c       | 23 +++++++++++-----
 drivers/ata/libata-pmp.c      |  5 ++++
 drivers/ata/libata-sata.c     |  4 +--
 drivers/ata/libata-scsi.c     | 18 +++++++++----
 drivers/ata/libata-sff.c      | 12 +++++++--
 drivers/ata/libata.h          | 28 ++++++++++++-------
 drivers/ata/pata_arasan_cf.c  |  1 +
 drivers/ata/pdc_adma.c        | 16 ++++++++---
 drivers/ata/sata_dwc_460ex.c  |  9 ++++++-
 drivers/ata/sata_fsl.c        | 12 +++++++--
 drivers/ata/sata_inic162x.c   | 10 +++++--
 drivers/ata/sata_mv.c         | 18 +++++++++++--
 drivers/ata/sata_nv.c         | 18 +++++++++++++
 drivers/ata/sata_promise.c    | 16 +++++++++--
 drivers/ata/sata_qstor.c      | 20 ++++++++++++--
 drivers/ata/sata_rcar.c       |  5 ++++
 drivers/ata/sata_sil.c        |  4 +++
 drivers/ata/sata_sil24.c      | 10 ++++++-
 drivers/ata/sata_sx4.c        | 11 +++++---
 drivers/ata/sata_via.c        |  1 +
 drivers/ata/sata_vsc.c        |  7 +++++
 drivers/scsi/libsas/sas_ata.c |  4 +--
 include/linux/libata.h        | 50 ++++++++++++++++++++++------------
 29 files changed, 293 insertions(+), 81 deletions(-)

[PATCH v2 1/9] ata: libata: Fix ata_exec_internal()

[Bart Van Assche]

Some but not all ata_exec_internal() calls happen from the context of
the ATA error handler. Commit c0c362b60e25 ("libata: implement cross-port
EH exclusion") added ata_eh_release() and ata_eh_acquire() calls in
ata_exec_internal(). Calling these functions is necessary if the caller
holds the eh_mutex but is not allowed if the caller doesn't hold that
mutex. Fix this by only calling ata_eh_release() and ata_eh_acquire() if
the caller holds the eh_mutex. An example of an indirect caller of
ata_exec_internal() that does not hold the eh_mutex is
ata_host_register().

Fixes: c0c362b60e25 ("libata: implement cross-port EH exclusion")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/libata-core.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e76d15411e2a..3d25dec6ac6c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1540,6 +1540,7 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
 {
 	struct ata_link *link = dev->link;
 	struct ata_port *ap = link->ap;
+	const bool owns_eh_mutex = ap->host->eh_owner == current;
 	u8 command = tf->command;
 	struct ata_queued_cmd *qc;
 	struct scatterlist sgl;
@@ -1617,11 +1618,25 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
 		}
 	}
 
-	ata_eh_release(ap);
+	if (owns_eh_mutex) {
+		/*
+		 * To prevent that the compiler complains about the
+		 * ata_eh_release() call below.
+		 */
+		__acquire(&ap->host->eh_mutex);
+		ata_eh_release(ap);
+	}
 
 	rc = wait_for_completion_timeout(&wait, msecs_to_jiffies(timeout));
 
-	ata_eh_acquire(ap);
+	if (owns_eh_mutex) {
+		ata_eh_acquire(ap);
+		/*
+		 * To prevent that the compiler complains about the above
+		 * ata_eh_acquire() call.
+		 */
+		__release(&ap->host->eh_mutex);
+	}
 
 	ata_sff_flush_pio_task(ap);
 

Re: [PATCH v2 1/9] ata: libata: Fix ata_exec_internal()

[Niklas Cassel]

On Thu, May 21, 2026 at 10:33:29AM -0700, Bart Van Assche wrote:
> Some but not all ata_exec_internal() calls happen from the context of
> the ATA error handler. Commit c0c362b60e25 ("libata: implement cross-port
> EH exclusion") added ata_eh_release() and ata_eh_acquire() calls in
> ata_exec_internal(). Calling these functions is necessary if the caller
> holds the eh_mutex but is not allowed if the caller doesn't hold that
> mutex. Fix this by only calling ata_eh_release() and ata_eh_acquire() if
> the caller holds the eh_mutex. An example of an indirect caller of
> ata_exec_internal() that does not hold the eh_mutex is
> ata_host_register().
> 
> Fixes: c0c362b60e25 ("libata: implement cross-port EH exclusion")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ata/libata-core.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e76d15411e2a..3d25dec6ac6c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1540,6 +1540,7 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
>  {
>  	struct ata_link *link = dev->link;
>  	struct ata_port *ap = link->ap;
> +	const bool owns_eh_mutex = ap->host->eh_owner == current;

We have similar code in ata_msleep(), which does:

bool owns_eh = ap && ap->host->eh_owner == current;

do we perhaps want the "ap && " here as well?


>  	u8 command = tf->command;
>  	struct ata_queued_cmd *qc;
>  	struct scatterlist sgl;
> @@ -1617,11 +1618,25 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
>  		}
>  	}
>  
> -	ata_eh_release(ap);
> +	if (owns_eh_mutex) {
> +		/*
> +		 * To prevent that the compiler complains about the
> +		 * ata_eh_release() call below.
> +		 */
> +		__acquire(&ap->host->eh_mutex);
> +		ata_eh_release(ap);
> +	}
>  
>  	rc = wait_for_completion_timeout(&wait, msecs_to_jiffies(timeout));
>  
> -	ata_eh_acquire(ap);
> +	if (owns_eh_mutex) {
> +		ata_eh_acquire(ap);
> +		/*
> +		 * To prevent that the compiler complains about the above
> +		 * ata_eh_acquire() call.
> +		 */
> +		__release(&ap->host->eh_mutex);
> +	}
>  
>  	ata_sff_flush_pio_task(ap);
>  

Otherwise, this looks good to me:
Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH v2 1/9] ata: libata: Fix ata_exec_internal()

[Bart Van Assche]

On 5/26/26 6:43 AM, Niklas Cassel wrote:
> On Thu, May 21, 2026 at 10:33:29AM -0700, Bart Van Assche wrote:
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index e76d15411e2a..3d25dec6ac6c 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -1540,6 +1540,7 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
>>   {
>>   	struct ata_link *link = dev->link;
>>   	struct ata_port *ap = link->ap;
>> +	const bool owns_eh_mutex = ap->host->eh_owner == current;
> 
> We have similar code in ata_msleep(), which does:
> 
> bool owns_eh = ap && ap->host->eh_owner == current;
> 
> do we perhaps want the "ap && " here as well?

I don't think we need this since ata_exec_internal() unconditionally
dereferences the "ap" pointer and hence would already trigger a kernel
crash if "ap" would be NULL. I'm referring to the following statement
near the start of ata_exec_internal():

	spin_lock_irqsave(ap->lock, flags);

> Otherwise, this looks good to me:
> Reviewed-by: Niklas Cassel <cassel@kernel.org>

Thanks!

Bart.

Re: [PATCH v2 1/9] ata: libata: Fix ata_exec_internal()

[Niklas Cassel]

On Tue, May 26, 2026 at 08:56:49AM -0700, Bart Van Assche wrote:
> On 5/26/26 6:43 AM, Niklas Cassel wrote:
> > On Thu, May 21, 2026 at 10:33:29AM -0700, Bart Van Assche wrote:
> > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > > index e76d15411e2a..3d25dec6ac6c 100644
> > > --- a/drivers/ata/libata-core.c
> > > +++ b/drivers/ata/libata-core.c
> > > @@ -1540,6 +1540,7 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
> > >   {
> > >   	struct ata_link *link = dev->link;
> > >   	struct ata_port *ap = link->ap;
> > > +	const bool owns_eh_mutex = ap->host->eh_owner == current;
> > 
> > We have similar code in ata_msleep(), which does:
> > 
> > bool owns_eh = ap && ap->host->eh_owner == current;
> > 
> > do we perhaps want the "ap && " here as well?
> 
> I don't think we need this since ata_exec_internal() unconditionally
> dereferences the "ap" pointer and hence would already trigger a kernel
> crash if "ap" would be NULL. I'm referring to the following statement
> near the start of ata_exec_internal():
> 
> 	spin_lock_irqsave(ap->lock, flags);

I see, thank you!

Normally I would queue this up for the current release, but
considering that this has a Fixes-tag that references a commit
from 2010, and considering that Linus has explicitly said that
fixes that are not serious enough should go into linux-next:

https://lore.kernel.org/lkml/CAHk-=wjt1NiKOdyAMz_DT7NmZ++SizPOhRSi492ukdTnpDzHQw@mail.gmail.com/T/#u

I will queue this fix patch up in for-next.


Kind regards,
Niklas

> 
> > Otherwise, this looks good to me:
> > Reviewed-by: Niklas Cassel <cassel@kernel.org>
> 
> Thanks!
> 
> Bart.

Re: [PATCH v2 1/9] ata: libata: Fix ata_exec_internal()

[Damien Le Moal]

On 2026/05/22 2:33, Bart Van Assche wrote:
> Some but not all ata_exec_internal() calls happen from the context of
> the ATA error handler. Commit c0c362b60e25 ("libata: implement cross-port
> EH exclusion") added ata_eh_release() and ata_eh_acquire() calls in
> ata_exec_internal(). Calling these functions is necessary if the caller
> holds the eh_mutex but is not allowed if the caller doesn't hold that
> mutex. Fix this by only calling ata_eh_release() and ata_eh_acquire() if
> the caller holds the eh_mutex. An example of an indirect caller of
> ata_exec_internal() that does not hold the eh_mutex is
> ata_host_register().
> 
> Fixes: c0c362b60e25 ("libata: implement cross-port EH exclusion")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/9] ata: libata: Fix ata_exec_internal()

[Hannes Reinecke]

On 5/21/26 19:33, Bart Van Assche wrote:
> Some but not all ata_exec_internal() calls happen from the context of
> the ATA error handler. Commit c0c362b60e25 ("libata: implement cross-port
> EH exclusion") added ata_eh_release() and ata_eh_acquire() calls in
> ata_exec_internal(). Calling these functions is necessary if the caller
> holds the eh_mutex but is not allowed if the caller doesn't hold that
> mutex. Fix this by only calling ata_eh_release() and ata_eh_acquire() if
> the caller holds the eh_mutex. An example of an indirect caller of
> ata_exec_internal() that does not hold the eh_mutex is
> ata_host_register().
> 
> Fixes: c0c362b60e25 ("libata: implement cross-port EH exclusion")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ata/libata-core.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Bart Van Assche]

Prepare for adding lock context annotations that refer to the ATA port
argument (ap). No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/libata-sata.c | 2 +-
 drivers/ata/libata-scsi.c | 6 +++---
 drivers/ata/libata.h      | 3 ++-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 2ee54d60ea4b..5e5be6bbf32a 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1379,7 +1379,7 @@ EXPORT_SYMBOL_GPL(ata_sas_sdev_configure);
 int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
 {
 	if (likely(ata_dev_enabled(ap->link.device)))
-		return __ata_scsi_queuecmd(cmd, ap->link.device);
+		return __ata_scsi_queuecmd(cmd, ap->link.device, ap);
 
 	cmd->result = (DID_BAD_TARGET << 16);
 	scsi_done(cmd);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 6733f2b14521..55380a9d873a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4506,9 +4506,9 @@ static void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 }
 
 enum scsi_qc_status __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
-					struct ata_device *dev)
+					struct ata_device *dev,
+					struct ata_port *ap)
 {
-	struct ata_port *ap = dev->link->ap;
 	u8 scsi_op = scmd->cmnd[0];
 	ata_xlat_func_t xlat_func;
 
@@ -4595,7 +4595,7 @@ enum scsi_qc_status ata_scsi_queuecmd(struct Scsi_Host *shost,
 
 	dev = ata_scsi_find_dev(ap, scsidev);
 	if (likely(dev))
-		rc = __ata_scsi_queuecmd(cmd, dev);
+		rc = __ata_scsi_queuecmd(cmd, dev, ap);
 	else {
 		cmd->result = (DID_BAD_TARGET << 16);
 		scsi_done(cmd);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index b5423b6e97de..3940dd33efb9 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -166,7 +166,8 @@ void ata_scsi_sdev_config(struct scsi_device *sdev);
 int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
 		struct ata_device *dev);
 enum scsi_qc_status __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
-					struct ata_device *dev);
+					struct ata_device *dev,
+					struct ata_port *ap);
 void ata_scsi_deferred_qc_work(struct work_struct *work);
 void ata_scsi_requeue_deferred_qc(struct ata_port *ap);
 

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

[Niklas Cassel]

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() ?

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.


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.)



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.)


Thank you for spending time on this!


Kind regards,
Niklas

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

[Bart Van Assche]

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.

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

> 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.

> 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.

> 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?

Thanks,

Bart.

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

[Niklas Cassel]

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

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

[Damien Le Moal]

On 2026/05/22 2:33, Bart Van Assche wrote:
> Prepare for adding lock context annotations that refer to the ATA port
> argument (ap). No functionality has been changed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

I do not have a strong objection to this patch, but its justification without
the annotation change together with it is weak. This change is introduced
because you want to add a __must_hold(ap->lock) annotation. But why can't we
have the annotation be
__must_hold(dev->link->ap->lock) ?
Or a simple lockedp_assert_held(dev->link->ap->lock) ?

lockedp_assert_held() is for runtime. I am not sure if this includes a
__must_hold() call for compile time checks, but if not, what about defining a
lockdep_must_hold() function that does ? That way, we can have both runtime and
compile time checks.

And a similar comment applies to patches 3 to 7.

So in the end, I am not sure this is needed. See also my comment on patch 9.

-- 
Damien Le Moal
Western Digital Research

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

[Bart Van Assche]

On 5/27/26 11:43 AM, Damien Le Moal wrote:
> On 2026/05/22 2:33, Bart Van Assche wrote:
>> Prepare for adding lock context annotations that refer to the ATA port
>> argument (ap). No functionality has been changed.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 
> I do not have a strong objection to this patch, but its justification without
> the annotation change together with it is weak. This change is introduced
> because you want to add a __must_hold(ap->lock) annotation. But why can't we
> have the annotation be
> __must_hold(dev->link->ap->lock) ?
> Or a simple lockedp_assert_held(dev->link->ap->lock) ?
> 
> lockedp_assert_held() is for runtime. I am not sure if this includes a
> __must_hold() call for compile time checks, but if not, what about defining a
> lockdep_must_hold() function that does ? That way, we can have both runtime and
> compile time checks.
> 
> And a similar comment applies to patches 3 to 7.
> 
> So in the end, I am not sure this is needed. See also my comment on patch 9.

Hi Damien,

For every patch that is left out that adds the ATA port argument to a
function, one or more lockdep_assert_held() statements will have to be 
added. If nobody objects I will leave out the patches that add the ATA
port argument to several functions.

Bart.

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

[Damien Le Moal]

On 2026/05/28 3:55, Bart Van Assche wrote:
> On 5/27/26 11:43 AM, Damien Le Moal wrote:
>> On 2026/05/22 2:33, Bart Van Assche wrote:
>>> Prepare for adding lock context annotations that refer to the ATA port
>>> argument (ap). No functionality has been changed.
>>>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>
>> I do not have a strong objection to this patch, but its justification without
>> the annotation change together with it is weak. This change is introduced
>> because you want to add a __must_hold(ap->lock) annotation. But why can't we
>> have the annotation be
>> __must_hold(dev->link->ap->lock) ?
>> Or a simple lockedp_assert_held(dev->link->ap->lock) ?
>>
>> lockedp_assert_held() is for runtime. I am not sure if this includes a
>> __must_hold() call for compile time checks, but if not, what about defining a
>> lockdep_must_hold() function that does ? That way, we can have both runtime and
>> compile time checks.
>>
>> And a similar comment applies to patches 3 to 7.
>>
>> So in the end, I am not sure this is needed. See also my comment on patch 9.
> 
> Hi Damien,
> 
> For every patch that is left out that adds the ATA port argument to a
> function, one or more lockdep_assert_held() statements will have to be 
> added. If nobody objects I will leave out the patches that add the ATA
> port argument to several functions.

Yes, that will adds runtime checks in addition to the compile time checks. So
that would be nicer.



-- 
Damien Le Moal
Western Digital Research

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

[Hannes Reinecke]

On 5/21/26 19:33, Bart Van Assche wrote:
> Prepare for adding lock context annotations that refer to the ATA port
> argument (ap). No functionality has been changed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ata/libata-sata.c | 2 +-
>   drivers/ata/libata-scsi.c | 6 +++---
>   drivers/ata/libata.h      | 3 ++-
>   3 files changed, 6 insertions(+), 5 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 3/9] ata: libata: Pass the ATA port argument directly to ata_qc_schedule_eh()

[Bart Van Assche]

Prepare for adding lock context annotations that refer to the ATA port
argument (ap). No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/libata-core.c | 4 ++--
 drivers/ata/libata-eh.c   | 4 +---
 drivers/ata/libata.h      | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3d25dec6ac6c..5caae9a2aca3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5059,7 +5059,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 	if (unlikely(qc->flags & ATA_QCFLAG_EH)) {
 		fill_result_tf(qc);
 		trace_ata_qc_complete_failed(qc);
-		ata_qc_schedule_eh(qc);
+		ata_qc_schedule_eh(ap, qc);
 		return;
 	}
 
@@ -5092,7 +5092,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 		 * fast drain, and freeze the port.
 		 */
 		ap->pflags |= ATA_PFLAG_EH_PENDING;
-		ata_qc_schedule_eh(qc);
+		ata_qc_schedule_eh(ap, qc);
 		return;
 	}
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 6cb79a09423d..7826162bb8db 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -976,10 +976,8 @@ static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain)
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  */
-void ata_qc_schedule_eh(struct ata_queued_cmd *qc)
+void ata_qc_schedule_eh(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
-	struct ata_port *ap = qc->ap;
-
 	qc->flags |= ATA_QCFLAG_EH;
 	ata_eh_set_pending(ap, true);
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 3940dd33efb9..ca04427eb4ff 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -178,7 +178,7 @@ extern void ata_eh_acquire(struct ata_port *ap);
 extern void ata_eh_release(struct ata_port *ap);
 extern void ata_scsi_error(struct Scsi_Host *host);
 extern void ata_eh_fastdrain_timerfn(struct timer_list *t);
-extern void ata_qc_schedule_eh(struct ata_queued_cmd *qc);
+extern void ata_qc_schedule_eh(struct ata_port *ap, struct ata_queued_cmd *qc);
 extern void ata_dev_disable(struct ata_device *dev);
 extern void ata_eh_detach_dev(struct ata_device *dev);
 extern void ata_eh_about_to_do(struct ata_link *link, struct ata_device *dev,

[PATCH v2 4/9] ata: libata: Pass the ATA port argument directly to ata_qc_complete()

[Bart Van Assche]

Prepare for adding lock context annotations that refer to the ATA port
argument (ap). No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/libata-core.c     | 5 ++---
 drivers/ata/libata-eh.c       | 2 +-
 drivers/ata/libata-sata.c     | 2 +-
 drivers/ata/libata-sff.c      | 4 ++--
 drivers/ata/pdc_adma.c        | 4 ++--
 drivers/ata/sata_dwc_460ex.c  | 2 +-
 drivers/ata/sata_fsl.c        | 2 +-
 drivers/ata/sata_inic162x.c   | 2 +-
 drivers/ata/sata_promise.c    | 2 +-
 drivers/ata/sata_qstor.c      | 5 +++--
 drivers/ata/sata_sx4.c        | 6 +++---
 drivers/scsi/libsas/sas_ata.c | 2 +-
 include/linux/libata.h        | 2 +-
 13 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5caae9a2aca3..307ab48fe04c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5024,9 +5024,8 @@ static void ata_verify_xfer(struct ata_queued_cmd *qc)
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  */
-void ata_qc_complete(struct ata_queued_cmd *qc)
+void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
-	struct ata_port *ap = qc->ap;
 	struct ata_device *dev = qc->dev;
 	struct ata_eh_info *ehi = &dev->link->eh_info;
 
@@ -5232,7 +5231,7 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 sys_err:
 	qc->err_mask |= AC_ERR_SYSTEM;
 err:
-	ata_qc_complete(qc);
+	ata_qc_complete(ap, qc);
 }
 
 /**
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 7826162bb8db..ed724bd45529 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1058,7 +1058,7 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
 	ata_qc_for_each_with_internal(ap, qc, tag) {
 		if (qc && (!link || qc->dev->link == link)) {
 			qc->flags |= ATA_QCFLAG_EH;
-			ata_qc_complete(qc);
+			ata_qc_complete(ap, qc);
 			nr_aborted++;
 		}
 	}
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 5e5be6bbf32a..0428fe022f3f 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -786,7 +786,7 @@ int ata_qc_complete_multiple(struct ata_port *ap, u64 qc_active)
 
 		qc = ata_qc_from_tag(ap, tag);
 		if (qc) {
-			ata_qc_complete(qc);
+			ata_qc_complete(ap, qc);
 			nr_done++;
 		}
 		done_mask &= ~(1ULL << tag);
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 785b6e371abf..38226ed51bb5 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -890,13 +890,13 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
 		if (qc) {
 			if (likely(!(qc->err_mask & AC_ERR_HSM))) {
 				ata_sff_irq_on(ap);
-				ata_qc_complete(qc);
+				ata_qc_complete(ap, qc);
 			} else
 				ata_port_freeze(ap);
 		}
 	} else {
 		if (likely(!(qc->err_mask & AC_ERR_HSM)))
-			ata_qc_complete(qc);
+			ata_qc_complete(ap, qc);
 		else
 			ata_port_freeze(ap);
 	}
diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
index 17a5a59861c3..b8f7633537c1 100644
--- a/drivers/ata/pdc_adma.c
+++ b/drivers/ata/pdc_adma.c
@@ -415,7 +415,7 @@ static inline unsigned int adma_intr_pkt(struct ata_host *host)
 				qc->err_mask |= AC_ERR_OTHER;
 
 			if (!qc->err_mask)
-				ata_qc_complete(qc);
+				ata_qc_complete(ap, qc);
 			else {
 				struct ata_eh_info *ehi = &ap->link.eh_info;
 				ata_ehi_clear_desc(ehi);
@@ -457,7 +457,7 @@ static inline unsigned int adma_intr_mmio(struct ata_host *host)
 			pp->state = adma_state_idle;
 			qc->err_mask |= ac_err_mask(status);
 			if (!qc->err_mask)
-				ata_qc_complete(qc);
+				ata_qc_complete(ap, qc);
 			else {
 				struct ata_eh_info *ehi = &ap->link.eh_info;
 				ata_ehi_clear_desc(ehi);
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 64cb544903d8..169cfd85f0e6 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -758,7 +758,7 @@ static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
 	mask = (~(qcmd_tag_to_mask(tag)));
 	hsdev->sactive_queued = hsdev->sactive_queued & mask;
 	hsdev->sactive_issued = hsdev->sactive_issued & mask;
-	ata_qc_complete(qc);
+	ata_qc_complete(ap, qc);
 	return 0;
 }
 
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index be829fcc584d..ce6f50b8e38c 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1264,7 +1264,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 			 ioread32(hcr_base + CC));
 
 		if (qc) {
-			ata_qc_complete(qc);
+			ata_qc_complete(ap, qc);
 		}
 	} else {
 		/* Spurious Interrupt!! */
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index 46a8c20daf18..ccfb863f018f 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -407,7 +407,7 @@ static void inic_host_intr(struct ata_port *ap)
 			     (ATA_DF | ATA_ERR)))
 			qc->err_mask |= AC_ERR_DEV;
 
-		ata_qc_complete(qc);
+		ata_qc_complete(ap, qc);
 		return;
 	}
 
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index 2a005aede123..18d1eda69a4a 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -890,7 +890,7 @@ static unsigned int pdc_host_intr(struct ata_port *ap,
 	case ATAPI_PROT_DMA:
 	case ATAPI_PROT_NODATA:
 		qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
-		ata_qc_complete(qc);
+		ata_qc_complete(ap, qc);
 		handled = 1;
 		break;
 	default:
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index cfb9b5b61cd7..9967a5b62643 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -329,12 +329,13 @@ static unsigned int qs_qc_issue(struct ata_queued_cmd *qc)
 
 static void qs_do_or_die(struct ata_queued_cmd *qc, u8 status)
 {
+	struct ata_port *ap = qc->ap;
+
 	qc->err_mask |= ac_err_mask(status);
 
 	if (!qc->err_mask) {
-		ata_qc_complete(qc);
+		ata_qc_complete(ap, qc);
 	} else {
-		struct ata_port    *ap  = qc->ap;
 		struct ata_eh_info *ehi = &ap->link.eh_info;
 
 		ata_ehi_clear_desc(ehi);
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index 0986ebd1eb4e..607e8c34b813 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -672,7 +672,7 @@ static inline unsigned int pdc20621_host_intr(struct ata_port *ap,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
 			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
-			ata_qc_complete(qc);
+			ata_qc_complete(ap, qc);
 			pdc20621_pop_hdma(qc);
 		}
 
@@ -711,7 +711,7 @@ static inline unsigned int pdc20621_host_intr(struct ata_port *ap,
 				readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT));
 			/* get drive status; clear intr; complete txn */
 			qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
-			ata_qc_complete(qc);
+			ata_qc_complete(ap, qc);
 			pdc20621_pop_hdma(qc);
 		}
 		handled = 1;
@@ -722,7 +722,7 @@ static inline unsigned int pdc20621_host_intr(struct ata_port *ap,
 		status = ata_sff_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 		ata_port_dbg(ap, "BUS_NODATA (drv_stat 0x%X)\n", status);
 		qc->err_mask |= ac_err_mask(status);
-		ata_qc_complete(qc);
+		ata_qc_complete(ap, qc);
 		handled = 1;
 
 	} else {
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 61368e55bf86..eb69d2c7f4b6 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -145,7 +145,7 @@ static void sas_ata_task_done(struct sas_task *task)
 	}
 
 	qc->lldd_task = NULL;
-	ata_qc_complete(qc);
+	ata_qc_complete(ap, qc);
 	spin_unlock_irqrestore(ap->lock, flags);
 
 qc_already_gone:
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5c085ef4eda7..2cfd8f79ac2b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1203,7 +1203,7 @@ extern void ata_id_c_string(const u16 *id, unsigned char *s,
 			    unsigned int ofs, unsigned int len);
 extern unsigned int ata_do_dev_read_id(struct ata_device *dev,
 				       struct ata_taskfile *tf, __le16 *id);
-extern void ata_qc_complete(struct ata_queued_cmd *qc);
+extern void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc);
 extern u64 ata_qc_get_active(struct ata_port *ap);
 extern int ata_std_bios_param(struct scsi_device *sdev,
 			      struct gendisk *unused,

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The kernel-doc comment for ata_qc_complete() is missing the new '@ap' parameter.

Pre-existing issues:
- [High] In `sata_fsl_host_intr`, a spurious interrupt will prematurely complete an active internal command.
--

commit 0bfdcf1e846fdd4f2753f05417c311388a3aa360
Author: Bart Van Assche <bvanassche@acm.org>
ata: libata: Pass the ATA port argument directly to ata_qc_complete()

This commit prepares for adding lock context annotations by passing the ATA
port argument directly to ata_qc_complete(). No functional changes were made.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 5caae9a2aca39..307ab48fe04c2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5024,9 +5024,8 @@ static void ata_verify_xfer(struct ata_queued_cmd *qc)
>   *	LOCKING:
>   *	spin_lock_irqsave(host lock)
>   */
> -void ata_qc_complete(struct ata_queued_cmd *qc)
> +void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
>  {

[Severity: Low]
Is the kernel-doc block missing the new @ap parameter? Since this is an
exported symbol, omitting the new parameter might trigger kernel-doc
warnings during documentation builds.

> -	struct ata_port *ap = qc->ap;
>  	struct ata_device *dev = qc->dev;
>  	struct ata_eh_info *ehi = &dev->link->eh_info;

[ ... ]

> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index be829fcc584de..ce6f50b8e38c1 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1264,7 +1264,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>  			 ioread32(hcr_base + CC));
>  
>  		if (qc) {
> -			ata_qc_complete(qc);
> +			ata_qc_complete(ap, qc);
>  		}

[Severity: High]
This is a pre-existing issue, but does this code prematurely complete an
active internal command on a spurious interrupt?

When an internal command is active, ap->qc_active has bit 32 set
(ATA_TAG_INTERNAL). The done_mask is a 32-bit value read from the CC
register, so it cannot have bit 32 set. Consequently,
done_mask & ap->qc_active always evaluates to 0.

The driver then falls back to the else if block this hunk modifies, which
blindly assumes the internal command completed without verifying that the
completion bit in CC is actually set. If a non-error or spurious interrupt
arrives while an internal command is in flight, it appears the driver
will incorrectly complete the command before the hardware finishes processing
it.

>  	} else {
>  		/* Spurious Interrupt!! */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521173347.2079560-1-bvanassche@acm.org?part=4

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

[Bart Van Assche]

On 5/21/26 11:40 AM, sashiko-bot@kernel.org wrote:
> [Severity: Low]
> Is the kernel-doc block missing the new @ap parameter? Since this is an
> exported symbol, omitting the new parameter might trigger kernel-doc
> warnings during documentation builds.

Yes, the kernel-doc header needs to be updated in this patch and also in
several other patches in this series.

> [Severity: High]
> This is a pre-existing issue, but does this code prematurely complete an
> active internal command on a spurious interrupt?

Since this is a pre-existing issue not related to context analysis, it
falls outside of the scope of this patch series.

Bart.

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

[Niklas Cassel]

Hello Bart,

On Thu, May 21, 2026 at 01:30:20PM -0700, Bart Van Assche wrote:
> On 5/21/26 11:40 AM, sashiko-bot@kernel.org wrote:
> > [Severity: Low]
> > Is the kernel-doc block missing the new @ap parameter? Since this is an
> > exported symbol, omitting the new parameter might trigger kernel-doc
> > warnings during documentation builds.
> 
> Yes, the kernel-doc header needs to be updated in this patch and also in
> several other patches in this series.

I agree.


> 
> > [Severity: High]
> > This is a pre-existing issue, but does this code prematurely complete an
> > active internal command on a spurious interrupt?
> 
> Since this is a pre-existing issue not related to context analysis, it
> falls outside of the scope of this patch series.

I agree.


Kind regards,
Niklas

[PATCH v2 5/9] ata: libata: Pass the ATA port argument directly to ata_qc_issue()

[Bart Van Assche]

Prepare for adding lock context annotations that refer to the ATA port
argument (ap). No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/libata-core.c | 5 ++---
 drivers/ata/libata-scsi.c | 4 ++--
 drivers/ata/libata.h      | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 307ab48fe04c..5f0c2ee8846b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1605,7 +1605,7 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
 	qc->private_data = &wait;
 	qc->complete_fn = ata_qc_complete_internal;
 
-	ata_qc_issue(qc);
+	ata_qc_issue(ap, qc);
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
@@ -5159,9 +5159,8 @@ EXPORT_SYMBOL_GPL(ata_qc_get_active);
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  */
-void ata_qc_issue(struct ata_queued_cmd *qc)
+void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
-	struct ata_port *ap = qc->ap;
 	struct ata_link *link = qc->dev->link;
 	u8 prot = qc->tf.protocol;
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 55380a9d873a..352b9b9e9b6b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1680,7 +1680,7 @@ void ata_scsi_deferred_qc_work(struct work_struct *work)
 	if (qc && !ata_port_eh_scheduled(ap)) {
 		WARN_ON_ONCE(ap->ops->qc_defer(qc));
 		ap->deferred_qc = NULL;
-		ata_qc_issue(qc);
+		ata_qc_issue(ap, qc);
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
@@ -1815,7 +1815,7 @@ static int ata_scsi_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
 	}
 
 issue:
-	ata_qc_issue(qc);
+	ata_qc_issue(ap, qc);
 
 	return 0;
 }
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index ca04427eb4ff..5a8ee6f262de 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -88,7 +88,7 @@ extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern unsigned int ata_dev_set_feature(struct ata_device *dev,
 					u8 subcmd, u8 action);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
-extern void ata_qc_issue(struct ata_queued_cmd *qc);
+extern void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
 extern int atapi_check_dma(struct ata_queued_cmd *qc);
 extern void swap_buf_le16(u16 *buf, unsigned int buf_words);

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing kernel-doc parameter documentation for 'ap'.
--

commit b81f096dbed2ff9052d6614eac11c53f07517937
Author: Bart Van Assche <bvanassche@acm.org>

ata: libata: Pass the ATA port argument directly to ata_qc_issue()

This commit prepares for adding lock context annotations by passing the ATA
port argument directly to ata_qc_issue(). No functionality has been changed.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
[ ... ]
> @@ -5159,9 +5159,8 @@ EXPORT_SYMBOL_GPL(ata_qc_get_active);
>   *	LOCKING:
>   *	spin_lock_irqsave(host lock)
>   */
> -void ata_qc_issue(struct ata_queued_cmd *qc)
> +void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)

[Severity: Low]
This isn't a bug, but should the kernel-doc comment block above this
function be updated to document the new ap parameter? It looks like this
might trigger a warning during documentation builds.

>  {
> -	struct ata_port *ap = qc->ap;
>  	struct ata_link *link = qc->dev->link;
>  	u8 prot = qc->tf.protocol;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521173347.2079560-1-bvanassche@acm.org?part=5

[PATCH v2 6/9] ata: libata: Pass the ATA port argument directly to __ata_qc_complete()

[Bart Van Assche]

Prepare for adding lock context annotations that refer to the ATA port
argument (ap). No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/libata-core.c | 8 +++-----
 drivers/ata/libata-eh.c   | 2 +-
 drivers/ata/libata.h      | 2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5f0c2ee8846b..96f7706d6eb8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4939,15 +4939,13 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 		qc->tag = ATA_TAG_POISON;
 }
 
-void __ata_qc_complete(struct ata_queued_cmd *qc)
+void __ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
-	struct ata_port *ap;
 	struct ata_link *link;
 
 	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_ACTIVE)))
 		return;
 
-	ap = qc->ap;
 	link = qc->dev->link;
 
 	if (likely(qc->flags & ATA_QCFLAG_DMAMAP))
@@ -5050,7 +5048,7 @@ void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
 	if (unlikely(ata_tag_internal(qc->tag))) {
 		fill_result_tf(qc);
 		trace_ata_qc_complete_internal(qc);
-		__ata_qc_complete(qc);
+		__ata_qc_complete(ap, qc);
 		return;
 	}
 
@@ -5119,7 +5117,7 @@ void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
 	if (unlikely(dev->flags & ATA_DFLAG_DUBIOUS_XFER))
 		ata_verify_xfer(qc);
 
-	__ata_qc_complete(qc);
+	__ata_qc_complete(ap, qc);
 }
 EXPORT_SYMBOL_GPL(ata_qc_complete);
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index ed724bd45529..b9f1af9e1955 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1212,7 +1212,7 @@ static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
 
 	spin_lock_irqsave(ap->lock, flags);
 	qc->scsidone = ata_eh_scsidone;
-	__ata_qc_complete(qc);
+	__ata_qc_complete(ap, qc);
 	WARN_ON(ata_tag_valid(qc->tag));
 	spin_unlock_irqrestore(ap->lock, flags);
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5a8ee6f262de..5cdcb71f47b3 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -89,7 +89,7 @@ extern unsigned int ata_dev_set_feature(struct ata_device *dev,
 					u8 subcmd, u8 action);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc);
-extern void __ata_qc_complete(struct ata_queued_cmd *qc);
+extern void __ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc);
 extern int atapi_check_dma(struct ata_queued_cmd *qc);
 extern void swap_buf_le16(u16 *buf, unsigned int buf_words);
 extern bool ata_phys_link_online(struct ata_link *link);

[PATCH v2 7/9] ata: libata: Pass the ATA port argument directly to ata_link_abort()

[Bart Van Assche]

Prepare for adding lock context annotations that refer to the ATA port
argument (ap). No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/libahci.c         | 2 +-
 drivers/ata/libata-core.c     | 2 +-
 drivers/ata/libata-eh.c       | 4 ++--
 drivers/ata/sata_fsl.c        | 2 +-
 drivers/ata/sata_mv.c         | 4 ++--
 drivers/ata/sata_sil24.c      | 2 +-
 drivers/scsi/libsas/sas_ata.c | 2 +-
 include/linux/libata.h        | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 0c5e5b29bce4..2f685ab5ba78 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1853,7 +1853,7 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 	if (irq_stat & PORT_IRQ_FREEZE)
 		ata_port_freeze(ap);
 	else if (fbs_need_dec) {
-		ata_link_abort(link);
+		ata_link_abort(ap, link);
 		ahci_fbs_dec_intr(ap);
 	} else
 		ata_port_abort(ap);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 96f7706d6eb8..f96e077514f9 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5208,7 +5208,7 @@ void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
 	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
 		link->eh_info.action |= ATA_EH_RESET;
 		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
-		ata_link_abort(link);
+		ata_link_abort(ap, link);
 		return;
 	}
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b9f1af9e1955..66ea590fad58 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1081,9 +1081,9 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
  *	RETURNS:
  *	Number of aborted qc's.
  */
-int ata_link_abort(struct ata_link *link)
+int ata_link_abort(struct ata_port *ap, struct ata_link *link)
 {
-	return ata_do_link_abort(link->ap, link);
+	return ata_do_link_abort(ap, link);
 }
 EXPORT_SYMBOL_GPL(ata_link_abort);
 
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index ce6f50b8e38c..fc17e0c3e2f9 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1170,7 +1170,7 @@ static void sata_fsl_error_intr(struct ata_port *ap)
 		ata_port_freeze(ap);
 	else if (abort) {
 		if (qc)
-			ata_link_abort(qc->dev->link);
+			ata_link_abort(ap, qc->dev->link);
 		else
 			ata_port_abort(ap);
 	}
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index ffb396f61731..84c1abd4d0ac 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2449,7 +2449,7 @@ static void mv_pmp_eh_prep(struct ata_port *ap, unsigned int pmp_map)
 			ata_ehi_push_desc(ehi, "dev err");
 			ehi->err_mask |= AC_ERR_DEV;
 			ehi->action |= ATA_EH_RESET;
-			ata_link_abort(link);
+			ata_link_abort(ap, link);
 		}
 	}
 }
@@ -2728,7 +2728,7 @@ static void mv_err_intr(struct ata_port *ap)
 
 	if (abort) {
 		if (qc)
-			ata_link_abort(qc->dev->link);
+			ata_link_abort(ap, qc->dev->link);
 		else
 			ata_port_abort(ap);
 	}
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index d642ece9f07a..ab27559667b6 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -1086,7 +1086,7 @@ static void sil24_error_intr(struct ata_port *ap)
 		ata_port_freeze(ap);
 	else if (abort) {
 		if (qc)
-			ata_link_abort(qc->dev->link);
+			ata_link_abort(ap, qc->dev->link);
 		else
 			ata_port_abort(ap);
 	}
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index eb69d2c7f4b6..d6823d8ac876 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -943,7 +943,7 @@ void sas_ata_device_link_abort(struct domain_device *device, bool force_reset)
 	link->eh_info.err_mask |= AC_ERR_DEV;
 	if (force_reset)
 		link->eh_info.action |= ATA_EH_RESET;
-	ata_link_abort(link);
+	ata_link_abort(ap, link);
 	spin_unlock_irqrestore(ap->lock, flags);
 }
 EXPORT_SYMBOL_GPL(sas_ata_device_link_abort);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2cfd8f79ac2b..e953a9c8e660 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1408,7 +1408,7 @@ static inline int ata_acpi_cbl_pata_type(struct ata_port *ap)
  */
 extern void ata_port_schedule_eh(struct ata_port *ap);
 extern void ata_port_wait_eh(struct ata_port *ap);
-extern int ata_link_abort(struct ata_link *link);
+extern int ata_link_abort(struct ata_port *ap, struct ata_link *link);
 extern int ata_port_abort(struct ata_port *ap);
 extern int ata_port_freeze(struct ata_port *ap);
 

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing kernel-doc parameter documentation for the newly added 'ap' argument in ata_link_abort().
--

commit f5db400cd9dab3479ff54c3d92701ede91c3836c
Author: Bart Van Assche <bvanassche@acm.org>

ata: libata: Pass the ATA port argument directly to ata_link_abort()

This patch modifies ata_link_abort() to take the ATA port as a direct
argument, preparing for future lock context annotations. No functional
changes are introduced.

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index fb790e8b6b57b..7528831bb59a4 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
[ ... ]
> @@ -1081,9 +1081,9 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
>   *	RETURNS:
>   *	Number of aborted qc's.
>   */
> -int ata_link_abort(struct ata_link *link)
> +int ata_link_abort(struct ata_port *ap, struct ata_link *link)

[Severity: Low]
Does this kernel-doc block need an update to document the new ap parameter?
Adding the parameter to the function signature without adding it to the
documentation will trigger a kernel-doc warning.

>  {
> -	return ata_do_link_abort(link->ap, link);
> +	return ata_do_link_abort(ap, link);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521173347.2079560-1-bvanassche@acm.org?part=7

[PATCH v2 8/9] ata: libata: Enable context analysis

[Bart Van Assche]

Enable Clang's context analysis. This will cause the build to fail if
e.g. a locking bug would be introduced in an error path. This patch
should not affect the generated assembler code.

Note: although the Linux kernel documentation specifies 22 as minimal
version for Clang for context analysis support, annotating function
pointers is a Clang 23 feature. As one can see here, a patch has been
queued that fixes the kernel documentation:
https://lore.kernel.org/all/177926568868.711.3058599932884307249.tip-bot2@tip-bot2/

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/Makefile         |  2 ++
 drivers/ata/ahci.h           |  3 ++-
 drivers/ata/ahci_imx.c       |  1 +
 drivers/ata/libata-core.c    |  1 +
 drivers/ata/libata-eh.c      |  3 +++
 drivers/ata/libata-pmp.c     |  5 +++++
 drivers/ata/libata.h         | 12 ++++++++----
 drivers/ata/pata_arasan_cf.c |  1 +
 drivers/ata/sata_dwc_460ex.c |  1 +
 drivers/ata/sata_fsl.c       |  1 +
 drivers/ata/sata_inic162x.c  |  1 +
 drivers/ata/sata_mv.c        |  1 +
 drivers/ata/sata_nv.c        |  2 ++
 drivers/ata/sata_promise.c   |  1 +
 drivers/ata/sata_qstor.c     |  1 +
 drivers/ata/sata_sil24.c     |  1 +
 drivers/ata/sata_sx4.c       |  1 +
 drivers/ata/sata_via.c       |  1 +
 include/linux/libata.h       | 18 ++++++++++++------
 19 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 20e6645ab737..b96025abd45e 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
+CONTEXT_ANALYSIS := y
+
 obj-$(CONFIG_ATA)		+= libata.o
 
 # non-SFF interface
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 9e8b6319025c..b57cca352faa 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -448,7 +448,8 @@ void ahci_set_em_messages(struct ahci_host_priv *hpriv,
 int ahci_reset_em(struct ata_host *host);
 void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, const struct scsi_host_template *sht);
-void ahci_error_handler(struct ata_port *ap);
+void ahci_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex);
 u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked);
 
 static inline void __iomem *__ahci_port_base(struct ahci_host_priv *hpriv,
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 3d26595524d3..6aaa18e29abc 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -598,6 +598,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
 }
 
 static void ahci_imx_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	u32 reg_val;
 	struct ata_device *dev;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f96e077514f9..8b8081f4dc63 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6828,6 +6828,7 @@ EXPORT_SYMBOL_GPL(ata_ratelimit);
  *	Might sleep.
  */
 void ata_msleep(struct ata_port *ap, unsigned int msecs)
+	__context_unsafe(conditional locking)
 {
 	bool owns_eh = ap && ap->host->eh_owner == current;
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 66ea590fad58..ddea4979e94b 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3877,6 +3877,9 @@ int ata_eh_recover(struct ata_port *ap, struct ata_reset_operations *reset_ops,
 	ata_for_each_link(link, ap, EDGE) {
 		struct ata_eh_context *ehc = &link->eh_context;
 
+		/* Tell the compiler that link->ap == ap. */
+		__assume_ctx_lock(&link->ap->host->eh_mutex);
+
 		if (!(ehc->i.action & ATA_EH_RESET))
 			continue;
 
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index e3adc008fed1..486c554883bf 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -742,6 +742,7 @@ static int sata_pmp_revalidate_quick(struct ata_device *dev)
  */
 static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
 				   struct ata_reset_operations *reset_ops)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ata_link *link = &ap->link;
 	struct ata_eh_context *ehc = &link->eh_context;
@@ -750,6 +751,9 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
 	int detach = 0, rc = 0;
 	int reval_failed = 0;
 
+	/* Tell the compiler that ap->link.ap == ap. */
+	__assume_ctx_lock(&ap->link.ap->host->eh_mutex);
+
 	if (dev->flags & ATA_DFLAG_DETACH) {
 		detach = 1;
 		rc = -ENODEV;
@@ -907,6 +911,7 @@ static int sata_pmp_handle_link_fail(struct ata_link *link, int *link_tries)
  *	0 on success, -errno on failure.
  */
 static int sata_pmp_eh_recover(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ata_port_operations *ops = ap->ops;
 	int pmp_tries, link_tries[SATA_PMP_MAX_PORTS];
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5cdcb71f47b3..70e44e81f244 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -174,8 +174,10 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap);
 /* libata-eh.c */
 extern unsigned int ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd);
 extern void ata_internal_cmd_timed_out(struct ata_device *dev, u8 cmd);
-extern void ata_eh_acquire(struct ata_port *ap);
-extern void ata_eh_release(struct ata_port *ap);
+extern void ata_eh_acquire(struct ata_port *ap)
+	__acquires(ap->host->eh_mutex);
+extern void ata_eh_release(struct ata_port *ap)
+	__releases(ap->host->eh_mutex);
 extern void ata_scsi_error(struct Scsi_Host *host);
 extern void ata_eh_fastdrain_timerfn(struct timer_list *t);
 extern void ata_qc_schedule_eh(struct ata_port *ap, struct ata_queued_cmd *qc);
@@ -189,10 +191,12 @@ extern void ata_eh_autopsy(struct ata_port *ap);
 const char *ata_get_cmd_name(u8 command);
 extern void ata_eh_report(struct ata_port *ap);
 extern int ata_eh_reset(struct ata_link *link, int classify,
-			struct ata_reset_operations *reset_ops);
+			struct ata_reset_operations *reset_ops)
+	__must_hold(&link->ap->host->eh_mutex);
 extern int ata_eh_recover(struct ata_port *ap,
 			  struct ata_reset_operations *reset_ops,
-			  struct ata_link **r_failed_disk);
+			  struct ata_link **r_failed_disk)
+	__must_hold(&ap->host->eh_mutex);
 extern void ata_eh_finish(struct ata_port *ap);
 extern int ata_ering_map(struct ata_ering *ering,
 			 int (*map_fn)(struct ata_ering_entry *, void *),
diff --git a/drivers/ata/pata_arasan_cf.c b/drivers/ata/pata_arasan_cf.c
index a77fefd320c2..e05a4847e037 100644
--- a/drivers/ata/pata_arasan_cf.c
+++ b/drivers/ata/pata_arasan_cf.c
@@ -658,6 +658,7 @@ static void arasan_cf_freeze(struct ata_port *ap)
 }
 
 static void arasan_cf_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct arasan_cf_dev *acdev = ap->host->private_data;
 
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 169cfd85f0e6..8cd65a480010 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1041,6 +1041,7 @@ static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
 }
 
 static void sata_dwc_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	ata_sff_error_handler(ap);
 }
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index fc17e0c3e2f9..d05c9603489e 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1035,6 +1035,7 @@ static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
 }
 
 static void sata_fsl_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	sata_pmp_error_handler(ap);
 }
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index ccfb863f018f..6ea9fceb8261 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -660,6 +660,7 @@ static int inic_hardreset(struct ata_link *link, unsigned int *class,
 }
 
 static void inic_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	void __iomem *port_base = inic_port_base(ap);
 
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 84c1abd4d0ac..ea10c12f449c 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2398,6 +2398,7 @@ static struct ata_queued_cmd *mv_get_active_qc(struct ata_port *ap)
 }
 
 static void mv_pmp_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	unsigned int pmp, pmp_map;
 	struct mv_port_priv *pp = ap->private_data;
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 841e7de2bba6..19b927065868 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -1626,6 +1626,7 @@ static void nv_mcp55_thaw(struct ata_port *ap)
 }
 
 static void nv_adma_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct nv_adma_port_priv *pp = ap->private_data;
 	if (!(pp->flags & NV_ADMA_PORT_REGISTER_MODE)) {
@@ -1795,6 +1796,7 @@ static void nv_swncq_ncq_stop(struct ata_port *ap)
 }
 
 static void nv_swncq_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ata_eh_context *ehc = &ap->link.eh_context;
 
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index 18d1eda69a4a..c980e609b246 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -816,6 +816,7 @@ static int pdc_sata_hardreset(struct ata_link *link, unsigned int *class,
 }
 
 static void pdc_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	if (!ata_port_is_frozen(ap))
 		pdc_reset_port(ap);
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index 9967a5b62643..c682285ea5e5 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -220,6 +220,7 @@ static int qs_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val)
 }
 
 static void qs_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	qs_enter_reg_mode(ap);
 	ata_sff_error_handler(ap);
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index ab27559667b6..9fc7ca6fb7a1 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -1167,6 +1167,7 @@ static irqreturn_t sil24_interrupt(int irq, void *dev_instance)
 }
 
 static void sil24_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct sil24_port_priv *pp = ap->private_data;
 
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index 607e8c34b813..fce5fed2b3be 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -853,6 +853,7 @@ static int pdc_softreset(struct ata_link *link, unsigned int *class,
 }
 
 static void pdc_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	if (!ata_port_is_frozen(ap))
 		pdc_reset_port(ap);
diff --git a/drivers/ata/sata_via.c b/drivers/ata/sata_via.c
index 68e9003ec2d4..b672a1e05867 100644
--- a/drivers/ata/sata_via.c
+++ b/drivers/ata/sata_via.c
@@ -573,6 +573,7 @@ static irqreturn_t vt642x_interrupt(int irq, void *dev_instance)
 }
 
 static void vt6421_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct svia_priv *hpriv = ap->host->private_data;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e953a9c8e660..bf113e66f069 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -983,7 +983,8 @@ struct ata_port_operations {
 	void (*thaw)(struct ata_port *ap);
 	struct ata_reset_operations reset;
 	struct ata_reset_operations pmp_reset;
-	void (*error_handler)(struct ata_port *ap);
+	void (*error_handler)(struct ata_port *ap)
+		__must_hold(&ap->host->eh_mutex);
 	void (*lost_interrupt)(struct ata_port *ap);
 	void (*post_internal_cmd)(struct ata_queued_cmd *qc);
 	void (*sched_eh)(struct ata_port *ap);
@@ -1418,8 +1419,10 @@ extern void ata_eh_thaw_port(struct ata_port *ap);
 extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
 
-extern void ata_std_error_handler(struct ata_port *ap);
-extern void ata_std_sched_eh(struct ata_port *ap);
+extern void ata_std_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex);
+extern void ata_std_sched_eh(struct ata_port *ap)
+	__must_hold(ap->lock);
 extern void ata_std_end_eh(struct ata_port *ap);
 extern int ata_link_nr_enabled(struct ata_link *link);
 
@@ -1998,7 +2001,8 @@ extern void ata_timing_merge(const struct ata_timing *,
 extern const struct ata_port_operations sata_pmp_port_ops;
 
 extern int sata_pmp_qc_defer_cmd_switch(struct ata_queued_cmd *qc);
-extern void sata_pmp_error_handler(struct ata_port *ap);
+extern void sata_pmp_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex);
 
 #else /* CONFIG_SATA_PMP */
 
@@ -2062,7 +2066,8 @@ extern int sata_sff_hardreset(struct ata_link *link, unsigned int *class,
 			       unsigned long deadline);
 extern void ata_sff_postreset(struct ata_link *link, unsigned int *classes);
 extern void ata_sff_drain_fifo(struct ata_queued_cmd *qc);
-extern void ata_sff_error_handler(struct ata_port *ap);
+extern void ata_sff_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex);
 extern void ata_sff_std_ports(struct ata_ioports *ioaddr);
 #ifdef CONFIG_PCI
 extern int ata_pci_sff_init_host(struct ata_host *host);
@@ -2092,7 +2097,8 @@ extern enum ata_completion_errors ata_bmdma_dumb_qc_prep(struct ata_queued_cmd *
 extern unsigned int ata_bmdma_port_intr(struct ata_port *ap,
 				      struct ata_queued_cmd *qc);
 extern irqreturn_t ata_bmdma_interrupt(int irq, void *dev_instance);
-extern void ata_bmdma_error_handler(struct ata_port *ap);
+extern void ata_bmdma_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex);
 extern void ata_bmdma_post_internal_cmd(struct ata_queued_cmd *qc);
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
 extern void ata_bmdma_setup(struct ata_queued_cmd *qc);

Re: [PATCH v2 8/9] ata: libata: Enable context analysis

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The patch introduces incomplete or mismatched thread safety annotations, which will cause compilation failures under Clang Context Analysis.
- [High] Adding `__assume_ctx_lock` inside a loop causes a lockset mismatch compilation error.

Pre-existing issues:
- [High] `ata_port_freeze()` is called locklessly from `mv_pmp_error_handler()`, which can corrupt the port's freeze state and race with other hardware state modifications.
- [High] `__ata_bmdma_stop()` and active command masks are accessed locklessly in the NVIDIA SWNCQ error handler, leading to potential hardware and state corruption.
- [High] Promise and SX4 error handlers perform a lockless TOCTOU check of the port freeze state followed by an unprotected read-modify-write (RMW) on the PDC_CTLSTAT hardware register.
--

commit 9cf4f2610433aecbb931b34a78ec27ff09ed3edb
Author: Bart Van Assche <bvanassche@acm.org>
Subject: ata: libata: Enable context analysis

This patch enables Clang's context analysis in the ATA subsystem to
catch locking bugs in error paths. It adds thread safety annotations
such as __must_hold and __assume_ctx_lock to various functions.

Link: https://lore.kernel.org/all/177926568868.711.3058599932884307249.tip-bot2@tip-bot2/

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 7528831bb59a4..5930735bb901c 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3877,6 +3877,9 @@ int ata_eh_recover(struct ata_port *ap, struct ata_reset_operations *reset_ops,
>  	ata_for_each_link(link, ap, EDGE) {
>  		struct ata_eh_context *ehc = &link->eh_context;
>  
> +		/* Tell the compiler that link->ap == ap. */
> +		__assume_ctx_lock(&link->ap->host->eh_mutex);

[Severity: High]
Will adding this __assume_ctx_lock() inside a loop cause a lockset
mismatch compilation error?

Clang's context analysis expects the lockset at a loop header to match the
lockset at the loop back-edge. Since we are asserting the lock state inside
the loop without releasing it before the iteration ends, could this trigger
a lockset divergence warning that breaks the build?

>  		if (!(ehc->i.action & ATA_EH_RESET))
>  			continue;
>  

[ ... ]

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 84c1abd4d0ac4..ea10c12f449c7 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -2398,6 +2398,7 @@ static struct ata_queued_cmd *mv_get_active_qc(struct ata_port *ap)
>  }
>  
>  static void mv_pmp_error_handler(struct ata_port *ap)
> +	__must_hold(&ap->host->eh_mutex)
>  {
>  	unsigned int pmp, pmp_map;
>  	struct mv_port_priv *pp = ap->private_data;

[Severity: High]
This is a pre-existing issue, but does calling ata_port_freeze() locklessly
from mv_pmp_error_handler() risk corrupting the port's freeze state?

ata_port_freeze() modifies the port's internal state (ap->pflags) and
requires the host spinlock (ap->lock) to be held. Since
mv_pmp_error_handler() runs in the EH thread holding only the sleepable
eh_mutex, this lockless call could allow concurrent modification of
ap->pflags and command structures from interrupt handlers.

Should ata_eh_freeze_port(ap) be used here instead, as it acquires the
spinlock internally?

[ ... ]

> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 841e7de2bba6e..19b9270658686 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -1795,6 +1796,7 @@ static void nv_swncq_ncq_stop(struct ata_port *ap)
>  }
>  
>  static void nv_swncq_error_handler(struct ata_port *ap)
> +	__must_hold(&ap->host->eh_mutex)
>  {
>  	struct ata_eh_context *ehc = &ap->link.eh_context;
>  

[Severity: High]
This is a pre-existing issue, but are we accessing __ata_bmdma_stop() and
active command masks locklessly here, potentially causing state corruption?

nv_swncq_error_handler() executes holding only eh_mutex. It calls
nv_swncq_ncq_stop(), which modifies hardware registers via
__ata_bmdma_stop(ap) and nv_swncq_irq_clear(ap) without holding ap->lock.

Could this create a data race with nv_swncq_interrupt() running concurrently
on another CPU, which acquires ap->lock to modify DMA and IRQ states?

[ ... ]

> diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
> index 18d1eda69a4a8..c980e609b246e 100644
> --- a/drivers/ata/sata_promise.c
> +++ b/drivers/ata/sata_promise.c
> @@ -816,6 +816,7 @@ static int pdc_sata_hardreset(struct ata_link *link, unsigned int *class,
>  }
>  
>  static void pdc_error_handler(struct ata_port *ap)
> +	__must_hold(&ap->host->eh_mutex)
>  {
>  	if (!ata_port_is_frozen(ap))
>  		pdc_reset_port(ap);

[Severity: High]
This is a pre-existing issue, but does this pattern perform an unprotected
read-modify-write on the PDC_CTLSTAT hardware register?

We locklessly check !ata_port_is_frozen(ap) and then call pdc_reset_port(ap).
Because ap->lock is not held and we explicitly check that the port is not
frozen (meaning interrupts are not masked), an interrupt could fire
concurrently and execute pdc_freeze() or pdc_thaw().

Since those also perform read-modify-writes on PDC_CTLSTAT under ap->lock,
could this race silently overwrite hardware states and unintentionally unmask
interrupts?

[ ... ]

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index e953a9c8e660b..bf113e66f0691 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -983,7 +983,8 @@ struct ata_port_operations {
>  	void (*thaw)(struct ata_port *ap);
>  	struct ata_reset_operations reset;
>  	struct ata_reset_operations pmp_reset;
> -	void (*error_handler)(struct ata_port *ap);
> +	void (*error_handler)(struct ata_port *ap)
> +		__must_hold(&ap->host->eh_mutex);
>  	void (*lost_interrupt)(struct ata_port *ap);
>  	void (*post_internal_cmd)(struct ata_queued_cmd *qc);
>  	void (*sched_eh)(struct ata_port *ap);

[Severity: High]
Does this struct definition miss a __must_hold(ap->lock) annotation for
sched_eh?

The assigned ata_std_sched_eh function includes __must_hold(ap->lock),
but the struct member here does not.

Additionally, will adding __must_hold() to error_handler break the build
for ata_dummy_error_handler and forward declarations (like
sil24_error_handler) if they are not also updated with these annotations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521173347.2079560-1-bvanassche@acm.org?part=8

Re: [PATCH v2 8/9] ata: libata: Enable context analysis

[Bart Van Assche]

On 5/21/26 1:17 PM, sashiko-bot@kernel.org wrote:
> New issues:
> - [High] The patch introduces incomplete or mismatched thread safety annotations, which will cause compilation failures under Clang Context Analysis.

This is on purpose and is fine.

> - [High] Adding `__assume_ctx_lock` inside a loop causes a lockset mismatch compilation error.

No, the __assume_ctx_lock() statements added by this patch do not cause
a compilation error.

Bart.

Re: [PATCH v2 8/9] ata: libata: Enable context analysis

[Niklas Cassel]

On Thu, May 21, 2026 at 10:33:36AM -0700, Bart Van Assche wrote:
> Enable Clang's context analysis. This will cause the build to fail if
> e.g. a locking bug would be introduced in an error path. This patch
> should not affect the generated assembler code.
> 
> Note: although the Linux kernel documentation specifies 22 as minimal
> version for Clang for context analysis support, annotating function
> pointers is a Clang 23 feature. As one can see here, a patch has been
> queued that fixes the kernel documentation:
> https://lore.kernel.org/all/177926568868.711.3058599932884307249.tip-bot2@tip-bot2/
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

(snip)

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 66ea590fad58..ddea4979e94b 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3877,6 +3877,9 @@ int ata_eh_recover(struct ata_port *ap, struct ata_reset_operations *reset_ops,
>  	ata_for_each_link(link, ap, EDGE) {
>  		struct ata_eh_context *ehc = &link->eh_context;
>  
> +		/* Tell the compiler that link->ap == ap. */
> +		__assume_ctx_lock(&link->ap->host->eh_mutex);

This is quite ugly.
Could we remove __assume_ctx_lock() if we supplied ap to eh_reset() ?

[PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Bart Van Assche]

Annotate all functions and also their direct and indirect callers with
__must_hold() that have a lockdep_assert_held() statement in their
function body for the host lock. As one can see in the comments added by
this patch, locking is missing from the following two functions:
 * Some of the ata_port_freeze() callers.
 * nv_do_interrupt().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ata/ahci.h           |  3 ++-
 drivers/ata/ahci_xgene.c     |  1 +
 drivers/ata/libahci.c        |  9 +++++++++
 drivers/ata/libata-core.c    |  7 +++++++
 drivers/ata/libata-eh.c      |  8 ++++++++
 drivers/ata/libata-scsi.c    |  8 ++++++++
 drivers/ata/libata-sff.c     |  8 ++++++++
 drivers/ata/libata.h         | 15 ++++++++++-----
 drivers/ata/pdc_adma.c       | 12 +++++++++++-
 drivers/ata/sata_dwc_460ex.c |  6 ++++++
 drivers/ata/sata_fsl.c       |  7 +++++++
 drivers/ata/sata_inic162x.c  |  7 ++++++-
 drivers/ata/sata_mv.c        | 13 +++++++++++++
 drivers/ata/sata_nv.c        | 16 ++++++++++++++++
 drivers/ata/sata_promise.c   | 13 ++++++++++++-
 drivers/ata/sata_qstor.c     | 14 ++++++++++++++
 drivers/ata/sata_rcar.c      |  5 +++++
 drivers/ata/sata_sil.c       |  4 ++++
 drivers/ata/sata_sil24.c     |  7 +++++++
 drivers/ata/sata_sx4.c       |  4 ++++
 drivers/ata/sata_vsc.c       |  7 +++++++
 include/linux/libata.h       | 32 +++++++++++++++++++++-----------
 22 files changed, 186 insertions(+), 20 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index b57cca352faa..582b7a117ad4 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -450,7 +450,8 @@ void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, const struct scsi_host_template *sht);
 void ahci_error_handler(struct ata_port *ap)
 	__must_hold(&ap->host->eh_mutex);
-u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked);
+u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
+	__must_hold(host->lock);
 
 static inline void __iomem *__ahci_port_base(struct ahci_host_priv *hpriv,
 					     unsigned int port_no)
diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
index 98c99b5a8242..d17d57767fc9 100644
--- a/drivers/ata/ahci_xgene.c
+++ b/drivers/ata/ahci_xgene.c
@@ -555,6 +555,7 @@ static int xgene_ahci_softreset(struct ata_link *link, unsigned int *class,
  */
 static int xgene_ahci_handle_broken_edge_irq(struct ata_host *host,
 					     u32 irq_masked)
+	__must_hold(host->lock)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *port_mmio;
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 2f685ab5ba78..2af53a12634b 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1749,6 +1749,7 @@ static void ahci_fbs_dec_intr(struct ata_port *ap)
 }
 
 static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
+	__must_hold(ap->lock)
 {
 	struct ahci_host_priv *hpriv = ap->host->private_data;
 	struct ahci_port_priv *pp = ap->private_data;
@@ -1778,6 +1779,9 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 	if (!link)
 		link = &ap->link;
 
+	/* Tell the compiler that link->ap == ap. */
+	__assume_ctx_lock(link->ap->lock);
+
 	active_qc = ata_qc_from_tag(ap, link->active_tag);
 	active_ehi = &link->eh_info;
 
@@ -1860,6 +1864,7 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 }
 
 static void ahci_qc_complete(struct ata_port *ap, void __iomem *port_mmio)
+	__must_hold(ap->lock)
 {
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	struct ahci_port_priv *pp = ap->private_data;
@@ -1894,6 +1899,7 @@ static void ahci_qc_complete(struct ata_port *ap, void __iomem *port_mmio)
 
 static void ahci_handle_port_interrupt(struct ata_port *ap,
 				       void __iomem *port_mmio, u32 status)
+	__must_hold(ap->lock)
 {
 	struct ahci_port_priv *pp = ap->private_data;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
@@ -1955,6 +1961,7 @@ static void ahci_handle_port_interrupt(struct ata_port *ap,
 }
 
 static void ahci_port_intr(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 status;
@@ -1993,6 +2000,8 @@ u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
 
 		ap = host->ports[i];
 		if (ap) {
+			/* Tell the compiler that ap->lock == host->lock. */
+			__assume_ctx_lock(ap->lock);
 			ahci_port_intr(ap);
 		} else {
 			if (ata_ratelimit())
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8b8081f4dc63..52bcd8c78774 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5027,6 +5027,9 @@ void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
 	struct ata_device *dev = qc->dev;
 	struct ata_eh_info *ehi = &dev->link->eh_info;
 
+	/* Tell the compiler that qc->dev->link->ap == ap. */
+	__assume_ctx_lock(qc->dev->link->ap->lock);
+
 	/* Trigger the LED (if available) */
 	ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
 
@@ -5162,6 +5165,10 @@ void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
 	struct ata_link *link = qc->dev->link;
 	u8 prot = qc->tf.protocol;
 
+	/* Tell the compiler that qc->ap == qc->dev->link->ap->lock == ap. */
+	__assume_ctx_lock(qc->ap->lock);
+	__assume_ctx_lock(qc->dev->link->ap->lock);
+
 	/*
 	 * Make sure we have a valid tag and that only one non-NCQ command is
 	 * outstanding.
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index ddea4979e94b..8fe29659efe7 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -936,6 +936,7 @@ void ata_eh_fastdrain_timerfn(struct timer_list *t)
  *	spin_lock_irqsave(host lock)
  */
 static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain)
+	__must_hold(ap->lock)
 {
 	unsigned int cnt;
 
@@ -1047,6 +1048,7 @@ void ata_port_schedule_eh(struct ata_port *ap)
 EXPORT_SYMBOL_GPL(ata_port_schedule_eh);
 
 static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
+	__must_hold(ap->lock)
 {
 	struct ata_queued_cmd *qc;
 	int tag, nr_aborted = 0;
@@ -1057,6 +1059,9 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
 	/* include internal tag in iteration */
 	ata_qc_for_each_with_internal(ap, qc, tag) {
 		if (qc && (!link || qc->dev->link == link)) {
+			/* Tell the compiler that link->ap == ap. */
+			__assume_ctx_lock(&link->ap->host->eh_mutex);
+
 			qc->flags |= ATA_QCFLAG_EH;
 			ata_qc_complete(ap, qc);
 			nr_aborted++;
@@ -1149,6 +1154,9 @@ static void __ata_port_freeze(struct ata_port *ap)
  */
 int ata_port_freeze(struct ata_port *ap)
 {
+	/* TO DO: fix all callers of this function that do not hold ap->lock. */
+	__assume_ctx_lock(ap->lock);
+
 	__ata_port_freeze(ap);
 
 	return ata_port_abort(ap);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 352b9b9e9b6b..35575123408b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1705,6 +1705,7 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
 }
 
 static void ata_scsi_schedule_deferred_qc(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct ata_queued_cmd *qc = ap->deferred_qc;
 
@@ -1727,6 +1728,7 @@ static void ata_scsi_schedule_deferred_qc(struct ata_port *ap)
 }
 
 static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
+	__must_hold(qc->ap->lock)
 {
 	struct ata_port *ap = qc->ap;
 	struct scsi_cmnd *cmd = qc->scsicmd;
@@ -1763,6 +1765,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 }
 
 static int ata_scsi_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock)
 {
 	int ret;
 
@@ -1848,6 +1851,7 @@ static int ata_scsi_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
  */
 static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
 			      ata_xlat_func_t xlat_func)
+	__must_hold(dev->link->ap->lock)
 {
 	struct ata_port *ap = dev->link->ap;
 	struct ata_queued_cmd *qc;
@@ -3694,6 +3698,7 @@ static unsigned int ata_scsiop_maint_in(struct ata_device *dev,
  *	What a mess.
  */
 static void ata_scsi_report_zones_complete(struct ata_queued_cmd *qc)
+	__must_hold(qc->ap->lock)
 {
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	struct sg_mapping_iter miter;
@@ -4548,6 +4553,9 @@ enum scsi_qc_status __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
 		xlat_func = ata_get_xlat_func(dev, scsi_op);
 	}
 
+	/* Tell the compiler that dev->link->ap == ap. */
+	__assume_ctx_lock(dev->link->ap->lock);
+
 	if (xlat_func)
 		return ata_scsi_translate(dev, scmd, xlat_func);
 
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 38226ed51bb5..6250ec524cc3 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -881,9 +881,13 @@ static inline int ata_hsm_ok_in_wq(struct ata_port *ap,
  *	Otherwise, none on entry and grabs host lock.
  */
 static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
+	__must_hold(qc->dev->link->ap->lock)
 {
 	struct ata_port *ap = qc->ap;
 
+	/* Tell the compiler that qc->dev->link->ap == ap. */
+	__assume_ctx_lock(ap->lock);
+
 	if (in_wq) {
 		/* EH might have kicked in while host lock is released. */
 		qc = ata_qc_from_tag(ap, qc->tag);
@@ -921,6 +925,9 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 
 	lockdep_assert_held(ap->lock);
 
+	/* Tell the compiler that qc->dev->link->ap == ap. */
+	__assume_ctx_lock(qc->dev->link->ap->lock);
+
 	WARN_ON_ONCE((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
 
 	/* Make sure ata_sff_qc_issue() does not throw things
@@ -1394,6 +1401,7 @@ static unsigned int ata_sff_idle_irq(struct ata_port *ap)
 static unsigned int __ata_sff_port_intr(struct ata_port *ap,
 					struct ata_queued_cmd *qc,
 					bool hsmv_on_idle)
+	__must_hold(ap->lock)
 {
 	u8 status;
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 70e44e81f244..360ba7f7db05 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -88,8 +88,10 @@ extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern unsigned int ata_dev_set_feature(struct ata_device *dev,
 					u8 subcmd, u8 action);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
-extern void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc);
-extern void __ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc);
+extern void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock);
+extern void __ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock);
 extern int atapi_check_dma(struct ata_queued_cmd *qc);
 extern void swap_buf_le16(u16 *buf, unsigned int buf_words);
 extern bool ata_phys_link_online(struct ata_link *link);
@@ -167,9 +169,11 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
 		struct ata_device *dev);
 enum scsi_qc_status __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
 					struct ata_device *dev,
-					struct ata_port *ap);
+					struct ata_port *ap)
+	__must_hold(ap->lock);
 void ata_scsi_deferred_qc_work(struct work_struct *work);
-void ata_scsi_requeue_deferred_qc(struct ata_port *ap);
+void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
+	__must_hold(ap->lock);
 
 /* libata-eh.c */
 extern unsigned int ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd);
@@ -180,7 +184,8 @@ extern void ata_eh_release(struct ata_port *ap)
 	__releases(ap->host->eh_mutex);
 extern void ata_scsi_error(struct Scsi_Host *host);
 extern void ata_eh_fastdrain_timerfn(struct timer_list *t);
-extern void ata_qc_schedule_eh(struct ata_port *ap, struct ata_queued_cmd *qc);
+extern void ata_qc_schedule_eh(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock);
 extern void ata_dev_disable(struct ata_device *dev);
 extern void ata_eh_detach_dev(struct ata_device *dev);
 extern void ata_eh_about_to_do(struct ata_link *link, struct ata_device *dev,
diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
index b8f7633537c1..0760c760aefc 100644
--- a/drivers/ata/pdc_adma.c
+++ b/drivers/ata/pdc_adma.c
@@ -385,6 +385,7 @@ static unsigned int adma_qc_issue(struct ata_queued_cmd *qc)
 }
 
 static inline unsigned int adma_intr_pkt(struct ata_host *host)
+	__must_hold(host->lock)
 {
 	unsigned int handled = 0, port_no;
 
@@ -395,6 +396,9 @@ static inline unsigned int adma_intr_pkt(struct ata_host *host)
 		void __iomem *chan = ADMA_PORT_REGS(ap);
 		u8 status = readb(chan + ADMA_STATUS);
 
+		/* Tell the compiler that host->lock == ap->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		if (status == 0)
 			continue;
 		handled = 1;
@@ -404,6 +408,9 @@ static inline unsigned int adma_intr_pkt(struct ata_host *host)
 			continue;
 		qc = ata_qc_from_tag(ap, ap->link.active_tag);
 		if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
+			/* Tell the compiler that qc->dev->link->ap == ap. */
+			__assume_ctx_lock(qc->dev->link->ap->lock);
+
 			if (status & aPERR)
 				qc->err_mask |= AC_ERR_HOST_BUS;
 			else if ((status & (aPSD | aUIRQ)))
@@ -435,6 +442,7 @@ static inline unsigned int adma_intr_pkt(struct ata_host *host)
 }
 
 static inline unsigned int adma_intr_mmio(struct ata_host *host)
+	__must_hold(host->lock)
 {
 	unsigned int handled = 0, port_no;
 
@@ -443,11 +451,13 @@ static inline unsigned int adma_intr_mmio(struct ata_host *host)
 		struct adma_port_priv *pp = ap->private_data;
 		struct ata_queued_cmd *qc;
 
+		/* Tell the compiler that host->lock == ap->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		if (!pp || pp->state != adma_state_mmio)
 			continue;
 		qc = ata_qc_from_tag(ap, ap->link.active_tag);
 		if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
-
 			/* check main status, clearing INTRQ */
 			u8 status = ata_sff_check_status(ap);
 			if ((status & ATA_BUSY))
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8cd65a480010..a3d8a6b02609 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -410,6 +410,7 @@ static u32 qcmd_tag_to_mask(u8 tag)
 /* See ahci.c */
 static void sata_dwc_error_intr(struct ata_port *ap,
 				struct sata_dwc_device *hsdev, uint intpr)
+	__must_hold(ap->lock)
 {
 	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
 	struct ata_eh_info *ehi = &ap->link.eh_info;
@@ -478,6 +479,10 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 	intpr = sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);
 
 	ap = host->ports[port];
+
+	/* Tell the compiler that host->lock == ap->lock. */
+	__assume_ctx_lock(ap->lock);
+
 	hsdevp = HSDEVP_FROM_AP(ap);
 
 	dev_dbg(ap->dev, "%s intpr=0x%08x active_tag=%d\n", __func__, intpr,
@@ -738,6 +743,7 @@ static void sata_dwc_dma_xfer_complete(struct ata_port *ap)
 }
 
 static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock)
 {
 	u8 status = 0;
 	u32 mask = 0x0;
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index d05c9603489e..d32c0921968d 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1052,6 +1052,7 @@ static void sata_fsl_post_internal_cmd(struct ata_queued_cmd *qc)
 }
 
 static void sata_fsl_error_intr(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct sata_fsl_host_priv *host_priv = ap->host->private_data;
 	void __iomem *hcr_base = host_priv->hcr_base;
@@ -1178,6 +1179,7 @@ static void sata_fsl_error_intr(struct ata_port *ap)
 }
 
 static void sata_fsl_host_intr(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct sata_fsl_host_priv *host_priv = ap->host->private_data;
 	void __iomem *hcr_base = host_priv->hcr_base;
@@ -1197,6 +1199,9 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 	/* Workaround for data length mismatch errata */
 	if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
 		ata_qc_for_each_with_internal(ap, qc, tag) {
+			/* Tell the compiler that qc->dev->link->ap == ap. */
+			__assume_ctx_lock(qc->dev->link->ap->lock);
+
 			if (qc && ata_is_atapi(qc->tf.protocol)) {
 				u32 hcontrol;
 				/* Set HControl[27] to clear error registers */
@@ -1298,6 +1303,8 @@ static irqreturn_t sata_fsl_interrupt(int irq, void *dev_instance)
 
 	ap = host->ports[0];
 	if (ap) {
+		/* Tell the compiler that host->lock == ap->lock. */
+		__assume_ctx_lock(ap->lock);
 		sata_fsl_host_intr(ap);
 	} else {
 		dev_warn(host->dev, "interrupt on disabled port 0\n");
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index 6ea9fceb8261..0f7a06fe929d 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -324,6 +324,7 @@ static void inic_stop_idma(struct ata_port *ap)
 }
 
 static void inic_host_err_intr(struct ata_port *ap, u8 irq_stat, u16 idma_stat)
+	__must_hold(ap->lock)
 {
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	struct inic_port_priv *pp = ap->private_data;
@@ -380,6 +381,7 @@ static void inic_host_err_intr(struct ata_port *ap, u8 irq_stat, u16 idma_stat)
 }
 
 static void inic_host_intr(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	void __iomem *port_base = inic_port_base(ap);
 	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->link.active_tag);
@@ -430,11 +432,14 @@ static irqreturn_t inic_interrupt(int irq, void *dev_instance)
 
 	spin_lock(&host->lock);
 
-	for (i = 0; i < NR_PORTS; i++)
+	for (i = 0; i < NR_PORTS; i++) {
+		/* Tell the compiler that host->ports[i]->lock == host->lock. */
+		__assume_ctx_lock(host->ports[i]->lock);
 		if (host_irq_stat & (HIRQ_PORT0 << i)) {
 			inic_host_intr(host->ports[i]);
 			handled++;
 		}
+	}
 
 	spin_unlock(&host->lock);
 
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index ea10c12f449c..18a15dcc39aa 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2433,6 +2433,7 @@ static unsigned int mv_get_err_pmp_map(struct ata_port *ap)
 }
 
 static void mv_pmp_eh_prep(struct ata_port *ap, unsigned int pmp_map)
+	__must_hold(ap->lock)
 {
 	unsigned int pmp;
 
@@ -2445,6 +2446,9 @@ static void mv_pmp_eh_prep(struct ata_port *ap, unsigned int pmp_map)
 			struct ata_link *link = &ap->pmp_link[pmp];
 			struct ata_eh_info *ehi = &link->eh_info;
 
+			/* Tell the compiler that link->ap == ap. */
+			__assume_ctx_lock(link->ap->lock);
+
 			pmp_map &= ~this_pmp;
 			ata_ehi_clear_desc(ehi);
 			ata_ehi_push_desc(ehi, "dev err");
@@ -2468,6 +2472,7 @@ static int mv_req_q_empty(struct ata_port *ap)
 }
 
 static int mv_handle_fbs_ncq_dev_err(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct mv_port_priv *pp = ap->private_data;
 	int failed_links;
@@ -2528,6 +2533,7 @@ static int mv_handle_fbs_non_ncq_dev_err(struct ata_port *ap)
 }
 
 static int mv_handle_dev_err(struct ata_port *ap, u32 edma_err_cause)
+	__must_hold(ap->lock)
 {
 	struct mv_port_priv *pp = ap->private_data;
 
@@ -2601,6 +2607,7 @@ static void mv_unexpected_intr(struct ata_port *ap, int edma_was_enabled)
  *      Inherited from caller.
  */
 static void mv_err_intr(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	void __iomem *port_mmio = mv_ap_base(ap);
 	u32 edma_err_cause, eh_freeze_mask, serr = 0;
@@ -2764,6 +2771,7 @@ static bool mv_process_crpb_response(struct ata_port *ap,
 }
 
 static void mv_process_crpb_entries(struct ata_port *ap, struct mv_port_priv *pp)
+	__must_hold(ap->lock)
 {
 	void __iomem *port_mmio = mv_ap_base(ap);
 	struct mv_host_priv *hpriv = ap->host->private_data;
@@ -2806,6 +2814,7 @@ static void mv_process_crpb_entries(struct ata_port *ap, struct mv_port_priv *pp
 }
 
 static void mv_port_intr(struct ata_port *ap, u32 port_cause)
+	__must_hold(ap->lock)
 {
 	struct mv_port_priv *pp;
 	int edma_was_enabled;
@@ -2848,6 +2857,7 @@ static void mv_port_intr(struct ata_port *ap, u32 port_cause)
  *      Inherited from caller.
  */
 static int mv_host_intr(struct ata_host *host, u32 main_irq_cause)
+	__must_hold(host->lock)
 {
 	struct mv_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->base, *hc_mmio;
@@ -2861,6 +2871,9 @@ static int mv_host_intr(struct ata_host *host, u32 main_irq_cause)
 		struct ata_port *ap = host->ports[port];
 		unsigned int p, shift, hardport, port_cause;
 
+		/* Tell the compiler that ap->lock == host->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		MV_PORT_TO_SHIFT_AND_HARDPORT(port, shift, hardport);
 		/*
 		 * Each hc within the host has its own hc_irq_cause register,
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 19b927065868..01bf8302c88c 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -808,6 +808,7 @@ static unsigned int nv_adma_tf_to_cpb(struct ata_taskfile *tf, __le16 *cpb)
 }
 
 static int nv_adma_check_cpb(struct ata_port *ap, int cpb_num, int force_err)
+	__must_hold(ap->lock)
 {
 	struct nv_adma_port_priv *pp = ap->private_data;
 	u8 flags = pp->cpb[cpb_num].resp_flags;
@@ -853,6 +854,7 @@ static int nv_adma_check_cpb(struct ata_port *ap, int cpb_num, int force_err)
 }
 
 static int nv_host_intr(struct ata_port *ap, u8 irq_stat)
+	__must_hold(ap->lock)
 {
 	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->link.active_tag);
 
@@ -892,6 +894,9 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
 		u32 gen_ctl;
 		u32 notifier, notifier_error;
 
+		/* Tell the compiler that ap->lock == host->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		notifier_clears[i] = 0;
 
 		/* if ADMA is disabled, use standard ata interrupt handler */
@@ -1444,6 +1449,9 @@ static irqreturn_t nv_generic_interrupt(int irq, void *dev_instance)
 		struct ata_port *ap = host->ports[i];
 		struct ata_queued_cmd *qc;
 
+		/* Tell the compiler that ap->lock == &host->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		qc = ata_qc_from_tag(ap, ap->link.active_tag);
 		if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
 			handled += ata_bmdma_port_intr(ap, qc);
@@ -1465,7 +1473,10 @@ static irqreturn_t nv_do_interrupt(struct ata_host *host, u8 irq_stat)
 {
 	int i, handled = 0;
 
+	/* TO DO: protect the code below with &host->lock. */
+
 	for (i = 0; i < host->n_ports; i++) {
+		__assume_ctx_lock(host->ports[i]->lock);
 		handled += nv_host_intr(host->ports[i], irq_stat);
 		irq_stat >>= NV_INT_PORT_SHIFT;
 	}
@@ -2064,6 +2075,7 @@ static void nv_swncq_hotplug(struct ata_port *ap, u32 fis)
 }
 
 static int nv_swncq_sdbfis(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct ata_queued_cmd *qc;
 	struct nv_swncq_port_priv *pp = ap->private_data;
@@ -2179,6 +2191,7 @@ static void nv_swncq_dmafis(struct ata_port *ap)
 }
 
 static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis)
+	__must_hold(ap->lock)
 {
 	struct nv_swncq_port_priv *pp = ap->private_data;
 	struct ata_queued_cmd *qc;
@@ -2292,6 +2305,9 @@ static irqreturn_t nv_swncq_interrupt(int irq, void *dev_instance)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
+		/* Tell the compiler that ap->lock == host->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		if (ap->link.sactive) {
 			nv_swncq_host_interrupt(ap, (u16)irq_stat);
 			handled = 1;
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index c980e609b246..877961ac8512 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -835,6 +835,7 @@ static void pdc_post_internal_cmd(struct ata_queued_cmd *qc)
 
 static void pdc_error_intr(struct ata_port *ap, struct ata_queued_cmd *qc,
 			   u32 port_status, u32 err_mask)
+	__must_hold(ap->lock)
 {
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	unsigned int ac_err_mask = 0;
@@ -869,11 +870,15 @@ static void pdc_error_intr(struct ata_port *ap, struct ata_queued_cmd *qc,
 
 static unsigned int pdc_host_intr(struct ata_port *ap,
 				  struct ata_queued_cmd *qc)
+	__must_hold(ap->lock)
 {
 	unsigned int handled = 0;
 	void __iomem *ata_mmio = ap->ioaddr.cmd_addr;
 	u32 port_status, err_mask;
 
+	/* Tell the compiler that qc->dev->link->ap == ap. */
+	__assume_ctx_lock(qc->dev->link->ap->lock);
+
 	err_mask = PDC_ERR_MASK;
 	if (ap->flags & PDC_FLAG_GEN_II)
 		err_mask &= ~PDC1_ERR_MASK;
@@ -974,8 +979,14 @@ static irqreturn_t pdc_interrupt(int irq, void *dev_instance)
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->link.active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
+				/*
+				 * Tell the compiler that ap->lock ==
+				 * host->lock.
+				 */
+				__assume_ctx_lock(ap->lock);
 				handled += pdc_host_intr(ap, qc);
+			}
 		}
 	}
 
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index c682285ea5e5..2406598183b2 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -329,9 +329,13 @@ static unsigned int qs_qc_issue(struct ata_queued_cmd *qc)
 }
 
 static void qs_do_or_die(struct ata_queued_cmd *qc, u8 status)
+	__must_hold(qc->dev->link->ap->lock)
 {
 	struct ata_port *ap = qc->ap;
 
+	/* Tell the compiler that ap == qc->dev->link->ap. */
+	__assume_ctx_lock(ap->lock);
+
 	qc->err_mask |= ac_err_mask(status);
 
 	if (!qc->err_mask) {
@@ -350,6 +354,7 @@ static void qs_do_or_die(struct ata_queued_cmd *qc, u8 status)
 }
 
 static inline unsigned int qs_intr_pkt(struct ata_host *host)
+	__must_hold(host->lock)
 {
 	unsigned int handled = 0;
 	u8 sFFE;
@@ -376,6 +381,11 @@ static inline unsigned int qs_intr_pkt(struct ata_host *host)
 				continue;
 			qc = ata_qc_from_tag(ap, ap->link.active_tag);
 			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
+				/*
+				 * Tell the compiler that qc->dev->link->ap->
+				 * scsi_host == host.
+				 */
+				__assume_ctx_lock(qc->dev->link->ap->lock);
 				switch (sHST) {
 				case 0: /* successful CPB */
 				case 3: /* device error */
@@ -392,6 +402,7 @@ static inline unsigned int qs_intr_pkt(struct ata_host *host)
 }
 
 static inline unsigned int qs_intr_mmio(struct ata_host *host)
+	__must_hold(host->lock)
 {
 	unsigned int handled = 0, port_no;
 
@@ -400,6 +411,9 @@ static inline unsigned int qs_intr_mmio(struct ata_host *host)
 		struct qs_port_priv *pp = ap->private_data;
 		struct ata_queued_cmd *qc;
 
+		/* Tell the compiler that ap->lock == host->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		qc = ata_qc_from_tag(ap, ap->link.active_tag);
 		if (!qc) {
 			/*
diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
index 487eadd4073f..8dc6e325fb88 100644
--- a/drivers/ata/sata_rcar.c
+++ b/drivers/ata/sata_rcar.c
@@ -648,6 +648,7 @@ static struct ata_port_operations sata_rcar_port_ops = {
 };
 
 static void sata_rcar_serr_interrupt(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct sata_rcar_priv *priv = ap->host->private_data;
 	struct ata_eh_info *ehi = &ap->link.eh_info;
@@ -679,6 +680,7 @@ static void sata_rcar_serr_interrupt(struct ata_port *ap)
 }
 
 static void sata_rcar_ata_interrupt(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct ata_queued_cmd *qc;
 	int handled = 0;
@@ -713,6 +715,9 @@ static irqreturn_t sata_rcar_interrupt(int irq, void *dev_instance)
 
 	ap = host->ports[0];
 
+	/* Tell the compiler that ap->lock == host->lock. */
+	__assume_ctx_lock(ap->lock);
+
 	if (sataintstat & SATAINTSTAT_ATA)
 		sata_rcar_ata_interrupt(ap);
 
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 1b6dc950a42a..4545aadaaf10 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -417,6 +417,7 @@ static int sil_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 }
 
 static void sil_host_intr(struct ata_port *ap, u32 bmdma2)
+	__must_hold(ap->lock)
 {
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->link.active_tag);
@@ -524,6 +525,9 @@ static irqreturn_t sil_interrupt(int irq, void *dev_instance)
 		    !(bmdma2 & (SIL_DMA_COMPLETE | SIL_DMA_SATA_IRQ)))
 			continue;
 
+		/* Tell the compiler that ap->lock == host->lock. */
+		__assume_ctx_lock(ap->lock);
+
 		sil_host_intr(ap, bmdma2);
 		handled = 1;
 	}
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 9fc7ca6fb7a1..d61673f9a0d0 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -966,6 +966,7 @@ static void sil24_thaw(struct ata_port *ap)
 }
 
 static void sil24_error_intr(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	void __iomem *port = sil24_port_base(ap);
 	struct sil24_port_priv *pp = ap->private_data;
@@ -1093,6 +1094,7 @@ static void sil24_error_intr(struct ata_port *ap)
 }
 
 static inline void sil24_host_intr(struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	void __iomem *port = sil24_port_base(ap);
 	u32 slot_stat, qc_active;
@@ -1157,6 +1159,11 @@ static irqreturn_t sil24_interrupt(int irq, void *dev_instance)
 
 	for (i = 0; i < host->n_ports; i++)
 		if (status & (1 << i)) {
+			/*
+			 * Tell the compiler that this is the same lock as
+			 * host->lock.
+			 */
+			__assume_ctx_lock(host->ports[i]->lock);
 			sil24_host_intr(host->ports[i]);
 			handled++;
 		}
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index fce5fed2b3be..603ffff5ecef 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -656,6 +656,7 @@ static inline unsigned int pdc20621_host_intr(struct ata_port *ap,
 					  struct ata_queued_cmd *qc,
 					  unsigned int doing_hdma,
 					  void __iomem *mmio)
+	__must_hold(ap->lock)
 {
 	unsigned int port_no = ap->port_no;
 	unsigned int port_ofs =
@@ -778,6 +779,9 @@ static irqreturn_t pdc20621_interrupt(int irq, void *dev_instance)
 		if (tmp && ap) {
 			struct ata_queued_cmd *qc;
 
+			/* Tell the compiler that ap->lock == host->lock. */
+			__assume_ctx_lock(ap->lock);
+
 			qc = ata_qc_from_tag(ap, ap->link.active_tag);
 			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 				handled += pdc20621_host_intr(ap, qc, (i > 4),
diff --git a/drivers/ata/sata_vsc.c b/drivers/ata/sata_vsc.c
index a53a2dfc1e17..0ddc7a45b674 100644
--- a/drivers/ata/sata_vsc.c
+++ b/drivers/ata/sata_vsc.c
@@ -209,6 +209,7 @@ static void vsc_sata_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
 }
 
 static inline void vsc_error_intr(u8 port_status, struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	if (port_status & (VSC_SATA_INT_PHY_CHANGE | VSC_SATA_INT_ERROR_M))
 		ata_port_freeze(ap);
@@ -217,6 +218,7 @@ static inline void vsc_error_intr(u8 port_status, struct ata_port *ap)
 }
 
 static void vsc_port_intr(u8 port_status, struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct ata_queued_cmd *qc;
 	int handled = 0;
@@ -266,6 +268,11 @@ static irqreturn_t vsc_sata_interrupt(int irq, void *dev_instance)
 	for (i = 0; i < host->n_ports; i++) {
 		u8 port_status = (status >> (8 * i)) & 0xff;
 		if (port_status) {
+			/*
+			 * Tell the compiler that this is the same lock as
+			 * host->lock.
+			 */
+			__assume_ctx_lock(host->ports[i]->lock);
 			vsc_port_intr(port_status, host->ports[i]);
 			handled++;
 		}
diff --git a/include/linux/libata.h b/include/linux/libata.h
index bf113e66f069..19867a4909ca 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -666,7 +666,7 @@ struct ata_queued_cmd {
 
 	unsigned int		err_mask;
 	struct ata_taskfile	result_tf;
-	ata_qc_cb_t		complete_fn;
+	ata_qc_cb_t		complete_fn /*__must_hold(qc->ap->lock)*/;
 
 	void			*private_data;
 	void			*lldd_task;
@@ -962,7 +962,8 @@ struct ata_port_operations {
 	int (*qc_defer)(struct ata_queued_cmd *qc);
 	int (*check_atapi_dma)(struct ata_queued_cmd *qc);
 	enum ata_completion_errors (*qc_prep)(struct ata_queued_cmd *qc);
-	unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
+	unsigned int (*qc_issue)(struct ata_queued_cmd *qc)
+		__must_hold(&qc->dev->link->ap->lock);
 	void (*qc_fill_rtf)(struct ata_queued_cmd *qc);
 	void (*qc_ncq_fill_rtf)(struct ata_port *ap, u64 done_mask);
 
@@ -1204,7 +1205,8 @@ extern void ata_id_c_string(const u16 *id, unsigned char *s,
 			    unsigned int ofs, unsigned int len);
 extern unsigned int ata_do_dev_read_id(struct ata_device *dev,
 				       struct ata_taskfile *tf, __le16 *id);
-extern void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc);
+extern void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock);
 extern u64 ata_qc_get_active(struct ata_port *ap);
 extern int ata_std_bios_param(struct scsi_device *sdev,
 			      struct gendisk *unused,
@@ -1314,11 +1316,13 @@ extern int ata_tport_add(struct device *parent, struct ata_port *ap);
 extern void ata_tport_delete(struct ata_port *ap);
 int ata_sas_sdev_configure(struct scsi_device *sdev, struct queue_limits *lim,
 			   struct ata_port *ap);
-extern int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap);
+extern int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
+	__must_hold(ap->lock);
 extern void ata_tf_to_fis(const struct ata_taskfile *tf,
 			  u8 pmp, int is_cmd, u8 *fis);
 extern void ata_tf_from_fis(const u8 *fis, struct ata_taskfile *tf);
-extern int ata_qc_complete_multiple(struct ata_port *ap, u64 qc_active);
+extern int ata_qc_complete_multiple(struct ata_port *ap, u64 qc_active)
+	__must_hold(ap->lock);
 extern bool sata_lpm_ignore_phy_events(struct ata_link *link);
 extern int sata_async_notification(struct ata_port *ap);
 
@@ -1409,8 +1413,10 @@ static inline int ata_acpi_cbl_pata_type(struct ata_port *ap)
  */
 extern void ata_port_schedule_eh(struct ata_port *ap);
 extern void ata_port_wait_eh(struct ata_port *ap);
-extern int ata_link_abort(struct ata_port *ap, struct ata_link *link);
-extern int ata_port_abort(struct ata_port *ap);
+extern int ata_link_abort(struct ata_port *ap, struct ata_link *link)
+	__must_hold(ap->lock);
+extern int ata_port_abort(struct ata_port *ap)
+	__must_hold(ap->lock);
 extern int ata_port_freeze(struct ata_port *ap);
 
 extern void ata_eh_freeze_port(struct ata_port *ap);
@@ -2042,7 +2048,8 @@ extern unsigned int ata_sff_data_xfer32(struct ata_queued_cmd *qc,
 			unsigned char *buf, unsigned int buflen, int rw);
 extern void ata_sff_irq_on(struct ata_port *ap);
 extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
-			    u8 status, int in_wq);
+			    u8 status, int in_wq)
+	__must_hold(ap->lock);
 extern void ata_sff_queue_work(struct work_struct *work);
 extern void ata_sff_queue_delayed_work(struct delayed_work *dwork,
 		unsigned long delay);
@@ -2050,9 +2057,11 @@ extern void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay);
 extern unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc);
 extern void ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
 extern unsigned int ata_sff_port_intr(struct ata_port *ap,
-				      struct ata_queued_cmd *qc);
+				      struct ata_queued_cmd *qc)
+	__must_hold(ap->lock);
 extern irqreturn_t ata_sff_interrupt(int irq, void *dev_instance);
-extern void ata_sff_lost_interrupt(struct ata_port *ap);
+extern void ata_sff_lost_interrupt(struct ata_port *ap)
+	__must_hold(ap->lock);
 extern void ata_sff_freeze(struct ata_port *ap);
 extern void ata_sff_thaw(struct ata_port *ap);
 extern int ata_sff_prereset(struct ata_link *link, unsigned long deadline);
@@ -2095,7 +2104,8 @@ extern enum ata_completion_errors ata_bmdma_qc_prep(struct ata_queued_cmd *qc);
 extern unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc);
 extern enum ata_completion_errors ata_bmdma_dumb_qc_prep(struct ata_queued_cmd *qc);
 extern unsigned int ata_bmdma_port_intr(struct ata_port *ap,
-				      struct ata_queued_cmd *qc);
+				      struct ata_queued_cmd *qc)
+	__must_hold(ap->lock);
 extern irqreturn_t ata_bmdma_interrupt(int irq, void *dev_instance);
 extern void ata_bmdma_error_handler(struct ata_port *ap)
 	__must_hold(&ap->host->eh_mutex);

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Evasion of static analysis through `__assume_ctx_lock` and dangerous TO DO comments.
- [Medium] The `qc_issue` annotation incorrectly takes the address of a lock pointer, creating a double pointer.
- [Medium] Missing address-of operator in `__must_hold(host->lock)` annotations.
- [Medium] `ata_do_link_abort` asserts the wrong lock (`eh_mutex` instead of the port lock).
- [Low] Misleading comment refers to `scsi_host` instead of `lock`.
--

commit 1c3594816cc461100251a14c8ea9e6b3febb1a63
Author: Bart Van Assche <bvanassche@acm.org>

ata: Annotate the code that uses the host lock

This commit adds __must_hold() annotations for functions requiring the host
lock, along with context assertions to help the compiler understand lock
equivalencies. It identifies missing locks in ata_port_freeze() and
nv_do_interrupt().

> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
[ ... ]
> @@ -450,7 +450,8 @@ void ahci_error_handler(struct ata_port *ap)
>  	__must_hold(&ap->host->eh_mutex);
> -u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked);
> +u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
> +	__must_hold(host->lock);

[Severity: Medium]
Since host->lock is an embedded struct (spinlock_t lock) rather than a
pointer, should this be __must_hold(&host->lock)? As written, passing the
struct by value might cause the static analyzer to fail when matching it
against spin_lock(&host->lock). This same pattern appears in
xgene_ahci_handle_broken_edge_irq, adma_intr_pkt, and other host->lock
annotations in this patch.

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
[ ... ]
> @@ -1057,6 +1059,9 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
>  	/* include internal tag in iteration */
>  	ata_qc_for_each_with_internal(ap, qc, tag) {
>  		if (qc && (!link || qc->dev->link == link)) {
> +			/* Tell the compiler that link->ap == ap. */
> +			__assume_ctx_lock(&link->ap->host->eh_mutex);
> +
>  			qc->flags |= ATA_QCFLAG_EH;
>  			ata_qc_complete(ap, qc);

[Severity: Medium]
The comment indicates the intent is to assert that link->ap == ap for the
port lock, but the code asserts the sleepable eh_mutex is held instead.
Could this be a copy-paste error that will prevent the static analyzer from
verifying ap->lock for the inner ata_qc_complete() call?

> @@ -1149,6 +1154,9 @@ static void __ata_port_freeze(struct ata_port *ap)
>   */
>  int ata_port_freeze(struct ata_port *ap)
>  {
> +	/* TO DO: fix all callers of this function that do not hold ap->lock. */
> +	__assume_ctx_lock(ap->lock);
> +
>  	__ata_port_freeze(ap);

[Severity: High]
Does using __assume_ctx_lock() here and in nv_do_interrupt() to silence
static analysis warnings defeat the purpose of the locking annotations?
If there are missing locks, it might be safer to fix the actual locking
bugs or use __must_hold() to properly flag the missing lock context.

> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
[ ... ]
> @@ -1465,7 +1473,10 @@ static irqreturn_t nv_do_interrupt(struct ata_host *host, u8 irq_stat)
>  {
>  	int i, handled = 0;
>  
> +	/* TO DO: protect the code below with &host->lock. */
> +
>  	for (i = 0; i < host->n_ports; i++) {
> +		__assume_ctx_lock(host->ports[i]->lock);
>  		handled += nv_host_intr(host->ports[i], irq_stat);
>  		irq_stat >>= NV_INT_PORT_SHIFT;
>  	}

[Severity: High]
If future developers follow this TO DO comment and add a lock acquisition
here, will it introduce a guaranteed hard deadlock? Existing callers like
nv_nf2_interrupt and nv_ck804_interrupt already hold host->lock before
calling this function, as it is a non-recursive spinlock.

> diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
> --- a/drivers/ata/sata_qstor.c
> +++ b/drivers/ata/sata_qstor.c
[ ... ]
> @@ -376,6 +381,11 @@ static inline unsigned int qs_intr_pkt(struct ata_host *host)
>  				continue;
>  			qc = ata_qc_from_tag(ap, ap->link.active_tag);
>  			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
> +				/*
> +				 * Tell the compiler that qc->dev->link->ap->
> +				 * scsi_host == host.
> +				 */
> +				__assume_ctx_lock(qc->dev->link->ap->lock);

[Severity: Low]
This isn't a bug, but the comment mentions scsi_host == host while the
code asserts the lock context. Is this a leftover copy-paste from another
annotation?

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
[ ... ]
> @@ -962,7 +962,8 @@ struct ata_port_operations {
>  	int (*qc_defer)(struct ata_queued_cmd *qc);
>  	int (*check_atapi_dma)(struct ata_queued_cmd *qc);
>  	enum ata_completion_errors (*qc_prep)(struct ata_queued_cmd *qc);
> -	unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
> +	unsigned int (*qc_issue)(struct ata_queued_cmd *qc)
> +		__must_hold(&qc->dev->link->ap->lock);

[Severity: Medium]
Since ap->lock is already a pointer (spinlock_t *lock), does taking its
address with & here yield a spinlock_t **? This double pointer won't
match the lock context expression expected by the static analyzer when
spin_lock(ap->lock) is used. Should this be updated to drop the & operator?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521173347.2079560-1-bvanassche@acm.org?part=9

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Niklas Cassel]

Hello Bart,

On Thu, May 21, 2026 at 10:33:37AM -0700, Bart Van Assche wrote:
> @@ -5162,6 +5165,10 @@ void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
>  	struct ata_link *link = qc->dev->link;
>  	u8 prot = qc->tf.protocol;
>  
> +	/* Tell the compiler that qc->ap == qc->dev->link->ap->lock == ap. */
> +	__assume_ctx_lock(qc->ap->lock);
> +	__assume_ctx_lock(qc->dev->link->ap->lock);

When I look at the definition of lockdep_assert_held(), when building with
lockdep:
https://github.com/torvalds/linux/blob/v7.1-rc5/include/linux/lockdep.h#L284-L285

When building without lockdep:
https://github.com/torvalds/linux/blob/v7.1-rc5/include/linux/lockdep.h#L392

Even in the case where we don't build with lockdep, we will call
__assume_ctx_lock(l);

So my question is, why don't we instead add:
lockdep_assert_held(qc->ap->lock);
lockdep_assert_held(qc->dev->link->ap->lock);

Right now, there seems to be very few __assume_ctx_lock() in the code base,
would it not be more consistent to add lockdep_assert_held() calls instead?


> @@ -1149,6 +1154,9 @@ static void __ata_port_freeze(struct ata_port *ap)
>   */
>  int ata_port_freeze(struct ata_port *ap)
>  {
> +	/* TO DO: fix all callers of this function that do not hold ap->lock. */

Please write this as one word, i.e. TODO:



Kind regards,
Niklas

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Bart Van Assche]

On 5/26/26 8:16 AM, Niklas Cassel wrote:
> On Thu, May 21, 2026 at 10:33:37AM -0700, Bart Van Assche wrote:
>> @@ -5162,6 +5165,10 @@ void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
>>   	struct ata_link *link = qc->dev->link;
>>   	u8 prot = qc->tf.protocol;
>>   
>> +	/* Tell the compiler that qc->ap == qc->dev->link->ap->lock == ap. */
>> +	__assume_ctx_lock(qc->ap->lock);
>> +	__assume_ctx_lock(qc->dev->link->ap->lock);
> 
> When I look at the definition of lockdep_assert_held(), when building with
> lockdep:
> https://github.com/torvalds/linux/blob/v7.1-rc5/include/linux/lockdep.h#L284-L285
> 
> When building without lockdep:
> https://github.com/torvalds/linux/blob/v7.1-rc5/include/linux/lockdep.h#L392
> 
> Even in the case where we don't build with lockdep, we will call
> __assume_ctx_lock(l);
> 
> So my question is, why don't we instead add:
> lockdep_assert_held(qc->ap->lock);
> lockdep_assert_held(qc->dev->link->ap->lock);
> 
> Right now, there seems to be very few __assume_ctx_lock() in the code base,
> would it not be more consistent to add lockdep_assert_held() calls instead?

My opinion is that adding __assume_ctx_lock() in the implementation of
lockdep_assert_held() was wrong and that this change should be reverted.
Most code annotated with lockdep_assert_held() is suitable for compile-
time context analysis. Adding __assume_ctx_lock() in the
lockdep_assert_held() definition suppresses useful diagnostics from the
compiler about a lock not being held although it should be held.

Whether lockdep_assert_held() or __assume_ctx_lock() is used to help the
compiler alias analysis, there is a risk that these annotations are
incorrect. My preferred solution is that a new macro would be added that
supports expressing pointer equality in a direct way to the compiler 
alias analysis engine and also that evaluates pointer equality at
runtime. However, I'm afraid this requires compiler changes and changing
Clang is out-of-scope for me.

I chose __assume_ctx_lock() because it is evaluated only at compile time
and a compile time annotation is sufficient in this context.

>> @@ -1149,6 +1154,9 @@ static void __ata_port_freeze(struct ata_port *ap)
>>    */
>>   int ata_port_freeze(struct ata_port *ap)
>>   {
>> +	/* TO DO: fix all callers of this function that do not hold ap->lock. */
> 
> Please write this as one word, i.e. TODO:

OK, I will make this change.

Bart.

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Damien Le Moal]

On 5/27/26 6:33 AM, Bart Van Assche wrote:
>>> @@ -1149,6 +1154,9 @@ static void __ata_port_freeze(struct ata_port *ap)
>>>    */
>>>   int ata_port_freeze(struct ata_port *ap)
>>>   {
>>> +    /* TO DO: fix all callers of this function that do not hold ap->lock. */
>>
>> Please write this as one word, i.e. TODO:
> 
> OK, I will make this change.

Absolutely not. If this patch requires fixes for problems it creates, and these
changes are not included within this patch, then the patch is simply wrong.
If that is not the case, then please drop this comment as it means absolutely
nothing.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Marco Elver]

On Tue, 26 May 2026 at 23:33, Bart Van Assche <bvanassche@acm.org> wrote:
> On 5/26/26 8:16 AM, Niklas Cassel wrote:
> > On Thu, May 21, 2026 at 10:33:37AM -0700, Bart Van Assche wrote:
> >> @@ -5162,6 +5165,10 @@ void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
> >>      struct ata_link *link = qc->dev->link;
> >>      u8 prot = qc->tf.protocol;
> >>
> >> +    /* Tell the compiler that qc->ap == qc->dev->link->ap->lock == ap. */
> >> +    __assume_ctx_lock(qc->ap->lock);
> >> +    __assume_ctx_lock(qc->dev->link->ap->lock);
> >
> > When I look at the definition of lockdep_assert_held(), when building with
> > lockdep:
> > https://github.com/torvalds/linux/blob/v7.1-rc5/include/linux/lockdep.h#L284-L285
> >
> > When building without lockdep:
> > https://github.com/torvalds/linux/blob/v7.1-rc5/include/linux/lockdep.h#L392
> >
> > Even in the case where we don't build with lockdep, we will call
> > __assume_ctx_lock(l);
> >
> > So my question is, why don't we instead add:
> > lockdep_assert_held(qc->ap->lock);
> > lockdep_assert_held(qc->dev->link->ap->lock);
> >
> > Right now, there seems to be very few __assume_ctx_lock() in the code base,
> > would it not be more consistent to add lockdep_assert_held() calls instead?
>
> My opinion is that adding __assume_ctx_lock() in the implementation of
> lockdep_assert_held() was wrong and that this change should be reverted.

I don't know how to convince you otherwise, because the trade-offs are:

A. Current design: lockdep_assert_held() implies __assume_ctx_lock(),
and ensures that assumption are checked at runtime. Soundness, which
is your concern, is only affected if we never cover this code with
lockdep. For complex control flow or cases such as here, we have a
_sound_ escape hatch, not because the static checking is sound, but
because we _force_ a runtime fallback. That's the intended programming
model here (__assume_ctx_lock alone is discouraged).

B. Remove the implicit __assume_ctx_lock() from lockdep_assert_held():
For cases requiring __assume_ctx_lock() annotations, we have no forced
runtime fallback. We need to explicitly add lockdep_assert_held()
alongside, which is ugly and hard to enforce. The design is less sound
than A, and requires more churn, neither of which scales well.

The problem that lockdep_assert_held() suppresses useful checking is
only a problem because we're dealing with code that has pre-existing
lockdep_assert_held(). The correct design is A, it's just that we're
dealing with a codebase with history, and it'll take time to remove
redundant lockdep_assert_held() where Context Analysis now offers
better (static) analysis. Starting with the wrong design, and then
trying to transition to the good design is harder than accepting the
small hit to

> Most code annotated with lockdep_assert_held() is suitable for compile-
> time context analysis. Adding __assume_ctx_lock() in the
> lockdep_assert_held() definition suppresses useful diagnostics from the
> compiler about a lock not being held although it should be held.

Then the assumption is checked at runtime when running with lockdep.
This is by design, because it goes both ways: every time you now add
__assume_ctx_lock(), someone just has to move that code around and
suddenly the assumption no longer holds and you have a bug. If you
were using lockdep_assert_held() you're at least ensuring that someone
running with lockdep (e.g. syzkaller) will catch the bad assumption.

> Whether lockdep_assert_held() or __assume_ctx_lock() is used to help the
> compiler alias analysis, there is a risk that these annotations are
> incorrect. My preferred solution is that a new macro would be added that
> supports expressing pointer equality in a direct way to the compiler
> alias analysis engine and also that evaluates pointer equality at
> runtime. However, I'm afraid this requires compiler changes and changing
> Clang is out-of-scope for me.

Telling the compiler about pointer-equality is also unsound, because
there's no way to verify our assumption statically. The only way to
recover soundness is to again introduce runtime checks of some sort.

> I chose __assume_ctx_lock() because it is evaluated only at compile time
> and a compile time annotation is sufficient in this context.

There's a real risk that your assumption may become stale, and if it
were my codebase, I'd prefer lockdep_assert_held() just to make sure
we check our assumption at runtime. This is not my subsystem -- just
my 2c.

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Niklas Cassel]

On Wed, May 27, 2026 at 12:40:24AM +0200, Marco Elver wrote:
> On Tue, 26 May 2026 at 23:33, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> Then the assumption is checked at runtime when running with lockdep.
> This is by design, because it goes both ways: every time you now add
> __assume_ctx_lock(), someone just has to move that code around and
> suddenly the assumption no longer holds and you have a bug. If you
> were using lockdep_assert_held() you're at least ensuring that someone
> running with lockdep (e.g. syzkaller) will catch the bad assumption.

My biggest worry/issue with this series is all the __assume_ctx_lock()
annotations it adds. Because __assume_ctx_lock() seems to tell the compiler:
"trust me, I have taken this lock". It seems quite fragile, and if someone
refactors the code, the annotation might no longer be valid.

I suggested something similar to what Bart suggested, a:
__objects_are_equal(dev->link->ap->lock, ap->lock) annotation.
But as Bart said, that would probably require additions to Clang.

But here Marco says that pointer-equality is unsound, since it can't be
verified statically and would have to be verified at rutime...


> 
> > Whether lockdep_assert_held() or __assume_ctx_lock() is used to help the
> > compiler alias analysis, there is a risk that these annotations are
> > incorrect. My preferred solution is that a new macro would be added that
> > supports expressing pointer equality in a direct way to the compiler
> > alias analysis engine and also that evaluates pointer equality at
> > runtime. However, I'm afraid this requires compiler changes and changing
> > Clang is out-of-scope for me.
> 
> Telling the compiler about pointer-equality is also unsound, because
> there's no way to verify our assumption statically. The only way to
> recover soundness is to again introduce runtime checks of some sort.
> 
> > I chose __assume_ctx_lock() because it is evaluated only at compile time
> > and a compile time annotation is sufficient in this context.
> 
> There's a real risk that your assumption may become stale, and if it
> were my codebase, I'd prefer lockdep_assert_held() just to make sure
> we check our assumption at runtime. This is not my subsystem -- just
> my 2c.

Right now, I think my ideal solution would be to, to the furthest extent
possible, try to avoid adding __assume_ctx_lock().
Sure, a few special cases might be acceptable, if there are no reasonable
way to avoid it.

And if there is no reasonable way to avoid it, at least with the current
behavior in linux-next/master, I would prefer a lockdep_assert_held() over
a __assume_ctx_lock().


Kind regards,
Niklas

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Niklas Cassel]

On Thu, May 21, 2026 at 10:33:37AM -0700, Bart Van Assche wrote:
> Annotate all functions and also their direct and indirect callers with
> __must_hold() that have a lockdep_assert_held() statement in their
> function body for the host lock. As one can see in the comments added by
> this patch, locking is missing from the following two functions:
>  * Some of the ata_port_freeze() callers.
>  * nv_do_interrupt().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

(snip)

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 8b8081f4dc63..52bcd8c78774 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5027,6 +5027,9 @@ void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
>  	struct ata_device *dev = qc->dev;
>  	struct ata_eh_info *ehi = &dev->link->eh_info;
>  
> +	/* Tell the compiler that qc->dev->link->ap == ap. */
> +	__assume_ctx_lock(qc->dev->link->ap->lock);

Is there any way we could avoid this __assume_ctx_lock() ?

Which is the function we call that needs this?

Could we avoid it if we add a ap / qc / link argument to that function?



> +
>  	/* Trigger the LED (if available) */
>  	ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
>  
> @@ -5162,6 +5165,10 @@ void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
>  	struct ata_link *link = qc->dev->link;
>  	u8 prot = qc->tf.protocol;
>  
> +	/* Tell the compiler that qc->ap == qc->dev->link->ap->lock == ap. */
> +	__assume_ctx_lock(qc->ap->lock);
> +	__assume_ctx_lock(qc->dev->link->ap->lock);

Is there any way we could avoid these two __assume_ctx_lock() ?

Which is the function we call that needs this?

Could we avoid it if we add a ap / qc / link argument to that function?



Kind regards,
Niklas

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Damien Le Moal]

On 2026/05/22 2:33, Bart Van Assche wrote:
> Annotate all functions and also their direct and indirect callers with
> __must_hold() that have a lockdep_assert_held() statement in their
> function body for the host lock. As one can see in the comments added by
> this patch, locking is missing from the following two functions:
>  * Some of the ata_port_freeze() callers.
>  * nv_do_interrupt().

What do you mean with locking is missing ? Do you mean we have locking problems
and bugs ? Clarify please.

I am not a big fan of all the __assume_ctx_lock() calls because I do not really
understand them, especially together with the comments you have that do things like:

+			/* Tell the compiler that link->ap == ap. */
+			__assume_ctx_lock(&link->ap->host->eh_mutex);

Form the name __assume_ctx_lock(), I do not see the relevance of the comment. My
understanding is that __assume_ctx_lock() tells the compiler to assume that a
particular lock is being held. But if that's the case, and the addition of these
__assume_ctx_lock() are bogus, what do we endup checking here ?

Back to my previous comment on patch 2, I would prefer, at least for now, a
lighter version of this that adds more lockdep annotations, and simple/obvious
static checks with __must_hold() annotations. Ideally, as suggested, a
locked_must_hold() function doing both would be nice :)

I am all for improving lock checking in the ata code. But We have been doing
fine without it until now. So let's not take this too far from the start and
improve annotations step by step.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Bart Van Assche]

On 5/27/26 11:51 AM, Damien Le Moal wrote:
> On 2026/05/22 2:33, Bart Van Assche wrote:
>> Annotate all functions and also their direct and indirect callers with
>> __must_hold() that have a lockdep_assert_held() statement in their
>> function body for the host lock. As one can see in the comments added by
>> this patch, locking is missing from the following two functions:
>>   * Some of the ata_port_freeze() callers.
>>   * nv_do_interrupt().
> 
> What do you mean with locking is missing ? Do you mean we have locking problems
> and bugs ? Clarify please.

Yes. It means that there is a call chain from the code where I wrote 
that locking is missing to a lockdep_assert_held() statement for that
particular synchronization object.

Bart.

Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Damien Le Moal]

On 2026/05/28 3:54, Bart Van Assche wrote:
> On 5/27/26 11:51 AM, Damien Le Moal wrote:
>> On 2026/05/22 2:33, Bart Van Assche wrote:
>>> Annotate all functions and also their direct and indirect callers with
>>> __must_hold() that have a lockdep_assert_held() statement in their
>>> function body for the host lock. As one can see in the comments added by
>>> this patch, locking is missing from the following two functions:
>>>   * Some of the ata_port_freeze() callers.
>>>   * nv_do_interrupt().
>>
>> What do you mean with locking is missing ? Do you mean we have locking problems
>> and bugs ? Clarify please.
> 
> Yes. It means that there is a call chain from the code where I wrote 
> that locking is missing to a lockdep_assert_held() statement for that
> particular synchronization object.

OK. So this shows that improving lock annotations does find issues. Nice!

So please at the very least mentions that in the cover letter, with lockdep
splats or compiler warnings showing the issues. We need to fix that before
applying your patches. Ideally, your series should start with fix patches for
the issues you found. But if you cannot do it, we can look into it.



-- 
Damien Le Moal
Western Digital Research

Re: (subset) [PATCH v2 0/9] ata: libata-core: Enable context analysis

[Niklas Cassel]

On Thu, 21 May 2026 10:33:28 -0700, Bart Van Assche wrote:
> This patch series enables context analysis for the ATA subsystem. Please
> consider this patch series for the next merge window.
> 
> Thanks,
> 
> Bart.
> 
> [...]

Applied to libata/linux.git (for-next), thanks!

[1/9] ata: libata: Fix ata_exec_internal()
      https://git.kernel.org/libata/linux/c/6e0ce123

Kind regards,
Niklas