[PATCH] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"

[PATCH] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"

[Niklas Cassel]

This reverts commit 28ab9769117ca944cb6eb537af5599aa436287a4.

Sense data can be in either fixed format or descriptor format.

SAT-6 revision 1, "10.4.6 Control mode page", defines the D_SENSE bit:
"The SATL shall support this bit as defined in SPC-5 with the following
exception: if the D_ SENSE bit is set to zero (i.e., fixed format sense
data), then the SATL should return fixed format sense data for ATA
PASS-THROUGH commands."

The libata SATL has always kept D_SENSE set to zero by default. (It is
however possible to change the value using a MODE SELECT SG_IO command.)

Failed ATA PASS-THROUGH commands correctly respected the D_SENSE bit,
however, successful ATA PASS-THROUGH commands incorrectly returned the
sense data in descriptor format (regardless of the D_SENSE bit).

Commit 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for
CK_COND=1 and no error") fixed this bug for successful ATA PASS-THROUGH
commands.

However, after commit 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE
bit for CK_COND=1 and no error"), there were bug reports that hdparm,
hddtemp, and udisks were no longer working as expected.

These applications incorrectly assume the returned sense data is in
descriptor format, without even looking at the RESPONSE CODE field in the
returned sense data (to see which format the returned sense data is in).

Considering that there will be broken versions of these applications around
roughly forever, we are stuck with being bug compatible with older kernels.

Cc: stable@vger.kernel.org # 4.19+
Reported-by: Stephan Eisvogel <eisvogel@seitics.de>
Reported-by: Christian Heusel <christian@heusel.eu>
Closes: https://lore.kernel.org/linux-ide/0bf3f2f0-0fc6-4ba5-a420-c0874ef82d64@heusel.eu/
Fixes: 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d6f5e25e1ed8..473e00a58a8b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -951,8 +951,19 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 				   &sense_key, &asc, &ascq);
 		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
 	} else {
-		/* ATA PASS-THROUGH INFORMATION AVAILABLE */
-		ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
+		/*
+		 * ATA PASS-THROUGH INFORMATION AVAILABLE
+		 *
+		 * Note: we are supposed to call ata_scsi_set_sense(), which
+		 * respects the D_SENSE bit, instead of unconditionally
+		 * generating the sense data in descriptor format. However,
+		 * because hdparm, hddtemp, and udisks incorrectly assume sense
+		 * data in descriptor format, without even looking at the
+		 * RESPONSE CODE field in the returned sense data (to see which
+		 * format the returned sense data is in), we are stuck with
+		 * being bug compatible with older kernels.
+		 */
+		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
 	}
 }
 
-- 
2.46.0

Re: [PATCH] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"

[Martin K. Petersen]

Niklas,

> This reverts commit 28ab9769117ca944cb6eb537af5599aa436287a4.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"

[Hannes Reinecke]

On 8/13/24 15:19, Niklas Cassel wrote:
> This reverts commit 28ab9769117ca944cb6eb537af5599aa436287a4.
> 
> Sense data can be in either fixed format or descriptor format.
> 
> SAT-6 revision 1, "10.4.6 Control mode page", defines the D_SENSE bit:
> "The SATL shall support this bit as defined in SPC-5 with the following
> exception: if the D_ SENSE bit is set to zero (i.e., fixed format sense
> data), then the SATL should return fixed format sense data for ATA
> PASS-THROUGH commands."
> 
> The libata SATL has always kept D_SENSE set to zero by default. (It is
> however possible to change the value using a MODE SELECT SG_IO command.)
> 
> Failed ATA PASS-THROUGH commands correctly respected the D_SENSE bit,
> however, successful ATA PASS-THROUGH commands incorrectly returned the
> sense data in descriptor format (regardless of the D_SENSE bit).
> 
> Commit 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for
> CK_COND=1 and no error") fixed this bug for successful ATA PASS-THROUGH
> commands.
> 
> However, after commit 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE
> bit for CK_COND=1 and no error"), there were bug reports that hdparm,
> hddtemp, and udisks were no longer working as expected.
> 
> These applications incorrectly assume the returned sense data is in
> descriptor format, without even looking at the RESPONSE CODE field in the
> returned sense data (to see which format the returned sense data is in).
> 
> Considering that there will be broken versions of these applications around
> roughly forever, we are stuck with being bug compatible with older kernels.
> 
> Cc: stable@vger.kernel.org # 4.19+
> Reported-by: Stephan Eisvogel <eisvogel@seitics.de>
> Reported-by: Christian Heusel <christian@heusel.eu>
> Closes: https://lore.kernel.org/linux-ide/0bf3f2f0-0fc6-4ba5-a420-c0874ef82d64@heusel.eu/
> Fixes: 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich

