[PATCH 0/3] ata: add __must_hold(ap->lock) annotations in issuing path

[PATCH 0/3] ata: add __must_hold(ap->lock) annotations in issuing path

[Niklas Cassel]

Hello all,

This series is meant to be applied on top of Bart's patch that already sets:
CONTEXT_ANALYSIS := y
and adds __must_hold(ap->host->eh_mutex) for EH functions.


This adds __must_hold(ap->lock) annotations in the issuing path.

This only adds annotations to a small number of functions.
Additional functions can be annotated later.


By only annotating the issuing path, we are not blocked by first needing to
fix some of the ata_port_freeze() callers not taking the lock.

I tried to look at nv_do_interrupt(), but AFAICT, it does take the lock.



I much prefer adding a "struct ata_port *ap" parameter to the issuing functions,
because it avoids the problem that the context analysis cannot recognize that
qc->ap == ap. (Since it relies on static code analysis.)

Because the alternative, adding a bunch of lockdep_assert_held() before a
function call to a function annotated with __must_held(), simply tells the
compiler to assume that the lock is held (because the lockdep_assert_held()
macro has a call to __assume_ctx_lock()).

By adding an ap parameter, there is no need to assume any lock context,
it can be verified all the way from the function taking the lock.


Kind regards,
Niklas


Bart Van Assche (2):
  ata: libata: Pass ap parameter directly to functions in the issuing
    path
  ata: Annotate functions in the issuing path with __must_hold()

Niklas Cassel (1):
  ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

 drivers/ata/libata-core.c |  6 +++---
 drivers/ata/libata-sata.c |  3 ++-
 drivers/ata/libata-scsi.c | 20 ++++++++++----------
 drivers/ata/libata.h      |  7 +++++--
 include/linux/libata.h    |  3 ++-
 5 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.54.0

[PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[Niklas Cassel]

lockdep_assert_held() indirectly calls __assume_ctx_lock(), which will
suppress any real context analysis from being performed.

Remove the lockdep_assert_held(). This will be replaced with context
analysis in a follow-up commit.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 6733f2b14521..8bfd3ffd8b89 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1852,8 +1852,6 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
 	struct ata_port *ap = dev->link->ap;
 	struct ata_queued_cmd *qc;
 
-	lockdep_assert_held(ap->lock);
-
 	/*
 	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
 	 * have nothing further to do when allocating a qc fails.
-- 
2.54.0

[PATCH 2/3] ata: libata: Pass ap parameter directly to functions in the issuing path

[Niklas Cassel]

From: Bart Van Assche <bvanassche@acm.org>

Context analysis cannot recognize that qc->ap == ap.

Therefore, grow a struct ata_port parameter to the following functions:
ata_qc_issue(), __ata_scsi_queuecmd(), and ata_scsi_translate()
such that we will be able to enable context analysis in a follow-up commit.

No functionality has been changed.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 7e9faf2bceb3..830ce49587f3 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);
 
@@ -5160,9 +5160,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-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 8bfd3ffd8b89..c4b53c94c82b 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;
 }
@@ -1847,9 +1847,8 @@ static int ata_scsi_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
  *	command needs to be deferred.
  */
 static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
-			      ata_xlat_func_t xlat_func)
+			      ata_xlat_func_t xlat_func, struct ata_port *ap)
 {
-	struct ata_port *ap = dev->link->ap;
 	struct ata_queued_cmd *qc;
 
 	/*
@@ -4504,9 +4503,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;
 
@@ -4547,7 +4546,7 @@ enum scsi_qc_status __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
 	}
 
 	if (xlat_func)
-		return ata_scsi_translate(dev, scmd, xlat_func);
+		return ata_scsi_translate(dev, scmd, xlat_func, ap);
 
 	ata_scsi_simulate(dev, scmd);
 
@@ -4593,7 +4592,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 b41601ad9d11..7f9b889c9239 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);
@@ -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);
 
-- 
2.54.0

[PATCH 3/3] ata: Annotate functions in the issuing path with __must_hold()

[Niklas Cassel]

From: Bart Van Assche <bvanassche@acm.org>

Annotate the following functions used in the issuing path:
ata_qc_issue(), ata_sas_queuecmd(), ata_scsi_qc_issue(),
ata_scsi_translate(), __ata_scsi_queuecmd()

These functions are all used in the issuing path, so context analysis will
be able to verify that the ap lock is held, from it is taken in
sas_queuecommand() or ata_scsi_queuecmd() all the way down to
ata_qc_issue().

Commenting out the spin_lock_irqsave() successfully results in a compiler
error on Clang 23.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Co-developed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 1 +
 drivers/ata/libata-sata.c | 1 +
 drivers/ata/libata-scsi.c | 3 +++
 drivers/ata/libata.h      | 6 ++++--
 include/linux/libata.h    | 3 ++-
 5 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 830ce49587f3..6ff61eff245b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5161,6 +5161,7 @@ EXPORT_SYMBOL_GPL(ata_qc_get_active);
  *	spin_lock_irqsave(host lock)
  */
 void ata_qc_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock)
 {
 	struct ata_link *link = qc->dev->link;
 	u8 prot = qc->tf.protocol;
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 5e5be6bbf32a..b0706c30da05 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1377,6 +1377,7 @@ EXPORT_SYMBOL_GPL(ata_sas_sdev_configure);
  */
 
 int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	if (likely(ata_dev_enabled(ap->link.device)))
 		return __ata_scsi_queuecmd(cmd, ap->link.device, ap);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index c4b53c94c82b..b82a421e2242 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1763,6 +1763,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 +1849,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, struct ata_port *ap)
