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

[sashiko-bot@kernel.org]

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