Re: [PATCH] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"

[Ian Pilcher]

On 8/13/24 8:19 AM, Niklas Cassel wrote:
> This reverts commit 28ab9769117ca944cb6eb537af5599aa436287a4.
> 
> Sense data can be in either fixed format or descriptor format.
> 
> SAT-6 revision 1, "10.4.6 Control mode page", defines the D_SENSE bit:
> "The SATL shall support this bit as defined in SPC-5 with the following
> exception: if the D_ SENSE bit is set to zero (i.e., fixed format sense
> data), then the SATL should return fixed format sense data for ATA
> PASS-THROUGH commands."
> 
> The libata SATL has always kept D_SENSE set to zero by default. (It is
> however possible to change the value using a MODE SELECT SG_IO command.)
> 
> Failed ATA PASS-THROUGH commands correctly respected the D_SENSE bit,
> however, successful ATA PASS-THROUGH commands incorrectly returned the
> sense data in descriptor format (regardless of the D_SENSE bit).
> 
> Commit 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for
> CK_COND=1 and no error") fixed this bug for successful ATA PASS-THROUGH
> commands.
> 
> However, after commit 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE
> bit for CK_COND=1 and no error"), there were bug reports that hdparm,
> hddtemp, and udisks were no longer working as expected.
> 
> These applications incorrectly assume the returned sense data is in
> descriptor format, without even looking at the RESPONSE CODE field in the
> returned sense data (to see which format the returned sense data is in).
> 
> Considering that there will be broken versions of these applications around
> roughly forever, we are stuck with being bug compatible with older kernels.

I suppose it's a small quibble, but I don't think it's fair to say that
the applications are behaving incorrectly.  They assume that the
returned sense data is in descriptor format because it always was.  That
doesn't seem unreasonable.

-- 
========================================================================
Google                                      Where SkyNet meets Idiocracy
========================================================================

Re: [PATCH] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"

[Niklas Cassel]

On Tue, 13 Aug 2024 15:19:01 +0200, Niklas Cassel wrote:
> This reverts commit 28ab9769117ca944cb6eb537af5599aa436287a4.
> 
> Sense data can be in either fixed format or descriptor format.
> 
> SAT-6 revision 1, "10.4.6 Control mode page", defines the D_SENSE bit:
> "The SATL shall support this bit as defined in SPC-5 with the following
> exception: if the D_ SENSE bit is set to zero (i.e., fixed format sense
> data), then the SATL should return fixed format sense data for ATA
> PASS-THROUGH commands."
> 
> [...]

Applied to libata/linux.git (for-6.11-fixes), thanks!

[1/1] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"
      https://git.kernel.org/libata/linux/c/fa0db8e5

Kind regards,
Niklas

Re: [PATCH] Revert "ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error"

[Ian Pilcher]

On 8/13/24 8:19 AM, Niklas Cassel wrote:
> This reverts commit 28ab9769117ca944cb6eb537af5599aa436287a4.

Reviewed-by: Ian Pilcher <arequipeno@gmail.com>

-- 
========================================================================
Google                                      Where SkyNet meets Idiocracy
========================================================================