[PATCH] libata: add kernel parameter to force ATA_HORKAGE_NO_DMA_LOG

[PATCH] libata: add kernel parameter to force ATA_HORKAGE_NO_DMA_LOG

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 Documentation/admin-guide/kernel-parameters.txt | 2 ++
 drivers/ata/libata-core.c                       | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index bdb22006f713..191502e8fa74 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -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.
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 61c762961ca8..7fb865333a38 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6104,6 +6104,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) },
-- 
2.32.0

Re: [PATCH] libata: add kernel parameter to force ATA_HORKAGE_NO_DMA_LOG

[Christoph Hellwig]

Please just add quirks for the affected setups instead of requiring
the user to pass an obscure parameter.

[PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 Documentation/admin-guide/kernel-parameters.txt |  2 ++
 drivers/ata/libata-core.c                       | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index bdb22006f713..191502e8fa74 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -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.
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 61c762961ca8..219fa92ffc06 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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, see https://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
+	 * 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
+	 * 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) },
-- 
2.32.0

Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Damien Le Moal]

On 2021/08/16 1:27, 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.
> 
> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  2 ++
>  drivers/ata/libata-core.c                       | 15 +++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index bdb22006f713..191502e8fa74 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -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.
>  
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 61c762961ca8..219fa92ffc06 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -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, 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:

+	/*
+	 * 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.

>  	    !(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
> +	 * 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
> +	 * https://bugzilla.kernel.org/show_bug.cgi?id=195895
> +	 */

Same comment about the comment format.
I would also remove the bugzilla reference as it is in the commit message.

> +	{ "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) },
> 


-- 
Damien Le Moal
Western Digital Research

[PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 Documentation/admin-guide/kernel-parameters.txt |  2 ++
 drivers/ata/libata-core.c                       | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index bdb22006f713..191502e8fa74 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -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.
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 61c762961ca8..9934f6c465f4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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) {
@@ -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 */
 	{ }
 };
@@ -6104,6 +6116,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) },
-- 
2.32.0

Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Reimar Döffinger]

On Mon, Aug 16, 2021 at 07:15:43PM +0200, Reimar Döffinger wrote:
>  	/* 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.

Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Damien Le Moal]

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 <Reimar.Doeffinger@gmx.de>
> ---

And add the versions changelog here. Something like:

Changes from v1:
* Change 1
* change 2
* ...

>  Documentation/admin-guide/kernel-parameters.txt |  2 ++
>  drivers/ata/libata-core.c                       | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index bdb22006f713..191502e8fa74 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -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.
>  
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 61c762961ca8..9934f6c465f4 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -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".

> +	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 */
>  	{ }
>  };
> @@ -6104,6 +6116,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) },
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Reimar Döffinger]

On Mon, Aug 16, 2021 at 12:10:28AM +0000, Damien Le Moal wrote:
> > +	/* 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...

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

Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Damien Le Moal]

On 2021/08/17 2:26, Reimar Döffinger wrote:
> On Mon, Aug 16, 2021 at 12:10:28AM +0000, Damien Le Moal wrote:
>>> +	/* 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...

Yes, but long-term, web links tend to go stale too :)

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

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


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] libata: Disable ATA_CMD_READ_LOG_DMA_EXT in problematic cases.

[Reimar Döffinger]

On Tue, Aug 17, 2021 at 07:45:58AM +0000, Damien Le Moal wrote:
> On 2021/08/17 2:26, Reimar Döffinger wrote:
> > 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).


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

[PATCH] libata: fix setting and checking of DMA state.

[Reimar Döffinger]

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(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9934f6c465f4..a5fe20bb22d6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2004,7 +2004,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 retry:
 	ata_tf_init(dev, &tf);
-	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+	if (ata_dma_enabled(dev) && ata_id_has_read_log_dma_ext(dev->id) &&
 	    !(dev->horkage & ATA_HORKAGE_NO_DMA_LOG)) {
 		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
 		tf.protocol = ATA_PROT_DMA;
@@ -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;
 
 		/* If the controller has a pio mode setup function
 		 * then use it to set the chipset to rights. Don't
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 1b44f40c7700..7bb2c2acbc42 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -374,6 +374,7 @@ enum {
 
 	/* SETFEATURES stuff */
 	SETFEATURES_XFER	= 0x03,
+	XFER_SATA               = 0x48,
 	XFER_UDMA_7		= 0x47,
 	XFER_UDMA_6		= 0x46,
 	XFER_UDMA_5		= 0x45,