+	__must_hold(ap->lock)
 {
 	struct ata_queued_cmd *qc;
 
@@ -4505,6 +4507,7 @@ 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_port *ap)
+	__must_hold(ap->lock)
 {
 	u8 scsi_op = scmd->cmnd[0];
 	ata_xlat_func_t xlat_func;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 7f9b889c9239..902c53baa109 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -88,7 +88,8 @@ 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_issue(struct ata_port *ap, struct ata_queued_cmd *qc)
+	__must_hold(ap->lock);
 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);
@@ -167,7 +168,8 @@ 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);
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5f6bbe5d504f..c400aa3af93f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1314,7 +1314,8 @@ 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);
-- 
2.54.0

Re: [PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[Bart Van Assche]

On 5/28/26 10:28 AM, Niklas Cassel wrote:
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 6733f2b14521..8bfd3ffd8b89 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1852,8 +1852,6 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
>   	struct ata_port *ap = dev->link->ap;
>   	struct ata_queued_cmd *qc;
>   
> -	lockdep_assert_held(ap->lock);
> -
>   	/*
>   	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
>   	 * have nothing further to do when allocating a qc fails.

This patch removes a useful runtime check. I think it is better to
add the following annotation to ata_scsi_translate() if the goal is
to make the compiler check that all callers hold ap->lock:

__must_hold(dev->link->ap->lock)

Thanks,

Bart.

Re: [PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing the runtime `lockdep_assert_held()` assertion weakens dynamic lock verification in favor of static analysis.
--

commit 5b34afedb186c216f09eff8d76c27e0b405885ea
Author: Niklas Cassel <cassel@kernel.org>

ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

This commit removes the lockdep_assert_held() check from
ata_scsi_translate(), stating that it suppresses context analysis by
indirectly calling __assume_ctx_lock(). The commit message notes this
will be replaced with context analysis in a follow-up commit.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index f44612e269a4d..c31267091662a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1852,8 +1852,6 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
>  	struct ata_port *ap = dev->link->ap;
>  	struct ata_queued_cmd *qc;
>  
> -	lockdep_assert_held(ap->lock);
> -

[Severity: Medium]
Is it safe to completely remove this runtime assertion instead of combining
it with the static analysis annotation?

While __assume_ctx_lock() tells the compiler the lock is assumed held inside
the function, the standard practice to statically enforce that the caller
holds the lock is to add __must_hold(ap->lock) to the function signature
while retaining lockdep_assert_held() in the body. Retaining both provides
valuable run-time validation alongside compile-time checks.

Additionally, does removing this check here and introducing the static context
analysis in a follow-up commit temporarily leave this function without any
locking verification during intermediate bisect steps?

>  	/*
>  	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
>  	 * have nothing further to do when allocating a qc fails.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528172855.703631-5-cassel@kernel.org?part=1

Re: [PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[Niklas Cassel]

On Thu, May 28, 2026 at 10:33:30AM -0700, Bart Van Assche wrote:
> On 5/28/26 10:28 AM, Niklas Cassel wrote:
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 6733f2b14521..8bfd3ffd8b89 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1852,8 +1852,6 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
> >   	struct ata_port *ap = dev->link->ap;
> >   	struct ata_queued_cmd *qc;
> > -	lockdep_assert_held(ap->lock);
> > -
> >   	/*
> >   	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
> >   	 * have nothing further to do when allocating a qc fails.
> 
> This patch removes a useful runtime check. I think it is better to
> add the following annotation to ata_scsi_translate() if the goal is
> to make the compiler check that all callers hold ap->lock:
> 
> __must_hold(dev->link->ap->lock)

Well, this runtime check was added in 2025-12-17. We did not have it before
then, so it is a very recent check.

As you can see in patch 2/3, I opted to add a "struct ata_port *ap" parameter
to ata_scsi_translate() in order for the context to be provided all the way
from the function taking the lock (e.g. ata_scsi_queuecmd()) to ata_qc_issue().

So I guess you mean __must_hold(ap->lock) in ata_scsi_translate() ?
This series already does have that.


As you know lockdep_assert_held() does a __assume_ctx_lock().
What I don't like about this is that, if the lockdep_assert_held() is there,
you can even remove the __must_hold(ap->lock) from from ata_scsi_translate(),
and we will still not get a build error...

If I remove the lockdep_assert_held() and I don't have a __must_hold(ap->lock)
annotation on ata_scsi_translate(), then I get a build error.

So I wanted to remove it such that we don't have any "fake assumptions".
There is no need for any assumptions, since Clang will be able to verify the
call chain all the way from ata_scsi_queuecmd() to ata_qc_issue().

(I'm kind of starting to see why you wanted to remove the __assume_ctx_lock()
from lockdep_assert_held().)

I guess my take is that, lockdep_assert_held() is nice since it will do
runtime checking... but if we can do context analysis without any assumption
(uninterrupted by any __assume_ctx_lock()), then is seems like context analysis
is better in every way.


If we really want to do runtime verification, and runtime verification only,
perhaps we can add a new macro that explicitly does not call __assume_ctx_lock().
Something like lockdep_assert_held_no_assume_ctx_lock() :)


Kind regards,
Niklas

Re: [PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[Bart Van Assche]

On 5/28/26 10:54 AM, Niklas Cassel wrote:
> On Thu, May 28, 2026 at 10:33:30AM -0700, Bart Van Assche wrote:
>> On 5/28/26 10:28 AM, Niklas Cassel wrote:
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index 6733f2b14521..8bfd3ffd8b89 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -1852,8 +1852,6 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
>>>    	struct ata_port *ap = dev->link->ap;
>>>    	struct ata_queued_cmd *qc;
>>> -	lockdep_assert_held(ap->lock);
>>> -
>>>    	/*
>>>    	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
>>>    	 * have nothing further to do when allocating a qc fails.
>>
>> This patch removes a useful runtime check. I think it is better to
>> add the following annotation to ata_scsi_translate() if the goal is
>> to make the compiler check that all callers hold ap->lock:
>>
>> __must_hold(dev->link->ap->lock)
> 
> Well, this runtime check was added in 2025-12-17. We did not have it before
> then, so it is a very recent check.
> 
> As you can see in patch 2/3, I opted to add a "struct ata_port *ap" parameter
> to ata_scsi_translate() in order for the context to be provided all the way
> from the function taking the lock (e.g. ata_scsi_queuecmd()) to ata_qc_issue().
> 
> So I guess you mean __must_hold(ap->lock) in ata_scsi_translate() ?
> This series already does have that.
> 
> 
> As you know lockdep_assert_held() does a __assume_ctx_lock().
> What I don't like about this is that, if the lockdep_assert_held() is there,
> you can even remove the __must_hold(ap->lock) from from ata_scsi_translate(),
> and we will still not get a build error...
> 
> If I remove the lockdep_assert_held() and I don't have a __must_hold(ap->lock)
> annotation on ata_scsi_translate(), then I get a build error.
> 
> So I wanted to remove it such that we don't have any "fake assumptions".
> There is no need for any assumptions, since Clang will be able to verify the
> call chain all the way from ata_scsi_queuecmd() to ata_qc_issue().
> 
> (I'm kind of starting to see why you wanted to remove the __assume_ctx_lock()
> from lockdep_assert_held().)
> 
> I guess my take is that, lockdep_assert_held() is nice since it will do
> runtime checking... but if we can do context analysis without any assumption
> (uninterrupted by any __assume_ctx_lock()), then is seems like context analysis
> is better in every way.
> 
> 
> If we really want to do runtime verification, and runtime verification only,
> perhaps we can add a new macro that explicitly does not call __assume_ctx_lock().
> Something like lockdep_assert_held_no_assume_ctx_lock() :)

Please note that what I wrote is a suggestion only. If you want to 
proceed with this patch that's fine with me.

Thanks,

Bart.

Re: [PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[Hannes Reinecke]

On 5/28/26 19:28, Niklas Cassel wrote:
> lockdep_assert_held() indirectly calls __assume_ctx_lock(), which will
> suppress any real context analysis from being performed.
> 
> Remove the lockdep_assert_held(). This will be replaced with context
> analysis in a follow-up commit.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 2 --
>   1 file changed, 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

Re: [PATCH 2/3] ata: libata: Pass ap parameter directly to functions in the issuing path

[Hannes Reinecke]

On 5/28/26 19:28, Niklas Cassel wrote:
> From: Bart Van Assche <bvanassche@acm.org>
> 
> Context analysis cannot recognize that qc->ap == ap.
> 
> Therefore, grow a struct ata_port parameter to the following functions:
> ata_qc_issue(), __ata_scsi_queuecmd(), and ata_scsi_translate()
> such that we will be able to enable context analysis in a follow-up commit.
> 
> No functionality has been changed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ata/libata-core.c |  5 ++---
>   drivers/ata/libata-sata.c |  2 +-
>   drivers/ata/libata-scsi.c | 15 +++++++--------
>   drivers/ata/libata.h      |  5 +++--
>   4 files changed, 13 insertions(+), 14 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

Re: [PATCH 3/3] ata: Annotate functions in the issuing path with __must_hold()

[Hannes Reinecke]

On 5/28/26 19:28, Niklas Cassel wrote:
> From: Bart Van Assche <bvanassche@acm.org>
> 
> Annotate the following functions used in the issuing path:
> ata_qc_issue(), ata_sas_queuecmd(), ata_scsi_qc_issue(),
> ata_scsi_translate(), __ata_scsi_queuecmd()
> 
> These functions are all used in the issuing path, so context analysis will
> be able to verify that the ap lock is held, from it is taken in
> sas_queuecommand() or ata_scsi_queuecmd() all the way down to
> ata_qc_issue().
> 
> Commenting out the spin_lock_irqsave() successfully results in a compiler
> error on Clang 23.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Co-developed-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c | 1 +
>   drivers/ata/libata-sata.c | 1 +
>   drivers/ata/libata-scsi.c | 3 +++
>   drivers/ata/libata.h      | 6 ++++--
>   include/linux/libata.h    | 3 ++-
>   5 files changed, 11 insertions(+), 3 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

Re: [PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[Damien Le Moal]

On 2026/05/29 2:28, Niklas Cassel wrote:
> lockdep_assert_held() indirectly calls __assume_ctx_lock(), which will
> suppress any real context analysis from being performed.
> 
> Remove the lockdep_assert_held(). This will be replaced with context
> analysis in a follow-up commit.

I am not really liking the idea of removing runtime checks and replacing them
with compile time checks that work only with clang. What about gcc ? And are we
sure that the compile time checks are as solid as the runtime ones ?

As the sashiko comment mentions, can't we keep this and simply add a
__must_hold() annotation in the declaration ?

> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-scsi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 6733f2b14521..8bfd3ffd8b89 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1852,8 +1852,6 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
>  	struct ata_port *ap = dev->link->ap;
>  	struct ata_queued_cmd *qc;
>  
> -	lockdep_assert_held(ap->lock);
> -
>  	/*
>  	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
>  	 * have nothing further to do when allocating a qc fails.


-- 
Damien Le Moal
Western Digital Research