[PATCH 0/2] Fixedup status+error translation to sense key/code

[PATCH 0/2] Fixedup status+error translation to sense key/code

[Damien Le Moal]

A couple of patches to fix issues with the translation of a failed qc
status and error into a sense key and sense code. The first patch fixes
a long standing problem leading to nonsensical sense reports for failed
commands.

Damien Le Moal (2):
  ata: libata-scsi: Fix ata_to_sense_error() status handling
  ata: libata-scsi: Return aborted command when missing sense and result TF

 drivers/ata/libata-scsi.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

-- 
2.50.1

[PATCH 1/2] ata: libata-scsi: Fix ata_to_sense_error() status handling

[Damien Le Moal]

Commit 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
inadvertantly added the entry 0x40 (ATA_DRDY) to the stat_table array in
the function ata_to_sense_error(). This entry ties a failed qc which has
a status filed equal to ATA_DRDY to the sense key ILLEGAL REQUEST with
the additional sense code UNALIGNED WRITE COMMAND. This entry will be
used to generate a failed qc sense key and sense code when the qc is
missing sense data and there is no match for the qc error field in the
sense_table array of ata_to_sense_error().

As a result, for a failed qc for which we failed to get sense data (e.g.
read log 10h failed if qc is an NCQ command, or REQUEST SENSE EXT
command failed for the non-ncq case, the user very often end up seeing
the completely misleading "unaligned write command" error, even if qc
was not a write command. E.g.:

sd 0:0:0:0: [sda] tag#12 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
sd 0:0:0:0: [sda] tag#12 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#12 Add. Sense: Unaligned write command
sd 0:0:0:0: [sda] tag#12 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0

Fix this by removing the ATA_DRDY entry from the stat_table array so
that we default to always returning ABORTED COMMAND without any
additional sense code, since we do not know any better. The entry 0x08
(ATA_DRQ) is also removed since signaling ABORTED COMMAND with a parity
error is also misleading (as a parity error would likely be signaled
through a bus error). So for this case, also default to returning
ABORTED COMMAND without any additional sense code. With this, the
previous example error case becomes:

sd 0:0:0:0: [sda] tag#17 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
sd 0:0:0:0: [sda] tag#17 Sense Key : Aborted Command [current]
sd 0:0:0:0: [sda] tag#17 Add. Sense: No additional sense information
sd 0:0:0:0: [sda] tag#17 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0

Together with these fixes, refactor stat_table to make it more readable
by putting the entries comments in front of the entries and using the
defined status bits macros instead of hardcoded values.

Reported-by: Lorenz Brun <lorenz@brun.one>
Reported-by: Brandon Schwartz <Brandon.Schwartz@wdc.com>
Fixes: 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 27b15176db56..9b16c0f553e0 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -859,18 +859,14 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
 		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
 	};
 	static const unsigned char stat_table[][4] = {
-		/* Must be first because BUSY means no other bits valid */
-		{0x80,		ABORTED_COMMAND, 0x47, 0x00},
-		// Busy, fake parity for now
-		{0x40,		ILLEGAL_REQUEST, 0x21, 0x04},
-		// Device ready, unaligned write command
-		{0x20,		HARDWARE_ERROR,  0x44, 0x00},
-		// Device fault, internal target failure
-		{0x08,		ABORTED_COMMAND, 0x47, 0x00},
-		// Timed out in xfer, fake parity for now
-		{0x04,		RECOVERED_ERROR, 0x11, 0x00},
-		// Recovered ECC error	  Medium error, recovered
-		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
+		/* Busy: must be first because BUSY means no other bits valid */
+		{ ATA_BUSY,	ABORTED_COMMAND, 0x00, 0x00 },
+		/* Device fault: INTERNAL TARGET FAILURE */
+		{ ATA_DF,	HARDWARE_ERROR,  0x44, 0x00 },
+		/* Corrected data error */
+		{ ATA_CORR,	RECOVERED_ERROR, 0x00, 0x00 },
+
+		{ 0xFF, 0xFF, 0xFF, 0xFF }, /* END mark */
 	};
 
 	/*
-- 
2.50.1

[PATCH 2/2] ata: libata-scsi: Return aborted command when missing sense and result TF

[Damien Le Moal]

ata_gen_ata_sense() is always called for a failed qc missing sense data
so that a sense key, code and code qualifier can be generated using
ata_to_sense_error() from the qc status and error fields of its result
task file. However, if the qc does not have its result task file filled,
ata_gen_ata_sense() returns early without setting a sense key.

Improve this by defaulting to returning ABORTED COMMAND without any
additional sense code, since we do not know the reason for the failure.
The same fix is also applied in ata_gen_passthru_sense() with the
additional check that the qc failed (qc->err_mask is set).

Fixes: 816be86c7993 ("ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 9b16c0f553e0..57f674f51b0c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -938,6 +938,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
 		ata_dev_dbg(dev,
 			    "missing result TF: can't generate ATA PT sense data\n");
+		if (qc->err_mask)
+			ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
 		return;
 	}
 
@@ -992,8 +994,8 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 
 	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
 		ata_dev_dbg(dev,
-			    "missing result TF: can't generate sense data\n");
-		return;
+			    "Missing result TF: reporting aborted command\n");
+		goto aborted;
 	}
 
 	/* Use ata_to_sense_error() to map status register bits
@@ -1004,13 +1006,15 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 		ata_to_sense_error(tf->status, tf->error,
 				   &sense_key, &asc, &ascq);
 		ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
-	} else {
-		/* Could not decode error */
-		ata_dev_warn(dev, "could not decode error status 0x%x err_mask 0x%x\n",
-			     tf->status, qc->err_mask);
-		ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
 		return;
 	}
