[PATCH] ata: libata: Add debug dump around error handling and link resets

[PATCH] ata: libata: Add debug dump around error handling and link resets

[Junxian Huang]

Add a generic debug_dump() operation to struct ata_port_operations
so LLDDs can dump their own controller debug state around error
handling.

For the standard AHCI driver, implement this hook to dump the AHCI
controller registers through tracepoints:

  ahci_global_softreset_start / _done   - HBA global registers
  ahci_global_hardreset_start / _done
  ahci_global_eh_start / _done
  ahci_port_softreset_start / _done     - port registers
  ahci_port_hardreset_start / _done
  ahci_port_eh_start / _done

These tracepoints help debug error and link reset issues.

Example usage:

  $ echo 1 > /sys/kernel/tracing/events/libahci/ahci_global_eh_start/enable
  $ echo 1 > /sys/kernel/tracing/events/libahci/ahci_port_eh_start/enable
  ... trigger an error ...
  $ cat /sys/kernel/tracing/trace
  scsi_eh_4-899 [024] 717.683782: ahci_global_eh_start: ata_port=5 \
    global={0xef377fe1,...}
  scsi_eh_4-899 [024] 717.683808: ahci_port_eh_start: ata_port=5 \
    port={0xfffc0000,...}

Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 drivers/ata/libahci.c          |  57 +++++++++++++++++
 drivers/ata/libata-eh.c        |   6 ++
 include/linux/libata.h         |   6 ++
 include/trace/events/libahci.h | 114 +++++++++++++++++++++++++++++++++
 4 files changed, 183 insertions(+)
 create mode 100644 include/trace/events/libahci.h

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 08ab56e95566..4e1a327d8e68 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -30,6 +30,10 @@
 #include <scsi/scsi_cmnd.h>
 #include <linux/libata.h>
 #include <linux/pci.h>
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/libahci.h>
+
 #include "ahci.h"
 #include "libata.h"
 
@@ -50,6 +54,7 @@ static ssize_t ahci_led_store(struct ata_port *ap, const char *buf,
 			      size_t size);
 static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 					ssize_t size);
+static void ahci_debug_dump(struct ata_port *ap, enum ata_eh_dump phase);
 
 
 
@@ -168,6 +173,7 @@ struct ata_port_operations ahci_ops = {
 	.reset.postreset	= ahci_postreset,
 	.pmp_reset.softreset	= ahci_softreset,
 	.error_handler		= ahci_error_handler,
+	.debug_dump		= ahci_debug_dump,
 	.post_internal_cmd	= ahci_post_internal_cmd,
 	.dev_config		= ahci_dev_config,
 
@@ -1457,6 +1463,49 @@ static int ahci_exec_polled_cmd(struct ata_port *ap, int pmp,
 	return 0;
 }
 
+#define AHCI_DUMP_FN(site)					\
+static void ahci_dump_##site(struct ata_link *link)		\
+{								\
+	struct ahci_host_priv *hpriv = link->ap->host->private_data; \
+	void __iomem *port_mmio = ahci_port_base(link->ap);	\
+	int i;							\
+								\
+	if (trace_ahci_global_##site##_enabled()) {		\
+		u32 global_regs[AHCI_TRACE_GLOBAL_REGS];	\
+								\
+		for (i = 0; i < AHCI_TRACE_GLOBAL_REGS; i++)	\
+			global_regs[i] = readl(hpriv->mmio + i * 4); \
+								\
+		trace_ahci_global_##site(link, global_regs);	\
+	}							\
+								\
+	if (trace_ahci_port_##site##_enabled()) {		\
+		u32 port_regs[AHCI_TRACE_PORT_REGS];		\
+								\
+		for (i = 0; i < AHCI_TRACE_PORT_REGS; i++)	\
+			port_regs[i] = readl(port_mmio + i * 4); \
+								\
+		trace_ahci_port_##site(link, port_regs);	\
+	}							\
+}
+
+AHCI_DUMP_FN(softreset_start)
+AHCI_DUMP_FN(softreset_done)
+AHCI_DUMP_FN(hardreset_start)
+AHCI_DUMP_FN(hardreset_done)
+AHCI_DUMP_FN(eh_start)
+AHCI_DUMP_FN(eh_done)
+#undef AHCI_DUMP_FN
+
+static void ahci_debug_dump(struct ata_port *ap,
+			      enum ata_eh_dump phase)
+{
+	if (phase == ATA_EH_DUMP_ENTER)
+		ahci_dump_eh_start(&ap->link);
+	else
+		ahci_dump_eh_done(&ap->link);
+}
+
 int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 		      int pmp, unsigned long deadline,
 		      int (*check_ready)(struct ata_link *link))
