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

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

[Bart Van Assche]

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/

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Changes compared to v2:
 - Instead of annotating only function declarations, annotate both function
   declarations and function definitions.
 - Changed __assume_ctx_lock() into lockdep_assert_held().
 - Left out the host lock changes because there is no agreement about how to
   annotate functions that expect that the host lock is held.

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.

 drivers/ata/Makefile         |  2 ++
 drivers/ata/ahci.h           |  3 ++-
 drivers/ata/ahci_imx.c       |  1 +
 drivers/ata/libahci.c        |  1 +
 drivers/ata/libata-core.c    |  1 +
 drivers/ata/libata-eh.c      |  8 ++++++++
 drivers/ata/libata-pmp.c     |  6 ++++++
 drivers/ata/libata-sff.c     |  2 ++
 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       | 15 ++++++++++-----
 21 files changed, 53 insertions(+), 10 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/libahci.c b/drivers/ata/libahci.c
index 0c5e5b29bce4..5abc6662107a 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2212,6 +2212,7 @@ static void ahci_thaw(struct ata_port *ap)
 }
 
 void ahci_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ahci_host_priv *hpriv = ap->host->private_data;
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3d25dec6ac6c..7e9faf2bceb3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6832,6 +6832,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 6cb79a09423d..f2aa4e91eff1 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -469,6 +469,7 @@ static void ata_eh_clear_action(struct ata_link *link, struct ata_device *dev,
  *	EH context.
  */
 void ata_eh_acquire(struct ata_port *ap)
+	__acquires(ap->host->eh_mutex)
 {
 	mutex_lock(&ap->host->eh_mutex);
 	WARN_ON_ONCE(ap->host->eh_owner);
@@ -486,6 +487,7 @@ void ata_eh_acquire(struct ata_port *ap)
  *	EH context.
  */
 void ata_eh_release(struct ata_port *ap)
+	__releases(ap->host->eh_mutex)
 {
 	WARN_ON_ONCE(ap->host->eh_owner != current);
 	ap->host->eh_owner = NULL;
@@ -2833,6 +2835,7 @@ static bool ata_eh_followup_srst_needed(struct ata_link *link, int rc)
 
 int ata_eh_reset(struct ata_link *link, int classify,
 		 struct ata_reset_operations *reset_ops)
+	__must_hold(&link->ap->host->eh_mutex)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_link *slave = ap->slave_link;
@@ -3816,6 +3819,7 @@ static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
  */
 int ata_eh_recover(struct ata_port *ap, struct ata_reset_operations *reset_ops,
 		   struct ata_link **r_failed_link)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ata_link *link;
 	struct ata_device *dev;
@@ -3879,6 +3883,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. */
+		lockdep_assert_held(&link->ap->host->eh_mutex);
+
 		if (!(ehc->i.action & ATA_EH_RESET))
 			continue;
 
@@ -4112,6 +4119,7 @@ void ata_eh_finish(struct ata_port *ap)
  *	Kernel thread context (may sleep).
  */
 void ata_std_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ata_reset_operations *reset_ops = &ap->ops->reset;
 	struct ata_link *link = &ap->link;
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index e3adc008fed1..3fc559f0d000 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. */
+	lockdep_assert_held(&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];
@@ -1084,6 +1089,7 @@ static int sata_pmp_eh_recover(struct ata_port *ap)
  *	Kernel thread context (may sleep).
  */
 void sata_pmp_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	ata_eh_autopsy(ap);
 	ata_eh_report(ap);
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 785b6e371abf..734e7c88439a 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2053,6 +2053,7 @@ EXPORT_SYMBOL_GPL(ata_sff_drain_fifo);
  *	Kernel thread context (may sleep)
  */
 void ata_sff_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ata_queued_cmd *qc;
 	unsigned long flags;
@@ -2769,6 +2770,7 @@ EXPORT_SYMBOL_GPL(ata_bmdma_interrupt);
  *	Kernel thread context (may sleep)
  */
 void ata_bmdma_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex)
 {
 	struct ata_queued_cmd *qc;
 	unsigned long flags;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index b5423b6e97de..b41601ad9d11 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -173,8 +173,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_queued_cmd *qc);
@@ -188,10 +190,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 64cb544903d8..4fc22ce4bd9a 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 be829fcc584d..70b210afd291 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 46a8c20daf18..065408975f5b 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 ffb396f61731..f6942fd80bae 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 2a005aede123..c5fad448bafc 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 cfb9b5b61cd7..1add7a7d5ff6 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 d642ece9f07a..89a10704e98e 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 0986ebd1eb4e..227dc1d4e85b 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 5c085ef4eda7..5f6bbe5d504f 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,7 +1419,8 @@ 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_error_handler(struct ata_port *ap)
+	__must_hold(&ap->host->eh_mutex);
 extern void ata_std_sched_eh(struct ata_port *ap);
 extern void ata_std_end_eh(struct ata_port *ap);
 extern int ata_link_nr_enabled(struct ata_link *link);
@@ -1998,7 +2000,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 +2065,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 +2096,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 v3] ata: libata: Document when host->eh_mutex should be held

[Niklas Cassel]

Hello Bart,

On Wed, May 27, 2026 at 01:25:20PM -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/
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
> Changes compared to v2:
>  - Instead of annotating only function declarations, annotate both function
>    declarations and function definitions.
>  - Changed __assume_ctx_lock() into lockdep_assert_held().
>  - Left out the host lock changes because there is no agreement about how to
>    annotate functions that expect that the host lock is held.

I am happy with this patch, and think that it is basically ready to be
merged as it is.

It only adds two lockdep_assert_held() calls, so that seems okay,
considering that it is not a crazy amount.


Now for ap->lock: It would be a shame if the work you did on ap->lock
annotations were to go to waste.

In:
https://lore.kernel.org/linux-ide/fcc80572-88da-4e16-ad09-69819f43cd73@acm.org/

You said that you would leave out the patches that adds an "ap" argument to
several functions. That sounds good.

If we go with the option to not add "ap" arguments everywhere, how many
lockdep_assert_held() calls would a patch that adds ap->lock annotations
need to add?

If it is not a crazy number, I think we should just do it.


Kind regards,
Niklas

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

[sashiko-bot]

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

New issues:
- [Low] Missing '&' operator in lock capability expression for `__acquires` and `__releases` annotations on `ata_eh_acquire` and `ata_eh_release`.
- [Low] The `ata_dummy_error_handler` function is missing the `__must_hold(&ap->host->eh_mutex)` annotation.

Pre-existing issues:
- [High] The `mv_pmp_error_handler` modifies `pp->pp_flags` and calls `ata_port_freeze(ap)` without holding the host spinlock, leading to data races with the interrupt handler and lockdep/API contract violations.
- [High] The `sil24_error_handler` clears `pp->do_port_rst` without holding the host spinlock after the port has been thawed, creating a race condition with the interrupt handler.
--

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

ata: libata: Document when host->eh_mutex should be held

Annotate various error handling functions with __must_hold(&host->eh_mutex)
to enable Clang's context analysis. This helps catch locking bugs in error
paths at compile time.

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 9a4b67b90b17b..7b3c3e20115e5 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -469,6 +469,7 @@ static void ata_eh_clear_action(struct ata_link *link, struct ata_device *dev,
>   *	EH context.
>   */
>  void ata_eh_acquire(struct ata_port *ap)
> +	__acquires(ap->host->eh_mutex)
>  {

[Severity: Low]
Since the internal locking functions and __must_hold expect a pointer,
should this use the address-of operator like __acquires(&ap->host->eh_mutex)?
Without it, this might cause a capability mismatch in Clang's Thread Safety
Analysis and lead to build failures since CONTEXT_ANALYSIS is enabled.

[ ... ]

> @@ -486,6 +487,7 @@ void ata_eh_acquire(struct ata_port *ap)
>   *	EH context.
>   */
>  void ata_eh_release(struct ata_port *ap)
> +	__releases(ap->host->eh_mutex)
>  {

[Severity: Low]
Similarly, should this be __releases(&ap->host->eh_mutex) instead?

[ ... ]

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index ffb396f61731f..f6942fd80bae7 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 this function modify pp->pp_flags
and call ata_port_freeze() without holding the host spinlock?

The error_handler callback is invoked with ap->host->eh_mutex held, but
without the host spinlock (ap->lock). However, mv_pmp_error_handler()
performs a read-modify-write on pp->pp_flags:

	if (pp->pp_flags & MV_PP_FLAG_DELAYED_EH) {
		pmp_map = pp->delayed_eh_pmp_map;
		pp->pp_flags &= ~MV_PP_FLAG_DELAYED_EH;

This can race with mv_err_intr() which modifies the same flags from the
interrupt handler while holding the spinlock.

Additionally, this calls ata_port_freeze(ap) directly, which requires
the host lock to be held according to its locking contract.

[ ... ]

> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index d642ece9f07a1..89a10704e98e5 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;

[Severity: High]
This is a pre-existing issue, but does clearing pp->do_port_rst here
race with the interrupt handler?

sil24_error_handler() delegates to sata_pmp_error_handler(), which recovers
the port and re-enables hardware interrupts:

	sata_pmp_error_handler(ap);
	pp->do_port_rst = 0;

If an error interrupt fires after the port is thawed but before
pp->do_port_rst = 0 executes, sil24_interrupt() could set pp->do_port_rst = 1
to request a controller reset. The EH thread would then overwrite this with 0
without holding the ap->lock spinlock, discarding the reset request.

[ ... ]

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 5c085ef4eda7b..5f6bbe5d504fa 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);

[Severity: Low]
Does ata_dummy_error_handler() also need to be annotated with __must_hold?

Since ata_dummy_error_handler() lacks this annotation, assigning it to
ata_dummy_port_ops.error_handler will cause a type/capability mismatch in
Clang's Thread Safety Analysis and break the build.

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

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

[Bart Van Assche]

On 5/27/26 1:55 PM, Niklas Cassel wrote:
> Now for ap->lock: It would be a shame if the work you did on ap->lock
> annotations were to go to waste.
> 
> In:
> https://lore.kernel.org/linux-ide/fcc80572-88da-4e16-ad09-69819f43cd73@acm.org/
> 
> You said that you would leave out the patches that adds an "ap" argument to
> several functions. That sounds good.
> 
> If we go with the option to not add "ap" arguments everywhere, how many
> lockdep_assert_held() calls would a patch that adds ap->lock annotations
> need to add?
> 
> If it is not a crazy number, I think we should just do it.

Hi Niklas,

Making these changes requires adding locking where it is missing. I
prefer that these changes are made by someone who has a setup on which
all the modified drivers can be tested.

Thanks,

Bart.

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

[Damien Le Moal]

On 5/28/26 05:25, 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/
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Bart,

Thanks for doing this.
I only have a couple of nits below. With that fixed, please add:

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

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

Please drop this comment in the 2 places you have it. The comment is confusing
because it does not describe at all what lockdep_assert_held() checks. No
comment is fine. lockdep_assert_held() as a name is clear enough.

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

Same thing here.

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

Hello Bart,

On Wed, May 27, 2026 at 08:57:11PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [Low] Missing '&' operator in lock capability expression for `__acquires` and `__releases` annotations on `ata_eh_acquire` and `ata_eh_release`.

Is this review comment are valid?

> - [Low] The `ata_dummy_error_handler` function is missing the `__must_hold(&ap->host->eh_mutex)` annotation.

I don't see any downside of also just annotating ata_dummy_error_handler().


Kind regards,
Niklas

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

[Hannes Reinecke]

On 5/27/26 22:25, 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/
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
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