+
+	/* Could not decode error */
+	ata_dev_warn(dev,
+		"Could not decode error 0x%x, status 0x%x (err_mask=0x%x)\n",
+		tf->error, tf->status, qc->err_mask);
+aborted:
+	ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
 }
 
 void ata_scsi_sdev_config(struct scsi_device *sdev)
-- 
2.50.1

Re: [PATCH 1/2] ata: libata-scsi: Fix ata_to_sense_error() status handling

[Hannes Reinecke]

On 7/30/25 02:24, Damien Le Moal wrote:
> Commit 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
> inadvertantly added the entry 0x40 (ATA_DRDY) to the stat_table array in
> the function ata_to_sense_error(). This entry ties a failed qc which has
> a status filed equal to ATA_DRDY to the sense key ILLEGAL REQUEST with
> the additional sense code UNALIGNED WRITE COMMAND. This entry will be
> used to generate a failed qc sense key and sense code when the qc is
> missing sense data and there is no match for the qc error field in the
> sense_table array of ata_to_sense_error().
> 
> As a result, for a failed qc for which we failed to get sense data (e.g.
> read log 10h failed if qc is an NCQ command, or REQUEST SENSE EXT
> command failed for the non-ncq case, the user very often end up seeing
> the completely misleading "unaligned write command" error, even if qc
> was not a write command. E.g.:
> 
> sd 0:0:0:0: [sda] tag#12 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
> sd 0:0:0:0: [sda] tag#12 Sense Key : Illegal Request [current]
> sd 0:0:0:0: [sda] tag#12 Add. Sense: Unaligned write command
> sd 0:0:0:0: [sda] tag#12 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
> 
> Fix this by removing the ATA_DRDY entry from the stat_table array so
> that we default to always returning ABORTED COMMAND without any
> additional sense code, since we do not know any better. The entry 0x08
> (ATA_DRQ) is also removed since signaling ABORTED COMMAND with a parity
> error is also misleading (as a parity error would likely be signaled
> through a bus error). So for this case, also default to returning
> ABORTED COMMAND without any additional sense code. With this, the
> previous example error case becomes:
> 
> sd 0:0:0:0: [sda] tag#17 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
> sd 0:0:0:0: [sda] tag#17 Sense Key : Aborted Command [current]
> sd 0:0:0:0: [sda] tag#17 Add. Sense: No additional sense information
> sd 0:0:0:0: [sda] tag#17 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
> 
> Together with these fixes, refactor stat_table to make it more readable
> by putting the entries comments in front of the entries and using the
> defined status bits macros instead of hardcoded values.
> 
> Reported-by: Lorenz Brun <lorenz@brun.one>
> Reported-by: Brandon Schwartz <Brandon.Schwartz@wdc.com>
> Fixes: 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 27b15176db56..9b16c0f553e0 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -859,18 +859,14 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
>   		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
>   	};
>   	static const unsigned char stat_table[][4] = {
> -		/* Must be first because BUSY means no other bits valid */
> -		{0x80,		ABORTED_COMMAND, 0x47, 0x00},
> -		// Busy, fake parity for now
> -		{0x40,		ILLEGAL_REQUEST, 0x21, 0x04},
> -		// Device ready, unaligned write command
> -		{0x20,		HARDWARE_ERROR,  0x44, 0x00},
> -		// Device fault, internal target failure
> -		{0x08,		ABORTED_COMMAND, 0x47, 0x00},
> -		// Timed out in xfer, fake parity for now
> -		{0x04,		RECOVERED_ERROR, 0x11, 0x00},
> -		// Recovered ECC error	  Medium error, recovered
> -		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
> +		/* Busy: must be first because BUSY means no other bits valid */
> +		{ ATA_BUSY,	ABORTED_COMMAND, 0x00, 0x00 },
> +		/* Device fault: INTERNAL TARGET FAILURE */
> +		{ ATA_DF,	HARDWARE_ERROR,  0x44, 0x00 },
> +		/* Corrected data error */
> +		{ ATA_CORR,	RECOVERED_ERROR, 0x00, 0x00 },
> +
> +		{ 0xFF, 0xFF, 0xFF, 0xFF }, /* END mark */
>   	};
>   
>   	/*

Yeah, I think the translation to 'unaligned write command' was from the
very early days of SMR development.

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, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 2/2] ata: libata-scsi: Return aborted command when missing sense and result TF

[Hannes Reinecke]

On 7/30/25 02:24, Damien Le Moal wrote:
> ata_gen_ata_sense() is always called for a failed qc missing sense data
> so that a sense key, code and code qualifier can be generated using
> ata_to_sense_error() from the qc status and error fields of its result
> task file. However, if the qc does not have its result task file filled,
> ata_gen_ata_sense() returns early without setting a sense key.
> 
> Improve this by defaulting to returning ABORTED COMMAND without any
> additional sense code, since we do not know the reason for the failure.
> The same fix is also applied in ata_gen_passthru_sense() with the
> additional check that the qc failed (qc->err_mask is set).
> 
> Fixes: 816be86c7993 ("ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 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, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 0/2] Fixedup status+error translation to sense key/code

[Martin K. Petersen]

Damien,

> A couple of patches to fix issues with the translation of a failed qc
> status and error into a sense key and sense code. The first patch fixes
> a long standing problem leading to nonsensical sense reports for failed
> commands.

Looks fine.

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

-- 
Martin K. Petersen

Re: [PATCH 1/2] ata: libata-scsi: Fix ata_to_sense_error() status handling

[Igor Pylypiv]

On Wed, Jul 30, 2025 at 09:24:40AM +0900, Damien Le Moal wrote:
> Commit 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
> inadvertantly added the entry 0x40 (ATA_DRDY) to the stat_table array in
> the function ata_to_sense_error(). This entry ties a failed qc which has
> a status filed equal to ATA_DRDY to the sense key ILLEGAL REQUEST with
> the additional sense code UNALIGNED WRITE COMMAND. This entry will be
> used to generate a failed qc sense key and sense code when the qc is
> missing sense data and there is no match for the qc error field in the
> sense_table array of ata_to_sense_error().
> 
> As a result, for a failed qc for which we failed to get sense data (e.g.
> read log 10h failed if qc is an NCQ command, or REQUEST SENSE EXT
> command failed for the non-ncq case, the user very often end up seeing
> the completely misleading "unaligned write command" error, even if qc
> was not a write command. E.g.:
> 
> sd 0:0:0:0: [sda] tag#12 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
> sd 0:0:0:0: [sda] tag#12 Sense Key : Illegal Request [current]
> sd 0:0:0:0: [sda] tag#12 Add. Sense: Unaligned write command
> sd 0:0:0:0: [sda] tag#12 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
> 
> Fix this by removing the ATA_DRDY entry from the stat_table array so
> that we default to always returning ABORTED COMMAND without any
> additional sense code, since we do not know any better. The entry 0x08
> (ATA_DRQ) is also removed since signaling ABORTED COMMAND with a parity
> error is also misleading (as a parity error would likely be signaled
> through a bus error). So for this case, also default to returning
> ABORTED COMMAND without any additional sense code. With this, the
> previous example error case becomes:
> 
> sd 0:0:0:0: [sda] tag#17 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
> sd 0:0:0:0: [sda] tag#17 Sense Key : Aborted Command [current]
> sd 0:0:0:0: [sda] tag#17 Add. Sense: No additional sense information
> sd 0:0:0:0: [sda] tag#17 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
> 
> Together with these fixes, refactor stat_table to make it more readable
> by putting the entries comments in front of the entries and using the
> defined status bits macros instead of hardcoded values.
> 
> Reported-by: Lorenz Brun <lorenz@brun.one>
> Reported-by: Brandon Schwartz <Brandon.Schwartz@wdc.com>
> Fixes: 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-scsi.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 27b15176db56..9b16c0f553e0 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -859,18 +859,14 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
>  		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
>  	};
>  	static const unsigned char stat_table[][4] = {
> -		/* Must be first because BUSY means no other bits valid */
> -		{0x80,		ABORTED_COMMAND, 0x47, 0x00},
> -		// Busy, fake parity for now
> -		{0x40,		ILLEGAL_REQUEST, 0x21, 0x04},
> -		// Device ready, unaligned write command
> -		{0x20,		HARDWARE_ERROR,  0x44, 0x00},
> -		// Device fault, internal target failure
> -		{0x08,		ABORTED_COMMAND, 0x47, 0x00},
> -		// Timed out in xfer, fake parity for now
> -		{0x04,		RECOVERED_ERROR, 0x11, 0x00},
> -		// Recovered ECC error	  Medium error, recovered
> -		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
> +		/* Busy: must be first because BUSY means no other bits valid */
> +		{ ATA_BUSY,	ABORTED_COMMAND, 0x00, 0x00 },

