The ATA_CMD_READ_LOG_DMA_EXT can cause controller/device to
become unresponsive until the next power cycle.
This seems to particularly affect IDE to SATA adapters,
possibly in combination with certain SATA SSDs, though
there might be more/different cases.
Comment 5 of https://bugzilla.kernel.org/show_bug.cgi?id=195895
is an example.
Having an option to disable its use allows booting systems affected,
which besides being a useful workaround is also a necessary
step to allow gathering more debug info on these systems.
Existing workarounds like forcing PIO mode do not work
(in addition to the performance issues) because READ_LOG_DMA
is issued even if PIO mode is forced.
Signed-off-by: Reimar Döffinger <redacted>
---
Documentation/admin-guide/kernel-parameters.txt | 2 ++
drivers/ata/libata-core.c | 2 ++
2 files changed, 4 insertions(+)
@@ -2551,6 +2551,8 @@ * [no]ncqtrim: Turn off queued DSM TRIM.+ * [no]dmalog: Turn off use of ATA_CMD_READ_LOG_DMA_EXT (0x47) command+ * nohrst, nosrst, norst: suppress hard, soft and both resets.
The ATA_CMD_READ_LOG_DMA_EXT can cause controller/device to
become unresponsive until the next power cycle.
This seems to particularly affect IDE to SATA adapters,
possibly in combination with certain SATA SSDs, though
there might be more/different cases.
Comment 5 of https://bugzilla.kernel.org/show_bug.cgi?id=195895
is an example.
Disable it by default on Crucial MX500 devices and all
PATA controllers.
Also add an option to set the workaround state, which might
help with gathering more data on the exact devices/controllers
affected, and speed up narrowing down the cause for users that
are affect but not covered by the added quirks.
Existing workarounds like forcing PIO mode do not work
(in addition to the performance issues) because READ_LOG_DMA
is issued even if PIO mode is forced.
Signed-off-by: Reimar Döffinger <redacted>
---
Documentation/admin-guide/kernel-parameters.txt | 2 ++
drivers/ata/libata-core.c | 15 +++++++++++++++
2 files changed, 17 insertions(+)
@@ -2551,6 +2551,8 @@ * [no]ncqtrim: Turn off queued DSM TRIM.+ * [no]dmalog: Turn off use of ATA_CMD_READ_LOG_DMA_EXT (0x47) command+ * nohrst, nosrst, norst: suppress hard, soft and both resets.
@@ -2004,7 +2004,11 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,retry:ata_tf_init(dev,&tf);+/* Do not even attempt DMA with PATA-SATA adapters, they seem likely to+*hang,seehttps://bugzilla.kernel.org/show_bug.cgi?id=195895+*/if(dev->dma_mode&&ata_id_has_read_log_dma_ext(dev->id)&&+(ap_flags&ATA_FLAG_SATA)&&!(dev->horkage&ATA_HORKAGE_NO_DMA_LOG)){tf.command=ATA_CMD_READ_LOG_DMA_EXT;tf.protocol=ATA_PROT_DMA;
@@ -4000,6 +4004,15 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {{"WDC WD3000JD-*",NULL,ATA_HORKAGE_WD_BROKEN_LPM},{"WDC WD3200JD-*",NULL,ATA_HORKAGE_WD_BROKEN_LPM},+/* Devices with observed READ LOG DMA issues - unclear if only+*specificfirmwareversionsoronlyincombinationwithspecific+*controllers.+*Specificallybrokencombinationsreported+*CT1000MX500SSD4,M3CR020withB350M-Mortar+*CT500MX500SSD4,M3CR023withPATA-SATAadapter+*https://bugzilla.kernel.org/show_bug.cgi?id=195895+*/+{"Crucial_CT*MX500*",NULL,ATA_HORKAGE_NO_DMA_LOG},/* End Marker */{}};
@@ -6104,6 +6117,8 @@ static int __init ata_parse_force_one(char **cur,{"ncq",.horkage_off=ATA_HORKAGE_NONCQ},{"noncqtrim",.horkage_on=ATA_HORKAGE_NO_NCQ_TRIM},{"ncqtrim",.horkage_off=ATA_HORKAGE_NO_NCQ_TRIM},+{"nodmalog",.horkage_on=ATA_HORKAGE_NO_DMA_LOG},+{"dmalog",.horkage_off=ATA_HORKAGE_NO_DMA_LOG},{"dump_id",.horkage_on=ATA_HORKAGE_DUMP_ID},{"pio0",.xfer_mask=1<<(ATA_SHIFT_PIO+0)},{"pio1",.xfer_mask=1<<(ATA_SHIFT_PIO+1)},
Hi!
On Sun, Aug 15, 2021 at 09:18:42AM +0100, Christoph Hellwig wrote:
Please just add quirks for the affected setups instead of requiring
the user to pass an obscure parameter.
I sent a patch for that and still adding the parameter.
For completeness, my original reply which did not make it to the list
(it seems emails from gmx.de do not make it through):
The point of the parameter is to be able to gather data and find out which setups are affected, enabling less knowledgeable users to help.
The sample size so far is 2, and with quite different setups, it's not even clear to me if it's a controller issue, a drive issue, or a combination.
If this feature is not very relevant it would be possible to apply a fairly broad quirk, but I feel a bit hesitant about that.
From: Damien Le Moal <hidden> Date: 2021-08-16 00:10:31
On 2021/08/16 1:27, Reimar Döffinger wrote:
quoted hunk
The ATA_CMD_READ_LOG_DMA_EXT can cause controller/device to
become unresponsive until the next power cycle.
This seems to particularly affect IDE to SATA adapters,
possibly in combination with certain SATA SSDs, though
there might be more/different cases.
Comment 5 of https://bugzilla.kernel.org/show_bug.cgi?id=195895
is an example.
Disable it by default on Crucial MX500 devices and all
PATA controllers.
Also add an option to set the workaround state, which might
help with gathering more data on the exact devices/controllers
affected, and speed up narrowing down the cause for users that
are affect but not covered by the added quirks.
Existing workarounds like forcing PIO mode do not work
(in addition to the performance issues) because READ_LOG_DMA
is issued even if PIO mode is forced.
Signed-off-by: Reimar Döffinger <redacted>
---
Documentation/admin-guide/kernel-parameters.txt | 2 ++
drivers/ata/libata-core.c | 15 +++++++++++++++
2 files changed, 17 insertions(+)
@@ -2551,6 +2551,8 @@ * [no]ncqtrim: Turn off queued DSM TRIM.+ * [no]dmalog: Turn off use of ATA_CMD_READ_LOG_DMA_EXT (0x47) command+ * nohrst, nosrst, norst: suppress hard, soft and both resets.
@@ -2004,7 +2004,11 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,retry:ata_tf_init(dev,&tf);+/* Do not even attempt DMA with PATA-SATA adapters, they seem likely to+*hang,seehttps://bugzilla.kernel.org/show_bug.cgi?id=195895+*/
This is not the standard kernel comment style. Multi-lines comment should start
with a "/*" line:
+ /*
+ * Do not even attempt DMA with PATA-SATA adapters, they seem likely to
+ * hang, see https://bugzilla.kernel.org/show_bug.cgi?id=195895
+ */
Also, I would remove the bugzilla reference since it is in the commit message.
if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+ (ap_flags & ATA_FLAG_SATA) &&
Why is this necessary since you have the ATA_HORKAGE_NO_DMA_LOG flag ?
If this is really necessary, ATA_HORKAGE_NO_DMA_LOG should be set for all
devices that have ATA_FLAG_SATA when the device is initialized. With that, this
additional condition can go away.
The ATA_CMD_READ_LOG_DMA_EXT can cause controller/device to
become unresponsive until the next power cycle.
This seems to particularly affect IDE to SATA adapters,
possibly in combination with certain SATA SSDs, though
there might be more/different cases.
Comment 5 of https://bugzilla.kernel.org/show_bug.cgi?id=195895
is an example.
Disable it by default on Crucial MX500 devices and all
PATA controllers.
Also add an option to set the workaround state, which might
help with gathering more data on the exact devices/controllers
affected, and speed up narrowing down the cause for users that
are affect but not covered by the added quirks.
Existing workarounds like forcing PIO mode do not work
(in addition to the performance issues) because READ_LOG_DMA
is issued even if PIO mode is forced.
Signed-off-by: Reimar Döffinger <redacted>
---
Documentation/admin-guide/kernel-parameters.txt | 2 ++
drivers/ata/libata-core.c | 14 ++++++++++++++
2 files changed, 16 insertions(+)
@@ -2551,6 +2551,8 @@ * [no]ncqtrim: Turn off queued DSM TRIM.+ * [no]dmalog: Turn off use of ATA_CMD_READ_LOG_DMA_EXT (0x47) command+ * nohrst, nosrst, norst: suppress hard, soft and both resets.
@@ -2399,6 +2399,9 @@ int ata_dev_configure(struct ata_device *dev)/* set horkage */dev->horkage|=ata_dev_blacklisted(dev);+/* Disable READ_LOG_DMA with PATA-SATA adapters, they seem likely to hang */+if(!(ap->flags&ATA_FLAG_SATA))+dev->horkage|=ATA_HORKAGE_NO_DMA_LOG;ata_force_horkage(dev);if(dev->horkage&ATA_HORKAGE_DISABLE){
On Mon, Aug 16, 2021 at 12:10:28AM +0000, Damien Le Moal wrote:
quoted
+ /* Do not even attempt DMA with PATA-SATA adapters, they seem likely to+ * hang, see https://bugzilla.kernel.org/show_bug.cgi?id=195895+ */
This is not the standard kernel comment style. Multi-lines comment should start
with a "/*" line:
Fixed, somehow I thought I had followed the surrounding style, I was
wrong.
Also, I would remove the bugzilla reference since it is in the commit message.
Done, though long-term the commit message might be harder to find than
the comment...
quoted
if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+ (ap_flags & ATA_FLAG_SATA) &&
Why is this necessary since you have the ATA_HORKAGE_NO_DMA_LOG flag ?
Not sure I understand the question right.
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.
(should I put something more in the commit message? I didn't want to
bloat it too much)
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?
If this is really necessary, ATA_HORKAGE_NO_DMA_LOG should be set for all
devices that have ATA_FLAG_SATA when the device is initialized. With that, this
additional condition can go away.
I missed the right place to do that before, I think I now found it, so
moved.
Thanks for the review,
Reimar
From: Damien Le Moal <hidden> Date: 2021-08-17 07:46:10
On 2021/08/17 2:26, Reimar Döffinger wrote:
On Mon, Aug 16, 2021 at 12:10:28AM +0000, Damien Le Moal wrote:
quoted
quoted
+ /* Do not even attempt DMA with PATA-SATA adapters, they seem likely to+ * hang, see https://bugzilla.kernel.org/show_bug.cgi?id=195895+ */
This is not the standard kernel comment style. Multi-lines comment should start
with a "/*" line:
Fixed, somehow I thought I had followed the surrounding style, I was
wrong.
quoted
Also, I would remove the bugzilla reference since it is in the commit message.
Done, though long-term the commit message might be harder to find than
the comment...
Yes, but long-term, web links tend to go stale too :)
quoted
quoted
if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+ (ap_flags & ATA_FLAG_SATA) &&
Why is this necessary since you have the ATA_HORKAGE_NO_DMA_LOG flag ?
Not sure I understand the question right.
I was referring to the added condition "&& (ap_flags & ATA_FLAG_SATA)".
If the port has this flag, then setting the ATA_HORKAGE_NO_DMA_LOG for the
device before coming here would not require this additional condition.
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 ?
(should I put something more in the commit message? I didn't want to
bloat it too much)
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?
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. 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.
quoted
If this is really necessary, ATA_HORKAGE_NO_DMA_LOG should be set for all
devices that have ATA_FLAG_SATA when the device is initialized. With that, this
additional condition can go away.
I missed the right place to do that before, I think I now found it, so
moved.
Thanks for the review,
Reimar
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)
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function.
Update dma_mode initialization so that SATA devices
are reported as DMA enabled.
---
drivers/ata/libata-core.c | 4 ++--
include/linux/ata.h | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -2824,7 +2824,7 @@ int ata_bus_probe(struct ata_port *ap)*busaswemaybetalkingtoofast.*/dev->pio_mode=XFER_PIO_0;-dev->dma_mode=0xff;+dev->dma_mode=ap->flags&ATA_FLAG_SATA?XFER_SATA:0xff;/* If the controller has a pio mode setup function*thenuseittosetthechipsettorights.Don't
On Mon, Aug 16, 2021 at 07:15:43PM +0200, Reimar Döffinger wrote:
quoted hunk
/* set horkage */ dev->horkage |= ata_dev_blacklisted(dev);+ /* Disable READ_LOG_DMA with PATA-SATA adapters, they seem likely to hang */+ if (!(ap->flags & ATA_FLAG_SATA))+ dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
If the DMA enable patch is accept then this part is obsolete I believe,
from the testing on my device, leaving just adding the option and
disabling READ_LOG_DMA by default for MX500.
From: Damien Le Moal <hidden> Date: 2021-08-18 22:49:31
On 2021/08/17 2:15, Reimar Döffinger wrote:
The ATA_CMD_READ_LOG_DMA_EXT can cause controller/device to
become unresponsive until the next power cycle.
This seems to particularly affect IDE to SATA adapters,
possibly in combination with certain SATA SSDs, though
there might be more/different cases.
Comment 5 of https://bugzilla.kernel.org/show_bug.cgi?id=195895
is an example.
Disable it by default on Crucial MX500 devices and all
PATA controllers.
Also add an option to set the workaround state, which might
help with gathering more data on the exact devices/controllers
affected, and speed up narrowing down the cause for users that
are affect but not covered by the added quirks.
Existing workarounds like forcing PIO mode do not work
(in addition to the performance issues) because READ_LOG_DMA
is issued even if PIO mode is forced.
Please drop the dot at the end of the patch title. Also, it would be good to be
more precise with this title. So may be something like:
libata: Disable ATA_CMD_READ_LOG_DMA_EXT use with PATA adapters
Also, please add a version number to the patch so that reviewers (and Jens) can
keep track:
[PATCH v2] libata: Disable ATA_CMD_READ_LOG_DMA_EXT use with PATA adapters
Signed-off-by: Reimar Döffinger <redacted>
---
And add the versions changelog here. Something like:
Changes from v1:
* Change 1
* change 2
* ...
@@ -2551,6 +2551,8 @@ * [no]ncqtrim: Turn off queued DSM TRIM.+ * [no]dmalog: Turn off use of ATA_CMD_READ_LOG_DMA_EXT (0x47) command+ * nohrst, nosrst, norst: suppress hard, soft and both resets.
@@ -2399,6 +2399,9 @@ int ata_dev_configure(struct ata_device *dev)/* set horkage */dev->horkage|=ata_dev_blacklisted(dev);+/* Disable READ_LOG_DMA with PATA-SATA adapters, they seem likely to hang */
Remove "likely".
quoted hunk
+ if (!(ap->flags & ATA_FLAG_SATA))+ dev->horkage |= ATA_HORKAGE_NO_DMA_LOG; ata_force_horkage(dev); if (dev->horkage & ATA_HORKAGE_DISABLE) {
@@ -4000,6 +4003,15 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "WDC WD3000JD-*", NULL, ATA_HORKAGE_WD_BROKEN_LPM }, { "WDC WD3200JD-*", NULL, ATA_HORKAGE_WD_BROKEN_LPM },+ /*+ * Devices with observed READ_LOG_DMA issues - unclear if only+ * specific firmware versions or only in combination with specific+ * controllers.+ * Specifically broken combinations reported+ * CT1000MX500SSD4, M3CR020 with B350M-Mortar+ * CT500MX500SSD4, M3CR023 with PATA-SATA adapter+ */+ { "Crucial_CT*MX500*", NULL, ATA_HORKAGE_NO_DMA_LOG }, /* End Marker */ { } };
From: Damien Le Moal <hidden> Date: 2021-08-18 23:30:51
On 2021/08/18 4:03, Reimar Döffinger wrote:
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function.
Update dma_mode initialization so that SATA devices
are reported as DMA enabled.
Please drop the period at the end of the patch title
@@ -2824,7 +2824,7 @@ int ata_bus_probe(struct ata_port *ap) * bus as we may be talking too fast. */ dev->pio_mode = XFER_PIO_0;- dev->dma_mode = 0xff;+ dev->dma_mode = ap->flags & ATA_FLAG_SATA ? XFER_SATA : 0xff;
This however is not OK I think. You are setting a transfer speed without going
through the link speed negotiation.
quoted hunk
/* If the controller has a pio mode setup function
* then use it to set the chipset to rights. Don't
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Only the libata-core logic is tested on actual devices,
the other changes are based on code review only.
---
Changes to previous version:
- removed initialization change for SATA. I got confused by the
ping-pong between libata-eh and libata-core and thought SATA did not
set up xfermode
- reviewed other cases that used dma_mode in boolean context and those
seemed to need changing as well, so added them to patch.
I did not see any places that would set dma_mode to 0 ever, so I
do think they were all indeed wrong.
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-scsi.c | 4 ++--
drivers/ata/pata_ali.c | 2 +-
drivers/ata/pata_optidma.c | 4 ++--
drivers/ata/pata_radisys.c | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
@@ -3023,7 +3023,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)ata_qc_set_pc_nbytes(qc);/* We may not issue DMA commands if no DMA mode is set */-if(tf->protocol==ATA_PROT_DMA&&dev->dma_mode==0){+if(tf->protocol==ATA_PROT_DMA&&!ata_dma_enabled(dev)){fp=1;gotoinvalid_fld;}
@@ -3173,7 +3173,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)u8unmap=cdb[1]&0x8;/* we may not issue DMA commands if no DMA mode is set */-if(unlikely(!dev->dma_mode))+if(unlikely(!ata_dma_enabled(dev)))gotoinvalid_opcode;/*
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Only the libata-core logic is tested on actual devices,
the other changes are based on code review only.
---
Changes v3:
- found and updated more cases in pata_ali, pata_amd and pata_radisys.
Changes v2:
- removed initialization change for SATA. I got confused by the
ping-pong between libata-eh and libata-core and thought SATA did not
set up xfermode
- reviewed other cases that used dma_mode in boolean context and those
seemed to need changing as well, so added them to patch.
I did not see any places that would set dma_mode to 0 ever, so I
do think they were all indeed wrong.
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-scsi.c | 4 ++--
drivers/ata/pata_ali.c | 4 ++--
drivers/ata/pata_amd.c | 2 +-
drivers/ata/pata_optidma.c | 4 ++--
drivers/ata/pata_radisys.c | 4 ++--
6 files changed, 10 insertions(+), 10 deletions(-)
@@ -3023,7 +3023,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)ata_qc_set_pc_nbytes(qc);/* We may not issue DMA commands if no DMA mode is set */-if(tf->protocol==ATA_PROT_DMA&&dev->dma_mode==0){+if(tf->protocol==ATA_PROT_DMA&&!ata_dma_enabled(dev)){fp=1;gotoinvalid_fld;}
@@ -3173,7 +3173,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)u8unmap=cdb[1]&0x8;/* we may not issue DMA commands if no DMA mode is set */-if(unlikely(!dev->dma_mode))+if(unlikely(!ata_dma_enabled(dev)))gotoinvalid_opcode;/*
@@ -66,7 +66,7 @@ static void timing_setup(struct ata_port *ap, struct ata_device *adev, int offseif(peer){/* This may be over conservative */-if(peer->dma_mode){+if(ata_dma_enabled(peer)){ata_timing_compute(peer,peer->dma_mode,&apeer,T,UT);ata_timing_merge(&apeer,&at,&at,ATA_TIMING_8BIT);}
From: Paul Menzel <hidden> Date: 2021-09-27 08:56:38
Dear Reimar,
Thank you for the patch.
Am 19.08.21 um 10:13 schrieb Reimar Döffinger:
Maybe start with a problem statement:
With some SSDs Linux logs the error below:
failed to set xfermode (err_mask=0x40)
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Only the libata-core logic is tested on actual devices,
the other changes are based on code review only.
---
Changes to previous version:
- removed initialization change for SATA. I got confused by the
ping-pong between libata-eh and libata-core and thought SATA did not
set up xfermode
- reviewed other cases that used dma_mode in boolean context and those
seemed to need changing as well, so added them to patch.
I did not see any places that would set dma_mode to 0 ever, so I
do think they were all indeed wrong.
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-scsi.c | 4 ++--
drivers/ata/pata_ali.c | 2 +-
drivers/ata/pata_optidma.c | 4 ++--
drivers/ata/pata_radisys.c | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
@@ -3023,7 +3023,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)ata_qc_set_pc_nbytes(qc);/* We may not issue DMA commands if no DMA mode is set */-if(tf->protocol==ATA_PROT_DMA&&dev->dma_mode==0){+if(tf->protocol==ATA_PROT_DMA&&!ata_dma_enabled(dev)){fp=1;gotoinvalid_fld;}
@@ -3173,7 +3173,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)u8unmap=cdb[1]&0x8;/* we may not issue DMA commands if no DMA mode is set */-if(unlikely(!dev->dma_mode))+if(unlikely(!ata_dma_enabled(dev)))gotoinvalid_opcode;/*
@@ -173,7 +173,7 @@ static unsigned int radisys_qc_issue(struct ata_queued_cmd *qc)if(adev!=ap->private_data){/* UDMA timing is not shared */if(adev->dma_mode<XFER_UDMA_0){-if(adev->dma_mode)+if(ata_dma_enabled(adev))radisys_set_dmamode(ap,adev);elseif(adev->pio_mode)radisys_set_piomode(ap,adev);
I am sorry, it took me so long to test this.
Tested-by: Paul Menzel <redacted>
(MSI B350M MORTAR with a M.2 Crucial MX500 SSD (SATA/AHCI, CT1000MX500SSD4, M3CR020))
Kind regards,
Paul
On 27 Sep 2021, at 10:56, Paul Menzel [off-list ref] wrote:
Dear Reimar,
Thank you for the patch.
Am 19.08.21 um 10:13 schrieb Reimar Döffinger:
Maybe start with a problem statement:
With some SSDs Linux logs the error below:
failed to set xfermode (err_mask=0x40)
quoted
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Only the libata-core logic is tested on actual devices,
the other changes are based on code review only.
Thanks, I missed that the Signed-off-by got lost, and thanks for testing.
I actually wanted to ask (e.g. Damien?), would you like me to split out the fully
tested and confirmed working and necessary libata-core change
from the other changes?
I am quite confident that all the code modified was wrong before,
however it could be that some of the code actually relies
on that bug to cancel out further bugs, so fixing could well break more
than it fixes...
Best regards,
Reimar
From: Damien Le Moal <hidden> Date: 2021-09-27 09:15:59
On 2021/09/27 18:10, Reimar Döffinger wrote:
quoted
On 27 Sep 2021, at 10:56, Paul Menzel [off-list ref] wrote:
Dear Reimar,
Thank you for the patch.
Am 19.08.21 um 10:13 schrieb Reimar Döffinger:
Maybe start with a problem statement:
With some SSDs Linux logs the error below:
failed to set xfermode (err_mask=0x40)
quoted
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Only the libata-core logic is tested on actual devices,
the other changes are based on code review only.
Thanks, I missed that the Signed-off-by got lost, and thanks for testing.
I actually wanted to ask (e.g. Damien?), would you like me to split out the fully
tested and confirmed working and necessary libata-core change
from the other changes?
It is generally good practice to have one patch per driver, preceded if needed
by core changes. So yes, please split things if it can be done cleanly.
I am quite confident that all the code modified was wrong before,
however it could be that some of the code actually relies
on that bug to cancel out further bugs, so fixing could well break more
than it fixes...
I had a look when you sent the patch and did not see anything scary. Please
resend and I will check again.
--
Damien Le Moal
Western Digital Research
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
This meant that ATA_CMD_READ_LOG_DMA_EXT was used and probed
for before DMA was enabled, which caused hangs for some combinations
of controllers and devices.
It might also have caused it to be incorrectly disabled as broken,
but there have been no reports of that.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=195895
Signed-off-by: Reimar Döffinger <redacted>
Tested-by: Paul Menzel <redacted>
---
drivers/ata/libata-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Change based on code review, not tested due to lack of hardware.
Signed-off-by: Reimar Döffinger <redacted>
---
drivers/ata/pata_optidma.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Change based on code review, not tested due to lack of hardware.
Signed-off-by: Reimar Döffinger <redacted>
---
drivers/ata/pata_amd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -66,7 +66,7 @@ static void timing_setup(struct ata_port *ap, struct ata_device *adev, int offseif(peer){/* This may be over conservative */-if(peer->dma_mode){+if(ata_dma_enabled(peer)){ata_timing_compute(peer,peer->dma_mode,&apeer,T,UT);ata_timing_merge(&apeer,&at,&at,ATA_TIMING_8BIT);}
Patch series to add ata_dma_enabled calls instead of incorrectly
checking dev->dma_mode != 0.
Only the first patch is confirmed to have caused real issues
that it indeed fixes, rest based purely on code review.
Changes v4:
- split per file/driver
- added Signed-off-by and Tested-by lines, improved commit messages
Changes v3:
- found and updated more cases in pata_ali, pata_amd and pata_radisys.
Changes v2:
- removed initialization change for SATA. I got confused by the
ping-pong between libata-eh and libata-core and thought SATA did not
set up xfermode
- reviewed other cases that used dma_mode in boolean context and those
seemed to need changing as well, so added them to patch.
I did not see any places that would set dma_mode to 0 ever, so I
do think they were all indeed wrong.
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Change based on code review, not tested due to lack of hardware.
Signed-off-by: Reimar Döffinger <redacted>
---
drivers/ata/pata_radisys.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Change based on code review, not tested due to lack of hardware.
Signed-off-by: Reimar Döffinger <redacted>
---
drivers/ata/libata-scsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -2981,7 +2981,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)ata_qc_set_pc_nbytes(qc);/* We may not issue DMA commands if no DMA mode is set */-if(tf->protocol==ATA_PROT_DMA&&dev->dma_mode==0){+if(tf->protocol==ATA_PROT_DMA&&!ata_dma_enabled(dev)){fp=1;gotoinvalid_fld;}
@@ -3131,7 +3131,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)u8unmap=cdb[1]&0x8;/* we may not issue DMA commands if no DMA mode is set */-if(unlikely(!dev->dma_mode))+if(unlikely(!ata_dma_enabled(dev)))gotoinvalid_opcode;/*
Checking if DMA is enabled should be done via the
ata_dma_enabled helper function, since the init state
0xff indicates disabled.
Change based on code review, not tested due to lack of hardware.
Signed-off-by: Reimar Döffinger <redacted>
---
drivers/ata/pata_ali.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Damien Le Moal <hidden> Date: 2021-10-12 02:20:56
On 10/3/21 22:28, Reimar Döffinger wrote:
Patch series to add ata_dma_enabled calls instead of incorrectly
checking dev->dma_mode != 0.
Only the first patch is confirmed to have caused real issues
that it indeed fixes, rest based purely on code review.
Changes v4:
- split per file/driver
- added Signed-off-by and Tested-by lines, improved commit messages
Changes v3:
- found and updated more cases in pata_ali, pata_amd and pata_radisys.
Changes v2:
- removed initialization change for SATA. I got confused by the
ping-pong between libata-eh and libata-core and thought SATA did not
set up xfermode
- reviewed other cases that used dma_mode in boolean context and those
seemed to need changing as well, so added them to patch.
I did not see any places that would set dma_mode to 0 ever, so I
do think they were all indeed wrong.
This looks all good to me but I do not see any CC Stable tag. Do you
want this backported to stable versions ?
--
Damien Le Moal
Western Digital Research