@@ -1471,6 +1520,8 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 	bool fbs_disabled = false;
 	int rc;
 
+	ahci_dump_softreset_start(link);
+
 	/* prepare for SRST (AHCI-1.1 10.4.1) */
 	rc = ahci_kick_engine(ap);
 	if (rc && rc != -EOPNOTSUPP)
@@ -1530,6 +1581,8 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 	if (fbs_disabled)
 		ahci_enable_fbs(ap);
 
+	ahci_dump_softreset_done(link);
+
 	return 0;
 
  fail:
@@ -1613,6 +1666,8 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
 	struct ata_taskfile tf;
 	int rc;
 
+	ahci_dump_hardreset_start(link);
+
 	hpriv->stop_engine(ap);
 
 	/* clear D2H reception area to properly wait for D2H FIS */
@@ -1630,6 +1685,8 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
 	if (*online)
 		*class = ahci_dev_classify(ap);
 
+	ahci_dump_hardreset_done(link);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_do_hardreset);
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c93c24b22d9c..a8eb651ffb60 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -573,6 +573,9 @@ void ata_scsi_error(struct Scsi_Host *host)
 	int nr_timedout;
 	LIST_HEAD(eh_work_q);
 
+	if (ap->ops->debug_dump)
+		ap->ops->debug_dump(ap, ATA_EH_DUMP_ENTER);
+
 	spin_lock_irqsave(host->host_lock, flags);
 	list_splice_init(&host->eh_cmd_q, &eh_work_q);
 	spin_unlock_irqrestore(host->host_lock, flags);
@@ -591,6 +594,9 @@ void ata_scsi_error(struct Scsi_Host *host)
 		scsi_eh_flush_done_q(&ap->eh_done_q);
 
 	WARN_ON(!list_empty(&eh_work_q));