Hi Damien,

ata_to_sense_error() already has a check for ATA_BUSY. Perhaps we could add
a goto statement and fill ABORTED_COMMAND without looking up the same data in
the stat_table?

> +		/* Device fault: INTERNAL TARGET FAILURE */
> +		{ ATA_DF,	HARDWARE_ERROR,  0x44, 0x00 },
> +		/* Corrected data error */
> +		{ ATA_CORR,	RECOVERED_ERROR, 0x00, 0x00 },

I'm trying to understand what this "Corrected data error" is. ACS-6 does not
seem to have any references to such corrected errors. BIT(2) of STATUS field
is defined as "N/A or ALIGNMENT ERROR bit – See 6.2.2". Does it make sense
to translate this bit to "unaligned write command" instead?

Thanks,
Igor

> +
> +		{ 0xFF, 0xFF, 0xFF, 0xFF }, /* END mark */
>  	};
>  
>  	/*
> -- 
> 2.50.1
> 
> 

Re: [PATCH 2/2] ata: libata-scsi: Return aborted command when missing sense and result TF

[Igor Pylypiv]

On Wed, Jul 30, 2025 at 09:24:41AM +0900, Damien Le Moal wrote:
> ata_gen_ata_sense() is always called for a failed qc missing sense data
> so that a sense key, code and code qualifier can be generated using
> ata_to_sense_error() from the qc status and error fields of its result
> task file. However, if the qc does not have its result task file filled,
> ata_gen_ata_sense() returns early without setting a sense key.
> 
> Improve this by defaulting to returning ABORTED COMMAND without any
> additional sense code, since we do not know the reason for the failure.
> The same fix is also applied in ata_gen_passthru_sense() with the
> additional check that the qc failed (qc->err_mask is set).
> 
> Fixes: 816be86c7993 ("ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-scsi.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 9b16c0f553e0..57f674f51b0c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -938,6 +938,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
>  		ata_dev_dbg(dev,
>  			    "missing result TF: can't generate ATA PT sense data\n");
> +		if (qc->err_mask)
> +			ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
>  		return;
>  	}
>  
> @@ -992,8 +994,8 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>  
>  	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
>  		ata_dev_dbg(dev,
> -			    "missing result TF: can't generate sense data\n");
> -		return;
> +			    "Missing result TF: reporting aborted command\n");
> +		goto aborted;
>  	}
>  
>  	/* Use ata_to_sense_error() to map status register bits
> @@ -1004,13 +1006,15 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)

There is a redundant check in ata_gen_ata_sense(). qc->err_mask (is_error) is
already checked in ata_scsi_qc_complete() before it calls ata_gen_ata_sense().
 
	if (qc->err_mask ||
	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {

The function will be much cleaner once we remove this check. 

>  		ata_to_sense_error(tf->status, tf->error,
>  				   &sense_key, &asc, &ascq);
>  		ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
> -	} else {
> -		/* Could not decode error */
> -		ata_dev_warn(dev, "could not decode error status 0x%x err_mask 0x%x\n",
> -			     tf->status, qc->err_mask);
> -		ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
>  		return;
>  	}
> +
> +	/* Could not decode error */
> +	ata_dev_warn(dev,
> +		"Could not decode error 0x%x, status 0x%x (err_mask=0x%x)\n",
> +		tf->error, tf->status, qc->err_mask);
> +aborted:
> +	ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
>  }
>  
>  void ata_scsi_sdev_config(struct scsi_device *sdev)
> -- 
> 2.50.1
> 
> 

