Re: [PATCHv2] ata: pata_ep93xx: add missing "cc" clobber to inline asm

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 <[email protected]>

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/[email protected]?part=1