+
+	if (ap->ops->debug_dump)
+		ap->ops->debug_dump(ap, ATA_EH_DUMP_EXIT);
 }
 
 /**
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 313e96173b19..71b658dd4bdb 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -966,6 +966,11 @@ struct ata_reset_operations {
 	ata_postreset_fn_t	postreset;
 };
 
+enum ata_eh_dump {
+	ATA_EH_DUMP_ENTER,
+	ATA_EH_DUMP_EXIT,
+};
+
 struct ata_port_operations {
 	/*
 	 * Command execution
@@ -1000,6 +1005,7 @@ struct ata_port_operations {
 	void (*post_internal_cmd)(struct ata_queued_cmd *qc);
 	void (*sched_eh)(struct ata_port *ap);
 	void (*end_eh)(struct ata_port *ap);
+	void (*debug_dump)(struct ata_port *ap, enum ata_eh_dump phase);
 
 	/*
 	 * Optional features
diff --git a/include/trace/events/libahci.h b/include/trace/events/libahci.h
new file mode 100644
index 000000000000..33ff2487cc84
--- /dev/null
+++ b/include/trace/events/libahci.h
@@ -0,0 +1,114 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2026 Hisilicon Limited.
+ */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM libahci
+
+#if !defined(_TRACE_LIBAHCI_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_LIBAHCI_H
+
+#include <linux/libata.h>
+#include <linux/tracepoint.h>
+
+#define AHCI_TRACE_GLOBAL_REGS	64
+#define AHCI_TRACE_PORT_REGS	32
+
+DECLARE_EVENT_CLASS(ahci_global_reg_dump,
+
+	TP_PROTO(struct ata_link *link, const u32 *global_regs),
+
+	TP_ARGS(link, global_regs),
+
+	TP_STRUCT__entry(
+		__field(unsigned int, ata_port)
+		__array(u32, global_regs, AHCI_TRACE_GLOBAL_REGS)
+	),
+
+	TP_fast_assign(
+		__entry->ata_port = link->ap->print_id;
+		memcpy(__entry->global_regs, global_regs,
+		       sizeof(__entry->global_regs));
+	),
+
+	TP_printk("ata_port=%u global=%s",
+		  __entry->ata_port,
+		  __print_array(__entry->global_regs,
+				AHCI_TRACE_GLOBAL_REGS, 4))
+);
+
+DEFINE_EVENT(ahci_global_reg_dump, ahci_global_softreset_start,
+	     TP_PROTO(struct ata_link *link, const u32 *global_regs),
+	     TP_ARGS(link, global_regs));
+
+DEFINE_EVENT(ahci_global_reg_dump, ahci_global_softreset_done,
+	     TP_PROTO(struct ata_link *link, const u32 *global_regs),
+	     TP_ARGS(link, global_regs));
+
+DEFINE_EVENT(ahci_global_reg_dump, ahci_global_hardreset_start,
+	     TP_PROTO(struct ata_link *link, const u32 *global_regs),
+	     TP_ARGS(link, global_regs));
+
+DEFINE_EVENT(ahci_global_reg_dump, ahci_global_hardreset_done,
+	     TP_PROTO(struct ata_link *link, const u32 *global_regs),
+	     TP_ARGS(link, global_regs));
+
+DEFINE_EVENT(ahci_global_reg_dump, ahci_global_eh_start,
+	     TP_PROTO(struct ata_link *link, const u32 *global_regs),
+	     TP_ARGS(link, global_regs));
+
+DEFINE_EVENT(ahci_global_reg_dump, ahci_global_eh_done,
+	     TP_PROTO(struct ata_link *link, const u32 *global_regs),
+	     TP_ARGS(link, global_regs));
+
+DECLARE_EVENT_CLASS(ahci_port_reg_dump,
+
+	TP_PROTO(struct ata_link *link, const u32 *port_regs),
+
+	TP_ARGS(link, port_regs),
+
+	TP_STRUCT__entry(
+		__field(unsigned int, ata_port)
+		__array(u32, port_regs, AHCI_TRACE_PORT_REGS)
+	),
+
+	TP_fast_assign(
+		__entry->ata_port = link->ap->print_id;
+		memcpy(__entry->port_regs, port_regs,
+		       sizeof(__entry->port_regs));
+	),
+
+	TP_printk("ata_port=%u port=%s",
+		  __entry->ata_port,
+		  __print_array(__entry->port_regs,
+				AHCI_TRACE_PORT_REGS, 4))
+);
+
+DEFINE_EVENT(ahci_port_reg_dump, ahci_port_softreset_start,
+	     TP_PROTO(struct ata_link *link, const u32 *port_regs),
+	     TP_ARGS(link, port_regs));
+
+DEFINE_EVENT(ahci_port_reg_dump, ahci_port_softreset_done,
+	     TP_PROTO(struct ata_link *link, const u32 *port_regs),
+	     TP_ARGS(link, port_regs));
+
+DEFINE_EVENT(ahci_port_reg_dump, ahci_port_hardreset_start,
+	     TP_PROTO(struct ata_link *link, const u32 *port_regs),
+	     TP_ARGS(link, port_regs));
+
+DEFINE_EVENT(ahci_port_reg_dump, ahci_port_hardreset_done,
+	     TP_PROTO(struct ata_link *link, const u32 *port_regs),
+	     TP_ARGS(link, port_regs));
+
+DEFINE_EVENT(ahci_port_reg_dump, ahci_port_eh_start,
+	     TP_PROTO(struct ata_link *link, const u32 *port_regs),
+	     TP_ARGS(link, port_regs));
+
+DEFINE_EVENT(ahci_port_reg_dump, ahci_port_eh_done,
+	     TP_PROTO(struct ata_link *link, const u32 *port_regs),
+	     TP_ARGS(link, port_regs));
+
+#endif /* _TRACE_LIBAHCI_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
-- 
2.33.0