Re: [PATCH 1/2] ata: libata-scsi: Fix ata_to_sense_error() status handling

[Damien Le Moal]

On 8/2/25 05:04, Igor Pylypiv wrote:
> On Wed, Jul 30, 2025 at 09:24:40AM +0900, Damien Le Moal wrote:
>> Commit 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
>> inadvertantly added the entry 0x40 (ATA_DRDY) to the stat_table array in
>> the function ata_to_sense_error(). This entry ties a failed qc which has
>> a status filed equal to ATA_DRDY to the sense key ILLEGAL REQUEST with
>> the additional sense code UNALIGNED WRITE COMMAND. This entry will be
>> used to generate a failed qc sense key and sense code when the qc is
>> missing sense data and there is no match for the qc error field in the
>> sense_table array of ata_to_sense_error().
>>
>> As a result, for a failed qc for which we failed to get sense data (e.g.
>> read log 10h failed if qc is an NCQ command, or REQUEST SENSE EXT
>> command failed for the non-ncq case, the user very often end up seeing
>> the completely misleading "unaligned write command" error, even if qc
>> was not a write command. E.g.:
>>
>> sd 0:0:0:0: [sda] tag#12 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
>> sd 0:0:0:0: [sda] tag#12 Sense Key : Illegal Request [current]
>> sd 0:0:0:0: [sda] tag#12 Add. Sense: Unaligned write command
>> sd 0:0:0:0: [sda] tag#12 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
>> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
>>
>> Fix this by removing the ATA_DRDY entry from the stat_table array so
>> that we default to always returning ABORTED COMMAND without any
>> additional sense code, since we do not know any better. The entry 0x08
>> (ATA_DRQ) is also removed since signaling ABORTED COMMAND with a parity
>> error is also misleading (as a parity error would likely be signaled
>> through a bus error). So for this case, also default to returning
>> ABORTED COMMAND without any additional sense code. With this, the
>> previous example error case becomes:
>>
>> sd 0:0:0:0: [sda] tag#17 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
>> sd 0:0:0:0: [sda] tag#17 Sense Key : Aborted Command [current]
>> sd 0:0:0:0: [sda] tag#17 Add. Sense: No additional sense information
>> sd 0:0:0:0: [sda] tag#17 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
>> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
>>
>> Together with these fixes, refactor stat_table to make it more readable
>> by putting the entries comments in front of the entries and using the
>> defined status bits macros instead of hardcoded values.
>>
>> Reported-by: Lorenz Brun <lorenz@brun.one>
>> Reported-by: Brandon Schwartz <Brandon.Schwartz@wdc.com>
>> Fixes: 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  drivers/ata/libata-scsi.c | 20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 27b15176db56..9b16c0f553e0 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -859,18 +859,14 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
>>  		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
>>  	};
>>  	static const unsigned char stat_table[][4] = {
>> -		/* Must be first because BUSY means no other bits valid */
>> -		{0x80,		ABORTED_COMMAND, 0x47, 0x00},
>> -		// Busy, fake parity for now
>> -		{0x40,		ILLEGAL_REQUEST, 0x21, 0x04},
>> -		// Device ready, unaligned write command
>> -		{0x20,		HARDWARE_ERROR,  0x44, 0x00},
>> -		// Device fault, internal target failure
>> -		{0x08,		ABORTED_COMMAND, 0x47, 0x00},
>> -		// Timed out in xfer, fake parity for now
>> -		{0x04,		RECOVERED_ERROR, 0x11, 0x00},
>> -		// Recovered ECC error	  Medium error, recovered
>> -		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
>> +		/* Busy: must be first because BUSY means no other bits valid */
>> +		{ ATA_BUSY,	ABORTED_COMMAND, 0x00, 0x00 },
> 
> Hi Damien,
> 
> ata_to_sense_error() already has a check for ATA_BUSY. Perhaps we could add
> a goto statement and fill ABORTED_COMMAND without looking up the same data in
> the stat_table?

Yes, we could do that. I minimized the changes here since this is a bug fix.

>> +		/* Device fault: INTERNAL TARGET FAILURE */
>> +		{ ATA_DF,	HARDWARE_ERROR,  0x44, 0x00 },
>> +		/* Corrected data error */
>> +		{ ATA_CORR,	RECOVERED_ERROR, 0x00, 0x00 },
> 
> I'm trying to understand what this "Corrected data error" is. ACS-6 does not
> seem to have any references to such corrected errors. BIT(2) of STATUS field
> is defined as "N/A or ALIGNMENT ERROR bit – See 6.2.2". Does it make sense
> to translate this bit to "unaligned write command" instead?

This is an odd one. Indeed, ACS defines it as N/A or alignment error, but up to
SAT-3, its translation to sense data says "This condition is not considered an
error". The libata-scsi code has this defined as a recovered error since
forever, so I did not change it. SAT4 onward do not even define a translation
for this at all.

For modern drives, this is all almost completely irrelevant anyway as we get
sense data, unless there is a big issue with the drive preventing that, which is
the case this patch is for so that we do not get the non-sensical "unaligned error".

If you have a look at SAT specifications, you'll see that most of the error bits
translation in this function are "completely made up", or rather, as the SAT
specs allows, based on the 3ware driver translation (as noted in the comment for
the error table).

I do not want to hit regressions with excessive cleaning of this code, so I just
let it be.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] ata: libata-scsi: Return aborted command when missing sense and result TF

