[PATCHv3] ata: pata_ep93xx: add missing "cc" and "memory" clobbers to inline asm

[PATCHv3] ata: pata_ep93xx: add missing "cc" and "memory" clobbers to inline asm

[Rosen Penev]

The inline assembly in ep93xx_pata_delay() uses a "subs" instruction
which modifies the ARM condition code flags. Add a "cc" clobber so
the compiler knows the flags are altered. Without it, the compiler may
reorder flag-dependent instructions across the asm block when the
function is inlined, potentially causing incorrect control flow.

Additionally, add a "memory" clobber to prevent the compiler from
reordering I/O accesses across this delay loop. On older ARM
architectures like ARMv4T used in EP93xx, __iormb()/__iowmb() may
evaluate to empty when CONFIG_ARM_DMA_MEM_BUFFERABLE is not set,
leaving writel()/readl() without a compiler memory barrier. Without
a "memory" clobber here, the compiler optimizer might reorder
independent I/O accesses across the delay, violating ATA PIO timing
requirements. This also brings the ARM path in line with the non-ARM
fallback which uses cpu_relax() (a compiler barrier).

Fixes: 2fff27512600 ("PATA host controller driver for ep93xx")
Assisted-by: Opencode:Big-Pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v3: add memory barrier
 v2: add Fixes tag
 drivers/ata/pata_ep93xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index 587caadf78cb..6d948e41bc59 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -211,6 +211,7 @@ static void ep93xx_pata_delay(unsigned long count)
 		"bge 0b\n"
 		: "=r" (count)
 		: "0" (count)
+		: "cc", "memory"
 	);
 #else
 	while (count--)
-- 
2.55.0

Re: [PATCHv3] ata: pata_ep93xx: add missing "cc" and "memory" clobbers to inline asm

[sashiko-bot]

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

Pre-existing issues:
- [High] Double free and ERR_PTR dereference in DMA initialization error path.
--

--- Patch [1]: [PATCHv3] ata: pata_ep93xx: add missing "cc" and "memory" clobbers to inline asm ---
commit 05e8a0e6eff7bdb5719b3840e3f01ca22df401f7
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: add missing "cc" and "memory" clobbers to inline asm

This commit adds "cc" and "memory" clobbers to the inline assembly in
ep93xx_pata_delay(). It prevents compiler optimizations from incorrectly
reordering flag-dependent instructions or I/O accesses.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index 42a24dc51d26b..813e72425e6f7 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the error path in
ep93xx_pata_dma_init() result in a double-free and an invalid pointer
dereference if tx DMA setup fails?

When dma_request_chan(&pdev->dev, "tx") fails, it returns an ERR_PTR
which is assigned to drv_data->dma_tx_channel. The code then jumps to
fail_release_rx:

drivers/ata/pata_ep93xx.c:ep93xx_pata_dma_init() {
    ...
	drv_data->dma_tx_channel = dma_request_chan(&pdev->dev, "tx");
	if (IS_ERR(drv_data->dma_tx_channel)) {
		ret = dev_err_probe(dev, PTR_ERR(drv_data->dma_tx_channel),
				    "tx DMA setup failed\n");
		goto fail_release_rx;
	}
    ...
fail_release_rx:
	dma_release_channel(drv_data->dma_rx_channel);
fail_release_dma:
	ep93xx_pata_release_dma(drv_data);

	return ret;
}

In fail_release_rx, dma_release_channel() is called but
drv_data->dma_rx_channel is not set to NULL.

The code then falls through to ep93xx_pata_release_dma():

drivers/ata/pata_ep93xx.c:ep93xx_pata_release_dma() {
	if (drv_data->dma_rx_channel) {
		dma_release_channel(drv_data->dma_rx_channel);
		drv_data->dma_rx_channel = NULL;
	}
	if (drv_data->dma_tx_channel) {
		dma_release_channel(drv_data->dma_tx_channel);
		drv_data->dma_tx_channel = NULL;
	}
}

Will this cause dma_release_channel() to be called a second time for
dma_rx_channel since the pointer is still truthy?

Additionally, since dma_tx_channel contains an ERR_PTR rather than NULL,
won't the truthy check pass and cause an invalid pointer dereference in
dma_release_channel()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731205338.8011-1-rosenp@gmail.com?part=1