* Re: [PATCHv2] ata: pata_ep93xx: add missing "cc" clobber to inline asm
2026-07-20 23:59 [PATCHv2] ata: pata_ep93xx: add missing "cc" clobber to inline asm Rosen Penev
@ 2026-07-21 0:12 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-21 0:12 UTC (permalink / raw)
To: Rosen Penev; +Cc: linux-ide, dlemoal, cassel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Trailing byte transfers in `ep93xx_pata_data_xfer()` silently corrupt data on big-endian systems due to incorrect endian swapping and variable truncation. For READ, the code assigns the 16-bit swapped result of `cpu_to_le16(ep93xx_pata_read_data(...))` to `*pad`, which is an 8-bit `unsigned char`. This truncates the 16-bit value. On big-endian systems, `cpu_to_le16()` swaps the bytes, so this truncation discards the lower byte (the correct data from the bus) and saves the upper byte to the buffer. For WRITE, `le16_to_cpu(*pad)` promotes the 8-bit byte to an integer before swapping it, resulting in a shifted 16-bit value, which causes the device to receive corrupted data on the wrong data lines.
- [High] The inline assembly delay loop `ep93xx_pata_delay()` lacks a `"memory"` clobber, permitting the compiler to reorder it with respect to I/O accesses, which violates ATA PIO timing requirements. On older ARM architectures like ARMv4T (used in EP93xx) where `CONFIG_ARM_DMA_MEM_BUFFERABLE` is not selected, the standard I/O macros (`__iormb()` and `__iowmb()`) evaluate to empty, meaning `writel()` and `readl()` provide no compiler memory barriers at all. Because `__raw_readl()` and `__raw_writel()` are implemented using `asm volatile` without `"memory"` clobbers, GCC's instruction scheduler is completely free to reorder these volatile I/O accesses across the `ep93xx_pata_delay()` loop. Even if memory barriers were present after the read or before the write, the `asm volatile` instructions themselves lack dependencies and can be swapped.
--
--- Patch [1]: [PATCHv2] ata: pata_ep93xx: add missing "cc" clobber to inline asm ---
commit e8afdf774c87485ee82a0fe97b982fc47fc827bc
Author: Rosen Penev <rosenp@gmail.com>
ata: pata_ep93xx: add missing "cc" clobber to inline asm
This commit adds a missing cc clobber to the inline assembly in
ep93xx_pata_delay() to prevent the compiler from reordering flag-dependent
instructions.
> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index 42a24dc51d26b..0bb748a86ac6e 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"
[Severity: High]
This isn't a bug introduced by this patch, but since you are updating the
clobbers in ep93xx_pata_delay(), should this inline assembly also include a
"memory" clobber?
On older ARM architectures like ARMv4T used in EP93xx, standard I/O macros
can evaluate to empty, providing no compiler memory barriers.
Without a memory clobber here, the compiler optimizer might be free to
reorder independent I/O accesses across this delay loop, which could violate
ATA PIO timing requirements.
> );
> #else
> while (count--)
[ ... ]
[Severity: Critical]
This is a pre-existing issue in the same file, but could the trailing byte
transfers in ep93xx_pata_data_xfer() silently corrupt data on big-endian
systems due to truncation?
For READ transfers, the code assigns a swapped value to an 8-bit variable:
drivers/ata/pata_ep93xx.c:ep93xx_pata_data_xfer() {
...
*pad = cpu_to_le16(ep93xx_pata_read_data(drv_data, IDECTRL_ADDR_DATA));
...
}
Since pad is an unsigned char pointer, this truncates the 16-bit swapped
result. On big-endian systems, cpu_to_le16() swaps the bytes, so truncating
it to 8 bits discards the lower byte containing the correct data and saves
the upper byte instead.
Similarly for WRITE transfers:
drivers/ata/pata_ep93xx.c:ep93xx_pata_data_xfer() {
...
ep93xx_pata_write_data(drv_data, le16_to_cpu(*pad), IDECTRL_ADDR_DATA);
...
}
Promoting the 8-bit pad value to an integer before swapping it on a
big-endian system seems to shift the byte to the wrong data lines, sending
corrupted data to the device.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720235952.1334318-1-rosenp@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread