Re: [PATCH] ahci: Marvell controllers prefer DMA for ATAPI

[Niklas Cassel]

On Sun, Sep 08, 2024 at 05:46:04PM +0800, Huacai Chen wrote:
> We use Marvell CD/DVD controllers on many Loongson-based machines. We
> found its PIO doesn't work well, and on the opposite its DMA seems work
> very well. We don't know the detail of the controller, but we can set
> the ATA_FLAG_ATAPI_DMA and ATA_HORKAGE_ATAPI_MOD16_DMA flags on these
> controllers to prefer ATAPI DMA.
> 
> BTW, return -EOPNOTSUPP instead of 1 if ATAPI DMA is not supported in
> atapi_check_dma().
> 
> Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> ---
>  drivers/ata/ahci.c        | 3 +++
>  drivers/ata/libata-core.c | 6 +++++-
>  include/linux/libata.h    | 1 +
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index a05c17249448..b195e87e7109 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1939,6 +1939,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (hpriv->cap & HOST_CAP_PMP)
>  		pi.flags |= ATA_FLAG_PMP;
>  
> +	if (pdev->vendor == PCI_VENDOR_ID_MARVELL_EXT)
> +		pi.flags |= ATA_FLAG_ATAPI_DMA;
> +

Hello Huacai,

You are not providing a lot of information about:
1) The SATA controller.
2) The CD/DVD drive that you are using.


For 1), since you are patching ahci_init_one(), it appears to be a
AHCI controller from Marvell.

However, we do not write quirks that affect all PCI device IDs
for a specific vendor.

Please define a new board type in "enum board_ids" in ahci.c, e.g.
something like board_ahci_atapi_force_dma or board_ahci_atapi_prefer_dma,
and then add specific PCI vendor IDs and device IDs in ahci_pci_tbl that
should apply this quirk.


For 2), you are not giving us any information, so have you verified that
this problem happens with more than one specific CD/DVD drive model?

It would be interesting to know if the problem exists even if you
are using CD/DVD drives from different vendors.

If the problem is only for a specific drive model, then perhaps this
shouldn't be a controller quirk, but rather a device quirk?
Device specific quirks are defined in __ata_dev_quirks in libata-core.c.


Kind regards,
Niklas