Re: [PATCH] ata: ahci: force 32-bit DMA for ASMedia ASM1166

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The 32-bit DMA quirk is applied only to the ASM1166, leaving identical sibling controllers (ASM1164 and ASM1165) exposed to catastrophic silent data corruption.
- [Critical] The `AHCI_HFLAG_32BIT_ONLY` quirk clears `HOST_CAP_64` from `hpriv->cap`, preventing the driver from explicitly zeroing the upper 32-bit DMA address registers, which can lead to DMA to corrupted physical addresses.
--

commit aa72273e536a2a474b07a2e0e23ffb50c8b94418
Author: Alvin Lim <alvinwylim@gmail.com>

ata: ahci: force 32-bit DMA for ASMedia ASM1166

This commit limits the ASMedia ASM1166 SATA controller to 32-bit DMA.
It addresses a regression that occurs when the IOMMU is enabled and hands
out DMA addresses above 4 GB, resulting in silent data corruption. A new
board_ahci_32bit_dma board type is introduced and applied to the ASM1166.

> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 58f512f8952a9..895956c2ca15f 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
[ ... ]
> @@ -132,6 +133,13 @@ static const struct ata_port_info ahci_port_info[] = {
>  		.udma_mask	= ATA_UDMA6,
>  		.port_ops	= &ahci_ops,
>  	},
> +	[board_ahci_32bit_dma] = {
> +		AHCI_HFLAGS	(AHCI_HFLAG_32BIT_ONLY),

[Severity: Critical]
This is a pre-existing issue, but does using AHCI_HFLAG_32BIT_ONLY here
expose a regression in ahci_start_fis_rx()?

When AHCI_HFLAG_32BIT_ONLY is set, ahci_save_initial_config() clears
HOST_CAP_64 from the capability flags.

Later, in ahci_start_fis_rx(), the hardware registers PORT_LST_ADDR_HI and
PORT_FIS_ADDR_HI are not zeroed out because the flag is cleared:

    if (hpriv->cap & HOST_CAP_64) {
        writel((pp->cmd_slot_dma >> 16) >> 16,
               port_mmio + PORT_LST_ADDR_HI);
        writel((pp->rx_fis_dma >> 16) >> 16,
               port_mmio + PORT_FIS_ADDR_HI);
    }

If the hardware is genuinely 64-bit capable and not fully reset, can it
retain stale upper 32-bit addresses left by the firmware, causing DMA to
target the wrong physical addresses when concatenated with the new lower
32 bits?

> +		.flags		= AHCI_FLAG_COMMON,
> +		.pio_mask	= ATA_PIO4,
> +		.udma_mask	= ATA_UDMA6,
> +		.port_ops	= &ahci_ops,
> +	},
[ ... ]
> @@ -1559,7 +1567,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  	}, {
>  		/* ASM1166 */
>  		PCI_VDEVICE(ASMEDIA, 0x1166),
> -		.driver_data = board_ahci,
> +		.driver_data = board_ahci_32bit_dma,
>  	}, {

[Severity: Critical]
This is a pre-existing issue, but does this quirk need to be applied to
sibling controllers as well?

Looking at ahci_pci_tbl, ASM1164 and ASM1165 are from the same silicon
series and still map to plain board_ahci:

    /* ASM1164 */
    PCI_VDEVICE(ASMEDIA, 0x1164),
    .driver_data = board_ahci,
    }, {
    /* ASM1165 */
    PCI_VDEVICE(ASMEDIA, 0x1165),
    .driver_data = board_ahci,

Does leaving them without the 32-bit DMA limit expose them to the same
regression where IOMMU addresses above 4 GB cause data corruption?

>  		/*
>  		 * Samsung SSDs found on some macbooks.  NCQ times out if MSI is

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