Re: [PATCH] ata: libata: Add debug dump around error handling and link resets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] libahci: unmapped MMIO reads cause Data Abort on ARM platforms
- [Low] ata: missing ahci_dump_softreset_done() on softreset error path
--

commit 6261261e408a96bdca27f7aab27ae104e6a98735
Author: Junxian Huang <huangjunxian6@hisilicon.com>

ata: libata: Add debug dump around error handling and link resets

Add a generic debug_dump() operation to struct ata_port_operations so LLDDs
can dump their own controller debug state around error handling. For the
standard AHCI driver, implement this hook to dump the AHCI controller
registers through tracepoints.

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 08ab56e955662..4e1a327d8e680 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c

[ ... ]

> @@ -1457,6 +1463,49 @@ static int ahci_exec_polled_cmd(struct ata_port *ap, int pmp,
>  	return 0;
>  }
>  
> +#define AHCI_DUMP_FN(site)					\
> +static void ahci_dump_##site(struct ata_link *link)		\
> +{								\
> +	struct ahci_host_priv *hpriv = link->ap->host->private_data; \
> +	void __iomem *port_mmio = ahci_port_base(link->ap);	\
> +	int i;							\
> +								\
> +	if (trace_ahci_global_##site##_enabled()) {		\
> +		u32 global_regs[AHCI_TRACE_GLOBAL_REGS];	\
> +								\
> +		for (i = 0; i < AHCI_TRACE_GLOBAL_REGS; i++)	\
> +			global_regs[i] = readl(hpriv->mmio + i * 4); \

[Severity: High]
Does blindly reading global registers beyond implemented standard offsets
trigger bus errors on many ARM platforms?

On embedded ARM platforms using libahci, the AHCI IP is mapped on an AXI/AHB
bus which will commonly respond with a DECERR or SLVERR when accessing
unmapped, reserved, or unimplemented vendor-specific offsets.

This translates to an asynchronous SError or synchronous Data Abort exception,
causing a kernel panic when the tracepoint is enabled. Additionally, reading
vendor-specific registers without knowing their semantics can trigger
read-to-clear or state-altering side effects.

> +								\
> +		trace_ahci_global_##site(link, global_regs);	\
> +	}							\
> +								\
> +	if (trace_ahci_port_##site##_enabled()) {		\
> +		u32 port_regs[AHCI_TRACE_PORT_REGS];		\
> +								\
> +		for (i = 0; i < AHCI_TRACE_PORT_REGS; i++)	\
> +			port_regs[i] = readl(port_mmio + i * 4); \

[Severity: High]
Similarly, can blindly reading port registers up to offset 0x7C risk
accessing unimplemented offsets, causing SError or Data Abort on ARM?

> +								\
> +		trace_ahci_port_##site(link, port_regs);	\
> +	}							\
> +}

[ ... ]

> @@ -1530,6 +1581,8 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>  	if (fbs_disabled)
>  		ahci_enable_fbs(ap);
>  
> +	ahci_dump_softreset_done(link);
> +
>  	return 0;
>  
>   fail:
>  	ata_link_err(link, "softreset failed (%s)\n", reason);
>  	return rc;
>  }

[Severity: Low]
Is the tracepoint ahci_dump_softreset_done() missing in the error path?

If ahci_do_softreset() encounters an error and jumps to the fail label,
ahci_dump_softreset_done() is not emitted. This results in unbalanced trace
events and loses valuable hardware debug state right when it is most needed
to debug the failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016085657.3703353-1-huangjunxian6@hisilicon.com?part=1