-- 
2.32.0

Re: [PATCH] libata: fix setting and checking of DMA state.

[Damien Le Moal]

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

> ---
>  drivers/ata/libata-core.c | 4 ++--
>  include/linux/ata.h       | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 9934f6c465f4..a5fe20bb22d6 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2004,7 +2004,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  
>  retry:
>  	ata_tf_init(dev, &tf);
> -	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
> +	if (ata_dma_enabled(dev) && ata_id_has_read_log_dma_ext(dev->id) &&

This change is OK I think. Note that there are many other places where
dev->dma_mode is testing directly. It may be good to check those too.

>  	    !(dev->horkage & ATA_HORKAGE_NO_DMA_LOG)) {
>  		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
>  		tf.protocol = ATA_PROT_DMA;
> @@ -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.

>  
>  		/* If the controller has a pio mode setup function
>  		 * then use it to set the chipset to rights. Don't
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 1b44f40c7700..7bb2c2acbc42 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -374,6 +374,7 @@ enum {
>  
>  	/* SETFEATURES stuff */
>  	SETFEATURES_XFER	= 0x03,
> +	XFER_SATA               = 0x48,

Where does this come from ?

>  	XFER_UDMA_7		= 0x47,
>  	XFER_UDMA_6		= 0x46,
>  	XFER_UDMA_5		= 0x45,
> 


-- 
Damien Le Moal
Western Digital Research

[PATCH] libata: fix checking of DMA state

[Reimar Döffinger]

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(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9934f6c465f4..52469b39d424 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2004,7 +2004,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 retry:
 	ata_tf_init(dev, &tf);
-	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+	if (ata_dma_enabled(dev) && ata_id_has_read_log_dma_ext(dev->id) &&
 	    !(dev->horkage & ATA_HORKAGE_NO_DMA_LOG)) {
 		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
 		tf.protocol = ATA_PROT_DMA;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b9588c52815d..9e51251161e9 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -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;
 		goto invalid_fld;
 	}
@@ -3173,7 +3173,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	u8 unmap = 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)))
 		goto invalid_opcode;
 
 	/*
diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 557ecf466102..28b56811306f 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -215,7 +215,7 @@ static void ali_set_piomode(struct ata_port *ap, struct ata_device *adev)
 		struct ata_timing p;
 		ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
 		ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
-		if (pair->dma_mode) {
+		if (ata_dma_enabled(pair)) {
 			ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
 			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
 		}
diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
index f6278d9de348..ad1090b90e52 100644
--- a/drivers/ata/pata_optidma.c
+++ b/drivers/ata/pata_optidma.c
@@ -153,7 +153,7 @@ static void optidma_mode_setup(struct ata_port *ap, struct ata_device *adev, u8
 	if (pair) {
 		u8 pair_addr;
 		/* Hardware constraint */
-		if (pair->dma_mode)
+		if (ata_dma_enabled(pair))
 			pair_addr = 0;
 		else
 			pair_addr = addr_timing[pci_clock][pair->pio_mode - XFER_PIO_0];
@@ -301,7 +301,7 @@ static u8 optidma_make_bits43(struct ata_device *adev)
 	};
 	if (!ata_dev_enabled(adev))
 		return 0;
-	if (adev->dma_mode)
+	if (ata_dma_enabled(adev))
 		return adev->dma_mode - XFER_MW_DMA_0;
 	return bits43[adev->pio_mode - XFER_PIO_0];
 }
diff --git a/drivers/ata/pata_radisys.c b/drivers/ata/pata_radisys.c
index 8fde4a86401b..626b14f0f2ea 100644
--- a/drivers/ata/pata_radisys.c
+++ b/drivers/ata/pata_radisys.c
@@ -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);
 			else if (adev->pio_mode)
 				radisys_set_piomode(ap, adev);
-- 
2.32.0

[PATCH] libata: fix checking of DMA state

