On Tue, Aug 17, 2021 at 07:45:58AM +0000, Damien Le Moal wrote:

On 2021/08/17 2:26, Reimar Döffinger wrote:

quoted

The patch is a best-guess + "in doubt disable" attempt at
auto-detecting.
As I mentioned the data is rather light, so I wanted to only add the
option at first.
Now it does 2 things in addition:
- disable it for Crucial MX500 because the issue was seen and confirmed
  with and without PATA converter with that device
- disable it for anything connected with a PATA converter because there
  are lots of bug reports with different devices that have a PATA
  converter in common. Unfortunately no confirmation from any of those.
  However in quick testing I could verify the MX500 issue only with
  PATA adapter myself, so it seems like an appropriate precaution,
  especially as now with the new code it is possible to override both
  ways.

How can you detect the presence of a PATA converter ? Is it what "ap_flags &
ATA_FLAG_SATA" indicates ?

Well, if my understanding is right when ATA_FLAG_SATA is not set then
it means it is a PATA controller.
So if someone has a PATA device that supports READ_LOG_DMA then it
would be disabled even in that case, but I had the impression
that command is new enough for this to be not very likely (and
I am guessing most PATA users care more about things working
than getting the best possible performance).

quoted

Aside: I don't have the ATA spec, so I can't check, but I do find it
a bit suspicious that the logs look like the code runs the
READ_LOG_DMA command before SETXFERMODE is done, and thus while the
device is still in PIO mode?

This is a bit off-topic for the patch, so you can leave me to try and
figure it out myself, but I am afraid your explanation does not quite
add up to me:

ata_read_log_page() has a retry loop: it first tries ATA_CMD_READ_LOG_DMA_EXT
and if that fails, it goes with PIO ATA_CMD_READ_LOG_EXT. So there is no problem
for non-buggy drives with this. Note that initially, dev->dma_mode may not be
set, so PIO is done directly.

Hm, I see that might have been the intent, but that does not seem to be
the case.
The condition is:

if (dev->dma_mode &&

However the correct way to check is to use ata_dma_enabled.
Which is:
return (adev->dma_mode == 0xFF ? 0 : 1);
So kind of the opposite (actually I think dev->dma_mode is always true),
and matching that ata_bus_probe sets dev->dma_mode = 0xff;
Now ata_do_set_mode would set up the proper value, but the little
problem is that that one is never called for SATA (and quite a few
PATA?) controllers.
So it seems there is no working way to detect if dma is enabled or not?
Unless the initialization value after reset would need to depend on
the controller, setting it to 0xff for no dma for PATA and some "random"
UDMA setting (or a newly added DMA_SATA one) for SATA controllers?

Once set, DMA version will be used unless the
horkage bit is set. So this all works as expected, and there will (normally) be
no failed command even when DMA is not yet set.

A typical log looks as below, now I admit it is very confusing because
it says the mode is 6.0 Gbps which is not even possible with PATA,
so not really clear which mode is active at which point.
However I think that DMA is only properly up after ata_do_set_mode (however
that is not used at all by most controllers it seems?), which
is the "failed to set xfermode", however that is long after the
"READ LOG DMA EXT failed".

[    1.245078] ata5: SATA max UDMA/133 abar m2048@0xc161b000 port 0xc161b300 irq 28
[    1.557761] ata5: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[    1.557929] ata5.00: FORCE: horkage modified (noncq)
[    1.558096] ata5.00: ACPI cmd ef/10:09:00:00:00:b0 (SET FEATURES) succeeded
[    1.558101] ata5.00: ACPI cmd ef/10:03:00:00:00:b0 (SET FEATURES) filtered out
[    1.558250] ata5.00: ATA-10: Lenovo SPEED UP-CL-240GB, V2.7, max UDMA/133
[    1.558253] ata5.00: 468862128 sectors, multi 0: LBA48 NCQ (not used)
[    6.663379] ata5.00: qc timeout (cmd 0x47)
[    6.663390] ata5.00: READ LOG DMA EXT failed, trying PIO
[    6.663394] ata5.00: failed to get Identify Device Data, Emask 0x40
[    6.663407] ata5.00: failed to set xfermode (err_mask=0x40)
[    6.977799] ata5: SATA link up 6.0 Gbps (SStatus 133 SControl 300)