[Damien Le Moal]

On 8/2/25 05:28, Igor Pylypiv wrote:
> On Wed, Jul 30, 2025 at 09:24:41AM +0900, Damien Le Moal wrote:
>> ata_gen_ata_sense() is always called for a failed qc missing sense data
>> so that a sense key, code and code qualifier can be generated using
>> ata_to_sense_error() from the qc status and error fields of its result
>> task file. However, if the qc does not have its result task file filled,
>> ata_gen_ata_sense() returns early without setting a sense key.
>>
>> Improve this by defaulting to returning ABORTED COMMAND without any
>> additional sense code, since we do not know the reason for the failure.
>> The same fix is also applied in ata_gen_passthru_sense() with the
>> additional check that the qc failed (qc->err_mask is set).
>>
>> Fixes: 816be86c7993 ("ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  drivers/ata/libata-scsi.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 9b16c0f553e0..57f674f51b0c 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -938,6 +938,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>>  	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
>>  		ata_dev_dbg(dev,
>>  			    "missing result TF: can't generate ATA PT sense data\n");
>> +		if (qc->err_mask)
>> +			ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
>>  		return;
>>  	}
>>  
>> @@ -992,8 +994,8 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>>  
>>  	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
>>  		ata_dev_dbg(dev,
>> -			    "missing result TF: can't generate sense data\n");
>> -		return;
>> +			    "Missing result TF: reporting aborted command\n");
>> +		goto aborted;
>>  	}
>>  
>>  	/* Use ata_to_sense_error() to map status register bits
>> @@ -1004,13 +1006,15 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> 
> There is a redundant check in ata_gen_ata_sense(). qc->err_mask (is_error) is
> already checked in ata_scsi_qc_complete() before it calls ata_gen_ata_sense().
>  
> 	if (qc->err_mask ||
> 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> 
> The function will be much cleaner once we remove this check. 

Yep, we can remove the err_mask check.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] ata: libata-scsi: Return aborted command when missing sense and result TF

[Igor Pylypiv]

On Sat, Aug 02, 2025 at 12:33:05PM +0900, Damien Le Moal wrote:
> On 8/2/25 05:28, Igor Pylypiv wrote:
> > On Wed, Jul 30, 2025 at 09:24:41AM +0900, Damien Le Moal wrote:
> >> ata_gen_ata_sense() is always called for a failed qc missing sense data
> >> so that a sense key, code and code qualifier can be generated using
> >> ata_to_sense_error() from the qc status and error fields of its result
> >> task file. However, if the qc does not have its result task file filled,
> >> ata_gen_ata_sense() returns early without setting a sense key.
> >>
> >> Improve this by defaulting to returning ABORTED COMMAND without any
> >> additional sense code, since we do not know the reason for the failure.
> >> The same fix is also applied in ata_gen_passthru_sense() with the
> >> additional check that the qc failed (qc->err_mask is set).
> >>
> >> Fixes: 816be86c7993 ("ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >> ---
> >>  drivers/ata/libata-scsi.c | 18 +++++++++++-------
> >>  1 file changed, 11 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> >> index 9b16c0f553e0..57f674f51b0c 100644
> >> --- a/drivers/ata/libata-scsi.c
> >> +++ b/drivers/ata/libata-scsi.c
> >> @@ -938,6 +938,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >>  	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> >>  		ata_dev_dbg(dev,
> >>  			    "missing result TF: can't generate ATA PT sense data\n");
> >> +		if (qc->err_mask)
> >> +			ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
> >>  		return;
> >>  	}
> >>  
> >> @@ -992,8 +994,8 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >>  
> >>  	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> >>  		ata_dev_dbg(dev,
> >> -			    "missing result TF: can't generate sense data\n");
> >> -		return;
> >> +			    "Missing result TF: reporting aborted command\n");
> >> +		goto aborted;
> >>  	}
> >>  
> >>  	/* Use ata_to_sense_error() to map status register bits
> >> @@ -1004,13 +1006,15 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> > 
> > There is a redundant check in ata_gen_ata_sense(). qc->err_mask (is_error) is
> > already checked in ata_scsi_qc_complete() before it calls ata_gen_ata_sense().
> >  
> > 	if (qc->err_mask ||
> > 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > 
> > The function will be much cleaner once we remove this check. 
> 
> Yep, we can remove the err_mask check.
> 

To clarify, I mean that both conditions can be removed, not just the err_mask
check. In the current code the err_mask check always evaluates to true so
the right part of the OR expression is skipped due to lazy evaluation.

Thanks,
Igor

> 
> -- 
> Damien Le Moal
> Western Digital Research

Re: [PATCH 1/2] ata: libata-scsi: Fix ata_to_sense_error() status handling

[Igor Pylypiv]

On Sat, Aug 02, 2025 at 12:31:31PM +0900, Damien Le Moal wrote:
> On 8/2/25 05:04, Igor Pylypiv wrote:
> > On Wed, Jul 30, 2025 at 09:24:40AM +0900, Damien Le Moal wrote:
> >> Commit 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
> >> inadvertantly added the entry 0x40 (ATA_DRDY) to the stat_table array in
> >> the function ata_to_sense_error(). This entry ties a failed qc which has
> >> a status filed equal to ATA_DRDY to the sense key ILLEGAL REQUEST with
> >> the additional sense code UNALIGNED WRITE COMMAND. This entry will be
> >> used to generate a failed qc sense key and sense code when the qc is
> >> missing sense data and there is no match for the qc error field in the
> >> sense_table array of ata_to_sense_error().
> >>
> >> As a result, for a failed qc for which we failed to get sense data (e.g.
> >> read log 10h failed if qc is an NCQ command, or REQUEST SENSE EXT
> >> command failed for the non-ncq case, the user very often end up seeing
> >> the completely misleading "unaligned write command" error, even if qc
> >> was not a write command. E.g.:
> >>
> >> sd 0:0:0:0: [sda] tag#12 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
> >> sd 0:0:0:0: [sda] tag#12 Sense Key : Illegal Request [current]
> >> sd 0:0:0:0: [sda] tag#12 Add. Sense: Unaligned write command
> >> sd 0:0:0:0: [sda] tag#12 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
> >> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
> >>
> >> Fix this by removing the ATA_DRDY entry from the stat_table array so
> >> that we default to always returning ABORTED COMMAND without any
> >> additional sense code, since we do not know any better. The entry 0x08
> >> (ATA_DRQ) is also removed since signaling ABORTED COMMAND with a parity
> >> error is also misleading (as a parity error would likely be signaled
> >> through a bus error). So for this case, also default to returning
> >> ABORTED COMMAND without any additional sense code. With this, the
> >> previous example error case becomes:
> >>
> >> sd 0:0:0:0: [sda] tag#17 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
> >> sd 0:0:0:0: [sda] tag#17 Sense Key : Aborted Command [current]
> >> sd 0:0:0:0: [sda] tag#17 Add. Sense: No additional sense information
> >> sd 0:0:0:0: [sda] tag#17 CDB: Read(10) 28 00 00 00 10 00 00 00 08 00
> >> I/O error, dev sda, sector 4096 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
> >>
> >> Together with these fixes, refactor stat_table to make it more readable
> >> by putting the entries comments in front of the entries and using the
> >> defined status bits macros instead of hardcoded values.
> >>
> >> Reported-by: Lorenz Brun <lorenz@brun.one>
> >> Reported-by: Brandon Schwartz <Brandon.Schwartz@wdc.com>
> >> Fixes: 8ae720449fca ("libata: whitespace fixes in ata_to_sense_error()")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >> ---
> >>  drivers/ata/libata-scsi.c | 20 ++++++++------------
> >>  1 file changed, 8 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> >> index 27b15176db56..9b16c0f553e0 100644
> >> --- a/drivers/ata/libata-scsi.c
> >> +++ b/drivers/ata/libata-scsi.c
> >> @@ -859,18 +859,14 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
> >>  		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
> >>  	};
> >>  	static const unsigned char stat_table[][4] = {
> >> -		/* Must be first because BUSY means no other bits valid */
> >> -		{0x80,		ABORTED_COMMAND, 0x47, 0x00},
> >> -		// Busy, fake parity for now
> >> -		{0x40,		ILLEGAL_REQUEST, 0x21, 0x04},
> >> -		// Device ready, unaligned write command
> >> -		{0x20,		HARDWARE_ERROR,  0x44, 0x00},
> >> -		// Device fault, internal target failure
> >> -		{0x08,		ABORTED_COMMAND, 0x47, 0x00},
> >> -		// Timed out in xfer, fake parity for now
> >> -		{0x04,		RECOVERED_ERROR, 0x11, 0x00},
> >> -		// Recovered ECC error	  Medium error, recovered
> >> -		{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
> >> +		/* Busy: must be first because BUSY means no other bits valid */
> >> +		{ ATA_BUSY,	ABORTED_COMMAND, 0x00, 0x00 },
> > 
> > Hi Damien,
> > 
> > ata_to_sense_error() already has a check for ATA_BUSY. Perhaps we could add
> > a goto statement and fill ABORTED_COMMAND without looking up the same data in
> > the stat_table?
> 
> Yes, we could do that. I minimized the changes here since this is a bug fix.
> 
> >> +		/* Device fault: INTERNAL TARGET FAILURE */
> >> +		{ ATA_DF,	HARDWARE_ERROR,  0x44, 0x00 },
> >> +		/* Corrected data error */
> >> +		{ ATA_CORR,	RECOVERED_ERROR, 0x00, 0x00 },
> > 
> > I'm trying to understand what this "Corrected data error" is. ACS-6 does not
> > seem to have any references to such corrected errors. BIT(2) of STATUS field
> > is defined as "N/A or ALIGNMENT ERROR bit – See 6.2.2". Does it make sense
> > to translate this bit to "unaligned write command" instead?
> 
> This is an odd one. Indeed, ACS defines it as N/A or alignment error, but up to
> SAT-3, its translation to sense data says "This condition is not considered an
> error". The libata-scsi code has this defined as a recovered error since
> forever, so I did not change it. SAT4 onward do not even define a translation
> for this at all.
> 
> For modern drives, this is all almost completely irrelevant anyway as we get
> sense data, unless there is a big issue with the drive preventing that, which is
> the case this patch is for so that we do not get the non-sensical "unaligned error".
> 
> If you have a look at SAT specifications, you'll see that most of the error bits
> translation in this function are "completely made up", or rather, as the SAT
> specs allows, based on the 3ware driver translation (as noted in the comment for
> the error table).
> 
> I do not want to hit regressions with excessive cleaning of this code, so I just
> let it be.

Thank you for the details, Damien!

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

> 
> -- 
> Damien Le Moal
> Western Digital Research

Re: [PATCH 2/2] ata: libata-scsi: Return aborted command when missing sense and result TF

[Damien Le Moal]

On 8/3/25 2:49 AM, Igor Pylypiv wrote:
>>> There is a redundant check in ata_gen_ata_sense(). qc->err_mask (is_error) is
>>> already checked in ata_scsi_qc_complete() before it calls ata_gen_ata_sense().
>>>  
>>> 	if (qc->err_mask ||
>>> 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
>>>
>>> The function will be much cleaner once we remove this check. 
>>
>> Yep, we can remove the err_mask check.
>>
> 
> To clarify, I mean that both conditions can be removed, not just the err_mask
> check. In the current code the err_mask check always evaluates to true so
> the right part of the OR expression is skipped due to lazy evaluation.

Yes, we can simplify this.

-- 
Damien Le Moal
Western Digital Research