[Reimar Döffinger]

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(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9934f6c465f4..52469b39d424 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2004,7 +2004,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 retry:
 	ata_tf_init(dev, &tf);
-	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+	if (ata_dma_enabled(dev) && ata_id_has_read_log_dma_ext(dev->id) &&
 	    !(dev->horkage & ATA_HORKAGE_NO_DMA_LOG)) {
 		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
 		tf.protocol = ATA_PROT_DMA;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b9588c52815d..9e51251161e9 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -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;
 		goto invalid_fld;
 	}
@@ -3173,7 +3173,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	u8 unmap = 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)))
 		goto invalid_opcode;
 
 	/*
diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 557ecf466102..b7ff63ed3bbb 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -215,7 +215,7 @@ static void ali_set_piomode(struct ata_port *ap, struct ata_device *adev)
 		struct ata_timing p;
 		ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
 		ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
-		if (pair->dma_mode) {
+		if (ata_dma_enabled(pair)) {
 			ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
 			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
 		}
@@ -264,7 +264,7 @@ static void ali_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 			struct ata_timing p;
 			ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
 			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
-			if (pair->dma_mode) {
+			if (ata_dma_enabled(pair)) {
 				ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
 				ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
 			}
diff --git a/drivers/ata/pata_amd.c b/drivers/ata/pata_amd.c
index c8acba162d02..154748cfcc79 100644
--- a/drivers/ata/pata_amd.c
+++ b/drivers/ata/pata_amd.c
@@ -66,7 +66,7 @@ static void timing_setup(struct ata_port *ap, struct ata_device *adev, int offse
 
 	if (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);
 		}
diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
index f6278d9de348..ad1090b90e52 100644
--- a/drivers/ata/pata_optidma.c
+++ b/drivers/ata/pata_optidma.c
@@ -153,7 +153,7 @@ static void optidma_mode_setup(struct ata_port *ap, struct ata_device *adev, u8
 	if (pair) {
 		u8 pair_addr;
 		/* Hardware constraint */
-		if (pair->dma_mode)
+		if (ata_dma_enabled(pair))
 			pair_addr = 0;
 		else
 			pair_addr = addr_timing[pci_clock][pair->pio_mode - XFER_PIO_0];
@@ -301,7 +301,7 @@ static u8 optidma_make_bits43(struct ata_device *adev)
 	};
 	if (!ata_dev_enabled(adev))
 		return 0;
-	if (adev->dma_mode)
+	if (ata_dma_enabled(adev))
 		return adev->dma_mode - XFER_MW_DMA_0;
 	return bits43[adev->pio_mode - XFER_PIO_0];
 }
diff --git a/drivers/ata/pata_radisys.c b/drivers/ata/pata_radisys.c
index 8fde4a86401b..3aca8fe3fdb6 100644
--- a/drivers/ata/pata_radisys.c
+++ b/drivers/ata/pata_radisys.c
@@ -172,8 +172,8 @@ 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 (adev->dma_mode < XFER_UDMA_0 || !ata_dma_enabled(adev)) {
+			if (ata_dma_enabled(adev))
 				radisys_set_dmamode(ap, adev);
 			else if (adev->pio_mode)
 				radisys_set_piomode(ap, adev);
-- 
2.32.0

Re: [PATCH] libata: fix checking of DMA state

[Paul Menzel]

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.

Your Signed-off-by line is missing, and you might want to add the Linux 
kernel bug tracker entry:

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=195895

> ---
> 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(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 9934f6c465f4..52469b39d424 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2004,7 +2004,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>   
>   retry:
>   	ata_tf_init(dev, &tf);
> -	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
> +	if (ata_dma_enabled(dev) && ata_id_has_read_log_dma_ext(dev->id) &&
>   	    !(dev->horkage & ATA_HORKAGE_NO_DMA_LOG)) {
>   		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
>   		tf.protocol = ATA_PROT_DMA;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b9588c52815d..9e51251161e9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -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;
>   		goto invalid_fld;
>   	}
> @@ -3173,7 +3173,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>   	u8 unmap = 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)))
>   		goto invalid_opcode;
>   
>   	/*
> diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
> index 557ecf466102..28b56811306f 100644
> --- a/drivers/ata/pata_ali.c
> +++ b/drivers/ata/pata_ali.c
> @@ -215,7 +215,7 @@ static void ali_set_piomode(struct ata_port *ap, struct ata_device *adev)
>   		struct ata_timing p;
>   		ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
>   		ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
> -		if (pair->dma_mode) {
> +		if (ata_dma_enabled(pair)) {
>   			ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
>   			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
>   		}
> diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
> index f6278d9de348..ad1090b90e52 100644
> --- a/drivers/ata/pata_optidma.c
> +++ b/drivers/ata/pata_optidma.c
> @@ -153,7 +153,7 @@ static void optidma_mode_setup(struct ata_port *ap, struct ata_device *adev, u8
>   	if (pair) {
>   		u8 pair_addr;
>   		/* Hardware constraint */
> -		if (pair->dma_mode)
> +		if (ata_dma_enabled(pair))
>   			pair_addr = 0;
>   		else
>   			pair_addr = addr_timing[pci_clock][pair->pio_mode - XFER_PIO_0];
> @@ -301,7 +301,7 @@ static u8 optidma_make_bits43(struct ata_device *adev)
>   	};
>   	if (!ata_dev_enabled(adev))
>   		return 0;
> -	if (adev->dma_mode)
> +	if (ata_dma_enabled(adev))
>   		return adev->dma_mode - XFER_MW_DMA_0;
>   	return bits43[adev->pio_mode - XFER_PIO_0];
>   }
> diff --git a/drivers/ata/pata_radisys.c b/drivers/ata/pata_radisys.c
> index 8fde4a86401b..626b14f0f2ea 100644
> --- a/drivers/ata/pata_radisys.c
> +++ b/drivers/ata/pata_radisys.c
> @@ -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);
>   			else if (adev->pio_mode)
>   				radisys_set_piomode(ap, adev);
> 

