[PATCH] ahci: Marvell controllers prefer DMA for ATAPI

[PATCH] ahci: Marvell controllers prefer DMA for ATAPI

[Huacai Chen]

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

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@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

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

[Niklas Cassel]

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

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

[WangYuli]

Reported-by: WangYuli <wangyuli@uniontech.com>

Hi all,

During testing, we observed that reading CDs on an INSPUR CE3000F desktop
computer (with a Phytium FT2000/4 processor [1]) running deepin 23 [2]
distribution is very slow, taking over a minute.

Further testing revealed that mounting the device from the command line is
fast.
  e.g., executing "time mount /dev/sr0 /mnt" returns a value of only 0.898s.

However, deepin's dde-file-manager reads more than just mounting when opening
the optical drive; it also reads disc information (such as capacity, etc.).

After using udisksctl to first mount and then unmount the disc to reset its
state, and then using the cd_sessions tool to read disc information, we used
strace to monitor system calls, testing commands are as follows:
  udisksctl mount -b /dev/sr0
  udisksctl unmount -b /dev/sr0
  strace -Ttt cd_sessions /dev/sr0
and found that openat was blocked for 42 seconds, and at this point, the
process had entered kernel space.

[1]. https://www.phytium.com.cn/homepage/production/6/
[2]. https://cdimage.deepin.com/releases/23/arm64/deepin-desktop-community-23-arm64.iso


TEST REPORT:

For this patch, before its introduction, with original system-integrated optical
drives capable of recognizing discs, the total execution time for cd_sessions was
92.4 seconds, including 31.6 seconds for openat and 60.6 seconds for ioctl.

With LITE-ON optical drives, under the same condition of disc recognition, the
total execution time for cd_sessions was 108.2 seconds, with openat taking 45.4
seconds and ioctl taking 62.9 seconds.

With RITEK optical drives, also when discs were recognizable, the total
cd_sessions execution time was 98.2 seconds, with openat at 36.7 seconds and ioctl
at 60.8 seconds.

After the patch was introduced, the time to read disc information (using cd_sessions)
with all optical drives was reduced to less than 0.5 seconds.

I've tried different pairings of optical drives and SATA controllers, and the test
results were consistent across the board. The Marvell 88SE9215 controller consistently
emerges as the source of the problem, no matter the optical drive tested.

Devices lists are as follow:


CD-ROM DRIVER LIST:
  1. original system-integrated optical drive
           *-cdrom
                description: DVD writer
                product: DVD A  DH16AFSH
                vendor: ATAPI
                physical id: 0
                bus info: scsi@2:0.0.0
                logical name: /dev/cdrom
                logical name: /dev/sr0
                version: DC2M
                capabilities: removable audio cd-r cd-rw dvd dvd-r
                configuration: ansiversion=5 status=ready
              *-medium
                   physical id: 0
                   logical name: /dev/cdrom
  2. a LITE-ON optical drive
           *-cdrom
                description: DVD writer
                product: DVD-RW DH16AFSH
                vendor: PLDS
                physical id: 0
                bus info: scsi@2:0.0.0
                logical name: /dev/cdrom
                logical name: /dev/sr0
                version: DL3M
                capabilities: removable audio cd-r cd-rw dvd dvd-r
                configuration: ansiversion=5 status=nodisc
  3. a RITEK optical drive
           *-cdrom
                description: DVD writer
                product: DH-16AFSH SUPDRV
                vendor: RIDATA
                physical id: 0
                bus info: scsi@2:0.0.0
                logical name: /dev/cdrom
                logical name: /dev/sr0
                version: NWDN
                capabilities: removable audio cd-r cd-rw dvd dvd-r
                configuration: ansiversion=5 status=nodisc


