Re: [PATCH v4] ata: libata: Document when host->eh_mutex should be held

[Niklas Cassel <cassel@kernel.org>]

On Thu, May 28, 2026 at 10:47:26AM -0700, Bart Van Assche wrote:
> Annotate the following functions with __must_hold(&host->eh_mutex):
>  * All ata_port_operations.error_handler() implementations.
>  * ata_eh_reset() and ata_eh_recover() because these functions call
>    ata_eh_release() and ata_eh_acquire().
>  * All callers of ata_eh_reset() and ata_eh_recover().
> 
> 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/
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

After spending the whole day playing with Clang 23, personally I do prefer
growing an ap parameter in ata_eh_reset() (in a patch 1/2), because it will
avoid us having to add two lockdep_assert_held() (which we know, from context
analysis PoV, just fools the compiler to say that we have the lock, without
actually verifying anything. (Yes, when running lockdep in runtime, it
would be verified.)

But at least with an ap parameter to ata_eh_reset(), context analysis will
actually work, and will allow us to verify things already in compile time.
No lockdep needed. Don't get me wrong, lockdep is awesome, but if we can
verify things already in compile time in an easy way, that seems way better
IMO. Let's leave the runtime checking for things that we can't trivially
verify during compile time.

I.e. the diff would be something like this compared to your V4:


diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 8b5fd2a48660..a66663d3579e 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2833,11 +2833,10 @@ static bool ata_eh_followup_srst_needed(struct ata_link *link, int rc)
 	return false;
 }
 
-int ata_eh_reset(struct ata_link *link, int classify,
+int ata_eh_reset(struct ata_port *ap, struct ata_link *link, int classify,
 		 struct ata_reset_operations *reset_ops)
-	__must_hold(&link->ap->host->eh_mutex)
+	__must_hold(&ap->host->eh_mutex)
 {
-	struct ata_port *ap = link->ap;
 	struct ata_link *slave = ap->slave_link;
 	struct ata_eh_context *ehc = &link->eh_context;
 	struct ata_eh_context *sehc = slave ? &slave->eh_context : NULL;
@@ -3883,12 +3882,11 @@ 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;
 
-		lockdep_assert_held(&link->ap->host->eh_mutex);
-
 		if (!(ehc->i.action & ATA_EH_RESET))
 			continue;
 
-		rc = ata_eh_reset(link, ata_link_nr_vacant(link), reset_ops);
+		rc = ata_eh_reset(ap, link, ata_link_nr_vacant(link),
+				  reset_ops);
 		if (rc) {
 			ata_link_err(link, "reset failed, giving up\n");
 			goto out;
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index bf40633f8fb2..188fc12c2704 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -751,8 +751,6 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
 	int detach = 0, rc = 0;
 	int reval_failed = 0;
 
-	lockdep_assert_held(&ap->link.ap->host->eh_mutex);
-
 	if (dev->flags & ATA_DFLAG_DETACH) {
 		detach = 1;
 		rc = -ENODEV;
@@ -766,7 +764,7 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
 		struct ata_link *tlink;
 
 		/* reset */
-		rc = ata_eh_reset(link, 0, reset_ops);
+		rc = ata_eh_reset(ap, link, 0, reset_ops);
 		if (rc) {
 			ata_link_err(link, "failed to reset PMP, giving up\n");
 			goto fail;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 9e3817a1b143..0dd735c2e5b5 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -192,9 +192,9 @@ extern void ata_eh_done(struct ata_link *link, struct ata_device *dev,
 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)
-	__must_hold(&link->ap->host->eh_mutex);
+extern int ata_eh_reset(struct ata_port *ap, struct ata_link *link,
+			int classify, struct ata_reset_operations *reset_ops)
+	__must_hold(&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)