I am sorry, it took me so long to test this.

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>

(MSI B350M MORTAR with a M.2 Crucial MX500 SSD (SATA/AHCI, 
CT1000MX500SSD4, M3CR020))


Kind regards,

Paul

Re: [PATCH] libata: fix checking of DMA state

[Reimar Döffinger]

> On 27 Sep 2021, at 10:56, Paul Menzel <pmenzel@molgen.mpg.de> 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)
> 
>> 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.
> 
> Your Signed-off-by line is missing, and you might want to add the Linux kernel bug tracker entry:
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=195895

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

Re: [PATCH] libata: fix checking of DMA state

[Damien Le Moal]

On 2021/09/27 18:10, Reimar Döffinger wrote:
> 
>> On 27 Sep 2021, at 10:56, Paul Menzel <pmenzel@molgen.mpg.de> 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)
>>
>>> 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.
>>
>> Your Signed-off-by line is missing, and you might want to add the Linux kernel bug tracker entry:
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=195895
> 
> 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

[PATCH v4] Fixes to DMA state check

[Reimar Döffinger]

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.

[PATCH 1/6] libata: fix checking of DMA state

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/ata/libata-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index eed65311b5d1..046faf0dbdd3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2007,7 +2007,7 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 retry:
 	ata_tf_init(dev, &tf);
-	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+	if (ata_dma_enabled(dev) && ata_id_has_read_log_dma_ext(dev->id) &&
 	    !(dev->horkage & ATA_HORKAGE_NO_DMA_LOG)) {
 		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
 		tf.protocol = ATA_PROT_DMA;
-- 
2.33.0

[PATCH 2/6] libata-scsi: fix checking of DMA state

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1fb4611f7eeb..0adea33f2137 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -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;
 		goto invalid_fld;
 	}
@@ -3131,7 +3131,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	u8 unmap = 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)))
 		goto invalid_opcode;
 
 	/*
-- 
2.33.0

[PATCH 3/6] pata_ali: fix checking of DMA state

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 drivers/ata/pata_ali.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 557ecf466102..b7ff63ed3bbb 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -215,7 +215,7 @@ static void ali_set_piomode(struct ata_port *ap, struct ata_device *adev)
 		struct ata_timing p;
 		ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
 		ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
-		if (pair->dma_mode) {
+		if (ata_dma_enabled(pair)) {
 			ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
 			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
 		}
@@ -264,7 +264,7 @@ static void ali_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 			struct ata_timing p;
 			ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
 			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
-			if (pair->dma_mode) {
+			if (ata_dma_enabled(pair)) {
 				ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
 				ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
 			}
-- 
2.33.0

[PATCH 4/6] pata_amd: fix checking of DMA state

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 drivers/ata/pata_amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_amd.c b/drivers/ata/pata_amd.c
index c8acba162d02..154748cfcc79 100644
--- a/drivers/ata/pata_amd.c
+++ b/drivers/ata/pata_amd.c
@@ -66,7 +66,7 @@ static void timing_setup(struct ata_port *ap, struct ata_device *adev, int offse
 
 	if (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);
 		}
-- 
2.33.0

[PATCH 5/6] pata_optidma: fix checking of DMA state

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 drivers/ata/pata_optidma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
index f6278d9de348..ad1090b90e52 100644
--- a/drivers/ata/pata_optidma.c
+++ b/drivers/ata/pata_optidma.c
@@ -153,7 +153,7 @@ static void optidma_mode_setup(struct ata_port *ap, struct ata_device *adev, u8
 	if (pair) {
 		u8 pair_addr;
 		/* Hardware constraint */
