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@loongson.cn>
---
 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;
+
 	ahci_set_em_messages(hpriv, &pi);
 
 	if (ahci_broken_system_poweroff(pdev)) {
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 30932552437a..49d7d064f31b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3040,6 +3040,10 @@ int ata_dev_configure(struct ata_device *dev)
 		dev->max_sectors = ATA_MAX_SECTORS;
 	}
 
+	if ((dev->class == ATA_DEV_ATAPI) &&
+	    (ap->flags & ATA_FLAG_ATAPI_DMA))
+		dev->horkage |= ATA_HORKAGE_ATAPI_MOD16_DMA;
+
 	if ((dev->class == ATA_DEV_ATAPI) &&
 	    (atapi_command_packet_set(id) == TYPE_TAPE)) {
 		dev->max_sectors = ATA_MAX_SECTORS_TAPE;
@@ -4590,7 +4594,7 @@ int atapi_check_dma(struct ata_queued_cmd *qc)
 	 */
 	if (!(qc->dev->horkage & ATA_HORKAGE_ATAPI_MOD16_DMA) &&
 	    unlikely(qc->nbytes & 15))
-		return 1;
+		return -EOPNOTSUPP;
 
 	if (ap->ops->check_atapi_dma)
 		return ap->ops->check_atapi_dma(qc);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 17394098bee9..e2834c7564df 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -154,6 +154,7 @@ enum {
 					    /* (doesn't imply presence) */
 	ATA_FLAG_SATA		= (1 << 1),
 	ATA_FLAG_NO_LPM		= (1 << 2), /* host not happy with LPM */
+	ATA_FLAG_ATAPI_DMA	= (1 << 4), /* ATAPI use DMA */
 	ATA_FLAG_NO_LOG_PAGE	= (1 << 5), /* do not issue log page read */
 	ATA_FLAG_NO_ATAPI	= (1 << 6), /* No ATAPI support */
 	ATA_FLAG_PIO_DMA	= (1 << 7), /* PIO cmds via DMA */
-- 
2.43.5

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@loongson.cn>
> ---
>  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

On Mon, Sep 09, 2024 at 09:38:16AM +0200, Niklas Cassel wrote:
>
> Device specific quirks are defined in __ata_dev_quirks in libata-core.c.

Side note: the name "__ata_dev_quirks" is so far only queued up in:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.12

In older versions (and in mainline) it is still called "ata_device_blacklist".


Kind regards,
Niklas