Skip to contentSkip to conversation

ata: add __must_hold(ap->lock) annotations in issuing path

PATCH Applied Niklas Cassel 5 participants 14 messages May 28 – May 31 libata

lore
Share & tools
lore

Niklas Cassel

author maintainer
#0 lore raw

cover ata: add __must_hold(ap->lock) annotations in issuing path

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(-)
Signature · 1 line
2.54.0
author maintainer
#1 lore raw

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

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 <[email protected]>
---
 drivers/ata/libata-scsi.c | 2 --
 1 file changed, 2 deletions(-)
drivers/ata/libata-scsi.c +0 -2
index 6733f2b14521..8bfd3ffd8b89 100644
@@ -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.
Signature · 1 line
2.54.0
#2 lore raw

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

drivers/ata/libata-scsi.c static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd, · line 1857

Quoted 13 lines · Niklas Cassel
drivers/ata/libata-scsi.c +0 -2
index 6733f2b14521..8bfd3ffd8b89 100644
@@ -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.

author maintainer
#3 lore raw

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

Quoted 18 lines · Bart Van Assche

drivers/ata/libata-scsi.c static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd, · line 1856

Quoted 12 lines · Niklas Cassel
drivers/ata/libata-scsi.c +0 -2
index 6733f2b14521..8bfd3ffd8b89 100644
@@ -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:

Quoted 1 line

__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

#4 lore raw

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

Quoted 53 lines · Niklas Cassel
Quoted 18 lines · Bart Van Assche

drivers/ata/libata-scsi.c static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd, · line 1856

Quoted 12 lines · Niklas Cassel
drivers/ata/libata-scsi.c +0 -2
index 6733f2b14521..8bfd3ffd8b89 100644
@@ -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:

Quoted 1 line

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

Quoted 3 lines

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.

AI review
#5 lore raw

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

In reply to Niklas Cassel
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…

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 <[email protected]>

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.

drivers/ata/libata-scsi.c static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd, · line 1856

Quoted 10 lines
drivers/ata/libata-scsi.c +0 -2
index f44612e269a4d..c31267091662a 100644
@@ -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?

Quoted 3 lines
drivers/ata/libata-scsi.c +0 -0
 	/*
 	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
 	 * have nothing further to do when allocating a qc fails.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v1 1/3
#6 lore raw

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

In reply to Niklas Cassel
Quoted 11 lines · 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 <[email protected]>
---
  drivers/ata/libata-scsi.c | 2 --
  1 file changed, 2 deletions(-)
Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +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
maintainer
#7 lore raw

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

In reply to Niklas Cassel
Quoted 5 lines · 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.

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 ?

Quoted 19 lines
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-scsi.c | 2 --
 1 file changed, 2 deletions(-)
drivers/ata/libata-scsi.c +0 -2
index 6733f2b14521..8bfd3ffd8b89 100644
@@ -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.
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#8 lore raw

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

Quoted 9 lines · Damien Le Moal
Quoted 5 lines · 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.

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 ?

Intel 0-day CI testing robot builds with Clang as well.

Quoted 2 lines

And are we sure that the compile time checks are as solid as the runtime ones ?

Runtime checking can be performed on more complex cases (like Marco said checks in runtime does allow to compare two pointers which might point to the same object, which is something that context analysis might cannot catch), however I expect in the cases where compile time analysis is possible, as it is in this case (since we now supply an ap pointer), it will be as solid as runtime checks.

Quoted 3 lines

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

What I don't like about it is that, since lockdep_assert_held() has a __assume_ctx_lock(), it will tell the compiler that that the function always has the lock, which renders the __must_hold() in ata_scsi_translate() useless. I can even remove the annotation without getting a build error. (So it basically yields the __most_hold() in ata_scsi_translate() a no-op.)

Without the lockdep_assert_held(), we do get a build error if ata_scsi_translate() is missing the __must_hold() annotation. I do like the fact that we get an error if we don't have annotations all the way from ata_scsi_queuecmd() to ata_qc_issue(), if there is a function in between that is missing annotations.

And since the lockdep_assert_held() was added only in December last year, I don't see the big deal with replacing it with a compile time check.

But since both you and Bart seem to want to keep it, let's can keep it.

Kind regards,
Niklas

author maintainer
#9 lore raw

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

In reply to Niklas Cassel

From: Bart Van Assche <[email protected]>

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 <[email protected]>
---
 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(-)
drivers/ata/libata-core.c +2 -3
index 7e9faf2bceb3..830ce49587f3 100644
@@ -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;
drivers/ata/libata-sata.c +1 -1
index 2ee54d60ea4b..5e5be6bbf32a 100644
@@ -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);
drivers/ata/libata-scsi.c +7 -8
index 8bfd3ffd8b89..c4b53c94c82b 100644
@@ -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);
drivers/ata/libata.h +3 -2
index b41601ad9d11..7f9b889c9239 100644
@@ -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);
Signature · 1 line
2.54.0
Reviewed-by on v1 2/3
#10 lore raw

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

Quoted 10 lines · Niklas Cassel

From: Bart Van Assche <[email protected]>

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.

Quoted 8 lines
Signed-off-by Bart Van Assche <[email protected]>
---
  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 <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +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
author maintainer
#11 lore raw

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

In reply to Niklas Cassel

From: Bart Van Assche <[email protected]>

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 <[email protected]>
Co-developed-by Niklas Cassel <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
 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(-)
drivers/ata/libata-core.c +1 -0
index 830ce49587f3..6ff61eff245b 100644
@@ -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;
drivers/ata/libata-sata.c +1 -0
index 5e5be6bbf32a..b0706c30da05 100644
@@ -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);
drivers/ata/libata-scsi.c +3 -0
index c4b53c94c82b..b82a421e2242 100644
@@ -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;
drivers/ata/libata.h +4 -2
index 7f9b889c9239..902c53baa109 100644
@@ -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);
include/linux/libata.h +2 -1
index 5f6bbe5d504f..c400aa3af93f 100644
@@ -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);
Signature · 1 line
2.54.0
Reviewed-by on v1 3/3
#12 lore raw

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

Quoted 17 lines · Niklas Cassel

From: Bart Van Assche <[email protected]>

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 <[email protected]>
Co-developed-by Niklas Cassel <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
Quoted 8 lines
---
  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 <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +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
author maintainer Applied
#13 lore raw

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

In reply to Niklas Cassel
Quoted 8 lines · Niklas Cassel

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.

[...]

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

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

      https://git.kernel.org/libata/linux/c/ebfb3924
[3/3] ata: Annotate functions in the issuing path with __must_hold()
      https://git.kernel.org/libata/linux/c/28cee4cb

Kind regards,
Niklas