-		if (pair->dma_mode)
+		if (ata_dma_enabled(pair))
 			pair_addr = 0;
 		else
 			pair_addr = addr_timing[pci_clock][pair->pio_mode - XFER_PIO_0];
@@ -301,7 +301,7 @@ static u8 optidma_make_bits43(struct ata_device *adev)
 	};
 	if (!ata_dev_enabled(adev))
 		return 0;
-	if (adev->dma_mode)
+	if (ata_dma_enabled(adev))
 		return adev->dma_mode - XFER_MW_DMA_0;
 	return bits43[adev->pio_mode - XFER_PIO_0];
 }
-- 
2.33.0

[PATCH 6/6] pata_radisys: fix checking of DMA state

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 drivers/ata/pata_radisys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_radisys.c b/drivers/ata/pata_radisys.c
index 8fde4a86401b..3aca8fe3fdb6 100644
--- a/drivers/ata/pata_radisys.c
+++ b/drivers/ata/pata_radisys.c
@@ -172,8 +172,8 @@ 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 (adev->dma_mode < XFER_UDMA_0 || !ata_dma_enabled(adev)) {
+			if (ata_dma_enabled(adev))
 				radisys_set_dmamode(ap, adev);
 			else if (adev->pio_mode)
 				radisys_set_piomode(ap, adev);
-- 
2.33.0

Re: [PATCH v4] Fixes to DMA state check

[Damien Le Moal]

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

Re: [PATCH v4] Fixes to DMA state check

[Reimar Döffinger]

On Tue, Oct 12, 2021 at 11:20:47AM +0900, Damien Le Moal wrote:
> This looks all good to me but I do not see any CC Stable tag. Do you
> want this backported to stable versions ?

Sent a new version with patch 1/6 (and only that) having the stable Cc, which I think
is what makes most sense.

Best regards,
Reimar

Re: [PATCH v4] Fixes to DMA state check

[Damien Le Moal]

On 2021/10/12 14:57, Reimar Döffinger wrote:
> On 12 October 2021 04:20:47 CEST, Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote:
>> 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 ?
> 
> Sorry, I admit I am quite ignorant of these workflow details.
> Personally I have no need for a backport as it's only about one piece of legacy HW for me.
> I also feel like the untested patches do not belong in a stable backport.
> However the first, tested patch might be good to have backported if considering the whole user base and not just my use? Would that be sensible to handle that way? 

That works !

> 
> Best regards,
> Reimar
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4] Fixes to DMA state check

[Damien Le Moal]

On 2021/10/12 14:57, Reimar Döffinger wrote:
> On 12 October 2021 04:20:47 CEST, Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote:
>> 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 ?
> 
> Sorry, I admit I am quite ignorant of these workflow details.
> Personally I have no need for a backport as it's only about one piece of legacy HW for me.
> I also feel like the untested patches do not belong in a stable backport.
> However the first, tested patch might be good to have backported if considering the whole user base and not just my use? Would that be sensible to handle that way? 

Note that I will apply the entire series to for-5.16 so that the patches have
time to go through linux-next for more testing. The first patch will be
backported once we start the 5.16 cycle.


> 
> Best regards,
> Reimar
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] libata: add kernel parameter to force ATA_HORKAGE_NO_DMA_LOG

[Reimar Döffinger]

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.

[PATCH v5] Fixes to DMA state check

[Reimar Döffinger]

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 v5:
Add stable Cc to first patch, which is confirmed to fix
issues seen by users.

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.

[PATCH 3/6] pata_ali: fix checking of DMA state

[Reimar Döffinger]

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 <Reimar.Doeffinger@gmx.de>
---
 drivers/ata/pata_ali.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 557ecf466102..b7ff63ed3bbb 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -215,7 +215,7 @@ static void ali_set_piomode(struct ata_port *ap, struct ata_device *adev)
 		struct ata_timing p;
 		ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
 		ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
-		if (pair->dma_mode) {
+		if (ata_dma_enabled(pair)) {
 			ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
 			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
 		}
@@ -264,7 +264,7 @@ static void ali_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 			struct ata_timing p;
 			ata_timing_compute(pair, pair->pio_mode, &p, T, 1);
 			ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
-			if (pair->dma_mode) {
+			if (ata_dma_enabled(pair)) {
 				ata_timing_compute(pair, pair->dma_mode, &p, T, 1);
 				ata_timing_merge(&p, &t, &t, ATA_TIMING_SETUP|ATA_TIMING_8BIT);
 			}
-- 
2.33.0