SATA Controllers LIST:
  1. INSPUR CE3000F
        *-sata
             description: SATA controller
             product: 88SE9215 PCIe 2.0 x1 4-port SATA 6 Gb/s Controller
             vendor: Marvell Technology Group Ltd.
             physical id: 0
             bus info: pci@0000:03:00.0
             logical name: scsi2
             logical name: scsi3
             version: 11
             width: 32 bits
             clock: 33MHz
             capabilities: sata pm msi pciexpress ahci_1.0 bus_master cap_list rom emulated
             configuration: driver=ahci latency=0
             resources: irq:43 ioport:2020(size=8) ioport:2030(size=4) ioport:2028(size=8) ioport:2034(size=4) ioport:2000(size=32) memory:58140000-581407ff memory:58100000-5813ffff
  2. Maxsun MS-WS W680 D4
             Hardware Class: storage
             Model: "Intel SATA controller"
             Vendor: pci 0x8086 "Intel Corporation"
             Device: pci 0x7ae2 
             Revision: 0x11
             Driver: "ahci"
             Driver Modules: "ahci"
             Memory Range: 0x82100000-0x82101fff (rw,non-prefetchable)
             Memory Range: 0x82102800-0x821028ff (rw,non-prefetchable)
             Memory Range: 0x82102000-0x821027ff (rw,non-prefetchable)
             IRQ: 166 (no events)
             Module Alias: "pci:v00008086d00007AE2sv00000000sd00000000bc01sc06i01"
             Driver Info #0:
               Driver Status: ahci is active
               Driver Activation Cmd: "modprobe ahci"
             Config Status: cfg=new, avail=yes, need=no, active=unknown


Tested-by: Jie Fan <fanjie@uniontech.com>
Tested-by: Erpeng Xu <xuerpeng@uniontech.com>
Tested-by: WangYuli <wangyuli@uniontech.com>

Currently, due to the limited hardware I have access to, I'm unsure if this is a Marvell
88SE9215's specific issue or a general Marvell SATA controller problem.

So, I think it's reasonable to add a quirk to handle this for the Marvell 88SE9215 now.

Thanks,
--
WangYuli

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

[Niklas Cassel]

Hello WangYuli,

On Mon, Feb 17, 2025 at 03:01:20PM +0800, WangYuli wrote:

[...]

> Tested-by: Jie Fan <fanjie@uniontech.com>
> Tested-by: Erpeng Xu <xuerpeng@uniontech.com>
> Tested-by: WangYuli <wangyuli@uniontech.com>

It is a bit weird to see Tested-by tags here, since your email does not
contain an actual patch.


> 
> Currently, due to the limited hardware I have access to, I'm unsure if this is a Marvell
> 88SE9215's specific issue or a general Marvell SATA controller problem.
> 
> So, I think it's reasonable to add a quirk to handle this for the Marvell 88SE9215 now.

I agree.

Feel free to submit a patch that adds a quirks for Marvell 88SE9215.


Kind regards,
Niklas

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

[Huacai Chen]

Hi, Niklas,

On Mon, Feb 17, 2025 at 7:29 PM Niklas Cassel <cassel@kernel.org> wrote:
>
> Hello WangYuli,
>
> On Mon, Feb 17, 2025 at 03:01:20PM +0800, WangYuli wrote:
>
> [...]
>
> > Tested-by: Jie Fan <fanjie@uniontech.com>
> > Tested-by: Erpeng Xu <xuerpeng@uniontech.com>
> > Tested-by: WangYuli <wangyuli@uniontech.com>
>
> It is a bit weird to see Tested-by tags here, since your email does not
> contain an actual patch.
I think Yuli means I can add these names to the V2 patch, because I
only tested Loongson, others are tested by them.

>
>
> >
> > Currently, due to the limited hardware I have access to, I'm unsure if this is a Marvell
> > 88SE9215's specific issue or a general Marvell SATA controller problem.
> >
> > So, I think it's reasonable to add a quirk to handle this for the Marvell 88SE9215 now.
>
> I agree.
>
> Feel free to submit a patch that adds a quirks for Marvell 88SE9215.
OK, I will send V2 as soon as possible, sorry for the long delay.

Huacai

>
>
> Kind regards,
> Niklas