ata: libata: Clear DID_TIME_OUT for ATA PT commands with sense data

PATCH Superseded Niklas Cassel 2 participants 4 messages Sep 9, 2024 libata

Share & tools

Niklas Cassel

author maintainer

2024-09-09 08:48 UTC #0 lore raw

PATCH v2 1/1 ata: libata: Clear DID_TIME_OUT for ATA PT commands with sense data

When ata_qc_complete() schedules a command for EH using ata_qc_schedule_eh(), blk_abort_request() will be called, which leads to req->q->mq_ops->timeout() / scsi_timeout() being called.

scsi_timeout(), if the LLDD has no abort handler (libata has no abort handler), will set host byte to DID_TIME_OUT, and then call scsi_eh_scmd_add() to add the command to EH.

Thus, when commands first enter libata's EH strategy_handler, all the commands that have been added to EH will have DID_TIME_OUT set.

libata has its own flag (AC_ERR_TIMEOUT), that it sets for commands that have not received a completion at the time of entering EH.

Thus, libata doesn't really care about DID_TIME_OUT at all, and currently clears the host byte at the end of EH, in ata_scsi_qc_complete(), before scsi_eh_finish_cmd() is called.

However, this clearing in ata_scsi_qc_complete() is currently only done for commands that are not ATA passthrough commands.

Since the host byte is visible in the completion that we return to user space for ATA passthrough commands, for ATA passthrough commands that got completed via EH (commands with sense data), the user will incorrectly see:
ATA pass-through(16): transport error: Host_status=0x03 [DID_TIME_OUT]

Fix this by moving the clearing of the host byte (which is currently only done for commands that are not ATA passthrough commands) from ata_scsi_qc_complete() to the start of EH (regardless if the command is ATA passthrough or not).

This will make sure that we:
-Correctly clear DID_TIME_OUT for both ATA passthrough commands and
commands that are not ATA passthrough commands.
-Do not needlessly clear the host byte for commands that did not go via EH.
ata_scsi_qc_complete() is called both for commands that are completed normally (without going via EH), and for commands that went via EH, however, only commands that went via EH will have DID_TIME_OUT set.

Fixes 24aeebbf8ea9 ("scsi: ata: libata: Change ata_eh_request_sense() to not set CHECK_CONDITION")

Reported-by Igor Pylypiv <[email protected]>

Closes https://lore.kernel.org/linux-ide/[email protected]/ open here

Tested-by Igor Pylypiv <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
Changes since v1:
-Picked up tags from Igor.
-Added Fixes tag.
-Improved the commit message to clearly state that this is currently a
real bug for ATA PT commands with sense data.

 drivers/ata/libata-eh.c   | 9 +++++++++
 drivers/ata/libata-scsi.c | 3 ---
 2 files changed, 9 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +9 -0

index 7de97ee8e78b..450e9bd96c97 100644
@@ -630,6 +630,15 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 	list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {
 		struct ata_queued_cmd *qc;
 
+		/*
+		 * If the scmd was added to EH, via ata_qc_schedule_eh() ->
+		 * scsi_timeout() -> scsi_eh_scmd_add(), scsi_timeout() will
+		 * have set DID_TIME_OUT (since libata does not have an abort
+		 * handler). Thus to clear the DID_TIME_OUT, we clear the host
+		 * byte (but keep the SCSI ML and status byte).
+		 */
+		scmd->result &= 0x0000ffff;
+
 		ata_qc_for_each_raw(ap, qc, i) {
 			if (qc->flags & ATA_QCFLAG_ACTIVE &&
 			    qc->scsicmd == scmd)

drivers/ata/libata-scsi.c +0 -3

index 3a442f564b0d..6a90062c8b55 100644
@@ -1680,9 +1680,6 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 			set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
 	} else if (is_error && !have_sense) {
 		ata_gen_ata_sense(qc);
-	} else {
-		/* Keep the SCSI ML and status byte, clear host byte. */
-		cmd->result &= 0x0000ffff;
 	}
 
 	ata_qc_done(qc);

Signature · 1 line

2.46.0

Damien Le Moal

maintainer

2024-09-09 12:52 UTC #1 lore raw

Quoted 61 lines · Niklas Cassel

When ata_qc_complete() schedules a command for EH using ata_qc_schedule_eh(), blk_abort_request() will be called, which leads to req->q->mq_ops->timeout() / scsi_timeout() being called.
scsi_timeout(), if the LLDD has no abort handler (libata has no abort handler), will set host byte to DID_TIME_OUT, and then call scsi_eh_scmd_add() to add the command to EH.
Thus, when commands first enter libata's EH strategy_handler, all the commands that have been added to EH will have DID_TIME_OUT set.
libata has its own flag (AC_ERR_TIMEOUT), that it sets for commands that have not received a completion at the time of entering EH.
Thus, libata doesn't really care about DID_TIME_OUT at all, and currently clears the host byte at the end of EH, in ata_scsi_qc_complete(), before scsi_eh_finish_cmd() is called.
However, this clearing in ata_scsi_qc_complete() is currently only done for commands that are not ATA passthrough commands.
Since the host byte is visible in the completion that we return to user space for ATA passthrough commands, for ATA passthrough commands that got completed via EH (commands with sense data), the user will incorrectly see:
ATA pass-through(16): transport error: Host_status=0x03 [DID_TIME_OUT]
Fix this by moving the clearing of the host byte (which is currently only done for commands that are not ATA passthrough commands) from ata_scsi_qc_complete() to the start of EH (regardless if the command is ATA passthrough or not).
This will make sure that we:
(quoted diff) +0 -2
-Correctly clear DID_TIME_OUT for both ATA passthrough commands and
 commands that are not ATA passthrough commands.
-Do not needlessly clear the host byte for commands that did not go via EH.
 ata_scsi_qc_complete() is called both for commands that are completed
 normally (without going via EH), and for commands that went via EH,
 however, only commands that went via EH will have DID_TIME_OUT set.
Fixes 24aeebbf8ea9 ("scsi: ata: libata: Change ata_eh_request_sense() to not set CHECK_CONDITION")
Reported-by Igor Pylypiv <[email protected]>
Closes https://lore.kernel.org/linux-ide/[email protected]/ open here
Tested-by Igor Pylypiv <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
Changes since v1:
(quoted diff) +0 -3
-Picked up tags from Igor.
-Added Fixes tag.
-Improved the commit message to clearly state that this is currently a
 real bug for ATA PT commands with sense data.
 
 drivers/ata/libata-eh.c   | 9 +++++++++
 drivers/ata/libata-scsi.c | 3 ---
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/ata/libata-eh.c +0 -0
index 7de97ee8e78b..450e9bd96c97 100644
@@ -630,6 +630,15 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 	list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 640

Quoted 10 lines

drivers/ata/libata-eh.c +8 -0

 		struct ata_queued_cmd *qc;
 
+		/*
+		 * If the scmd was added to EH, via ata_qc_schedule_eh() ->
+		 * scsi_timeout() -> scsi_eh_scmd_add(), scsi_timeout() will
+		 * have set DID_TIME_OUT (since libata does not have an abort
+		 * handler). Thus to clear the DID_TIME_OUT, we clear the host
+		 * byte (but keep the SCSI ML and status byte).
+		 */
+		scmd->result &= 0x0000ffff;

I know it was like that, but why not:

		set_host_byte(scmd, 0);
or
		set_host_byte(scmd, DID_OK);

set_host_byte() uses the mask 0xff00ffff, since the upper 8 bits seem to be
ignored: bits [0..7] are the status byte, [16..23] are the host byte and bits [8..15] are the message byte but that is unused.

Quoted 18 lines

drivers/ata/libata-eh.c +1 -0

+
 		ata_qc_for_each_raw(ap, qc, i) {
 			if (qc->flags & ATA_QCFLAG_ACTIVE &&
 			    qc->scsicmd == scmd)

drivers/ata/libata-scsi.c +0 -3

index 3a442f564b0d..6a90062c8b55 100644
@@ -1680,9 +1680,6 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 			set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
 	} else if (is_error && !have_sense) {
 		ata_gen_ata_sense(qc);
-	} else {
-		/* Keep the SCSI ML and status byte, clear host byte. */
-		cmd->result &= 0x0000ffff;
 	}
 
 	ata_qc_done(qc);

Signature · 2 lines

Damien Le Moal
Western Digital Research

Niklas Cassel

author maintainer

2024-09-09 13:08 UTC #2 lore raw

Quoted 75 lines · Damien Le Moal

Quoted 61 lines · Niklas Cassel
When ata_qc_complete() schedules a command for EH using ata_qc_schedule_eh(), blk_abort_request() will be called, which leads to req->q->mq_ops->timeout() / scsi_timeout() being called.
scsi_timeout(), if the LLDD has no abort handler (libata has no abort handler), will set host byte to DID_TIME_OUT, and then call scsi_eh_scmd_add() to add the command to EH.
Thus, when commands first enter libata's EH strategy_handler, all the commands that have been added to EH will have DID_TIME_OUT set.
libata has its own flag (AC_ERR_TIMEOUT), that it sets for commands that have not received a completion at the time of entering EH.
Thus, libata doesn't really care about DID_TIME_OUT at all, and currently clears the host byte at the end of EH, in ata_scsi_qc_complete(), before scsi_eh_finish_cmd() is called.
However, this clearing in ata_scsi_qc_complete() is currently only done for commands that are not ATA passthrough commands.
Since the host byte is visible in the completion that we return to user space for ATA passthrough commands, for ATA passthrough commands that got completed via EH (commands with sense data), the user will incorrectly see:
ATA pass-through(16): transport error: Host_status=0x03 [DID_TIME_OUT]
Fix this by moving the clearing of the host byte (which is currently only done for commands that are not ATA passthrough commands) from ata_scsi_qc_complete() to the start of EH (regardless if the command is ATA passthrough or not).
This will make sure that we:
(quoted diff) +0 -2
-Correctly clear DID_TIME_OUT for both ATA passthrough commands and
 commands that are not ATA passthrough commands.
-Do not needlessly clear the host byte for commands that did not go via EH.
 ata_scsi_qc_complete() is called both for commands that are completed
 normally (without going via EH), and for commands that went via EH,
 however, only commands that went via EH will have DID_TIME_OUT set.
Fixes 24aeebbf8ea9 ("scsi: ata: libata: Change ata_eh_request_sense() to not set CHECK_CONDITION")
Reported-by Igor Pylypiv <[email protected]>
Closes https://lore.kernel.org/linux-ide/[email protected]/ open here
Tested-by Igor Pylypiv <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
Changes since v1:
(quoted diff) +0 -3
-Picked up tags from Igor.
-Added Fixes tag.
-Improved the commit message to clearly state that this is currently a
 real bug for ATA PT commands with sense data.
 
 drivers/ata/libata-eh.c   | 9 +++++++++
 drivers/ata/libata-scsi.c | 3 ---
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/ata/libata-eh.c +0 -0
index 7de97ee8e78b..450e9bd96c97 100644
@@ -630,6 +630,15 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 	list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {
drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 640
Quoted 10 lines
drivers/ata/libata-eh.c +8 -0
 		struct ata_queued_cmd *qc;
 
+		/*
+		 * If the scmd was added to EH, via ata_qc_schedule_eh() ->
+		 * scsi_timeout() -> scsi_eh_scmd_add(), scsi_timeout() will
+		 * have set DID_TIME_OUT (since libata does not have an abort
+		 * handler). Thus to clear the DID_TIME_OUT, we clear the host
+		 * byte (but keep the SCSI ML and status byte).
+		 */
+		scmd->result &= 0x0000ffff;
I know it was like that, but why not:

Quoted 5 lines

		set_host_byte(scmd, 0);
or
		set_host_byte(scmd, DID_OK);

No particular reason. Since we basically just moving the code, it made sense to keep it similar to the original code, but I can submit a v3 that instead does:
set_host_byte(scmd, DID_OK);

if you prefer that.

Strictly speaking, that would probably require us to drop Igor's Tested-by though (even if the generated code for an optimizing compiler ought to generate the same code).

Quoted 4 lines

set_host_byte() uses the mask 0xff00ffff, since the upper 8 bits seem to be
ignored: bits [0..7] are the status byte, [16..23] are the host byte and bits [8..15] are the message byte but that is unused.

Nit: 8..16 is the SCSI midlayer byte, not message byte, see 36ebf1e2aa14 ("scsi: core: Add error codes for internal SCSI midlayer use")

Kind regards,
Niklas

Damien Le Moal

maintainer

2024-09-09 13:45 UTC #3 lore raw

Quoted 80 lines · Niklas Cassel

Quoted 79 lines · Damien Le Moal
Quoted 61 lines · Niklas Cassel
When ata_qc_complete() schedules a command for EH using ata_qc_schedule_eh(), blk_abort_request() will be called, which leads to req->q->mq_ops->timeout() / scsi_timeout() being called.
scsi_timeout(), if the LLDD has no abort handler (libata has no abort handler), will set host byte to DID_TIME_OUT, and then call scsi_eh_scmd_add() to add the command to EH.
Thus, when commands first enter libata's EH strategy_handler, all the commands that have been added to EH will have DID_TIME_OUT set.
libata has its own flag (AC_ERR_TIMEOUT), that it sets for commands that have not received a completion at the time of entering EH.
Thus, libata doesn't really care about DID_TIME_OUT at all, and currently clears the host byte at the end of EH, in ata_scsi_qc_complete(), before scsi_eh_finish_cmd() is called.
However, this clearing in ata_scsi_qc_complete() is currently only done for commands that are not ATA passthrough commands.
Since the host byte is visible in the completion that we return to user space for ATA passthrough commands, for ATA passthrough commands that got completed via EH (commands with sense data), the user will incorrectly see:
ATA pass-through(16): transport error: Host_status=0x03 [DID_TIME_OUT]
Fix this by moving the clearing of the host byte (which is currently only done for commands that are not ATA passthrough commands) from ata_scsi_qc_complete() to the start of EH (regardless if the command is ATA passthrough or not).
This will make sure that we:
(quoted diff) +0 -2
-Correctly clear DID_TIME_OUT for both ATA passthrough commands and
 commands that are not ATA passthrough commands.
-Do not needlessly clear the host byte for commands that did not go via EH.
 ata_scsi_qc_complete() is called both for commands that are completed
 normally (without going via EH), and for commands that went via EH,
 however, only commands that went via EH will have DID_TIME_OUT set.
Fixes 24aeebbf8ea9 ("scsi: ata: libata: Change ata_eh_request_sense() to not set CHECK_CONDITION")
Reported-by Igor Pylypiv <[email protected]>
Closes https://lore.kernel.org/linux-ide/[email protected]/ open here
Tested-by Igor Pylypiv <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
Changes since v1:
(quoted diff) +0 -3
-Picked up tags from Igor.
-Added Fixes tag.
-Improved the commit message to clearly state that this is currently a
 real bug for ATA PT commands with sense data.
 
 drivers/ata/libata-eh.c   | 9 +++++++++
 drivers/ata/libata-scsi.c | 3 ---
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/ata/libata-eh.c +0 -0
index 7de97ee8e78b..450e9bd96c97 100644
@@ -630,6 +630,15 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 	list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {
drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 640
Quoted 10 lines
drivers/ata/libata-eh.c +8 -0
 		struct ata_queued_cmd *qc;
 
+		/*
+		 * If the scmd was added to EH, via ata_qc_schedule_eh() ->
+		 * scsi_timeout() -> scsi_eh_scmd_add(), scsi_timeout() will
+		 * have set DID_TIME_OUT (since libata does not have an abort
+		 * handler). Thus to clear the DID_TIME_OUT, we clear the host
+		 * byte (but keep the SCSI ML and status byte).
+		 */
+		scmd->result &= 0x0000ffff;
I know it was like that, but why not:
		set_host_byte(scmd, 0);
or
		set_host_byte(scmd, DID_OK);

Quoted 8 lines

Quoted 1 line
?
No particular reason. Since we basically just moving the code, it made sense to keep it similar to the original code, but I can submit a v3 that instead does:
set_host_byte(scmd, DID_OK);
if you prefer that.

I do prefer it. The magic 0x0000ffff mask is not exactly clear (the comment helps though)...

Side note: patching scsi to define macros for status, ML and host byte masks and shifts would be very nice :)

Quoted 17 lines

Strictly speaking, that would probably require us to drop Igor's Tested-by though (even if the generated code for an optimizing compiler ought to generate the same code).
Quoted 4 lines
set_host_byte() uses the mask 0xff00ffff, since the upper 8 bits seem to be
ignored: bits [0..7] are the status byte, [16..23] are the host byte and bits [8..15] are the message byte but that is unused.
Nit: 8..16 is the SCSI midlayer byte, not message byte, see 36ebf1e2aa14 ("scsi: core: Add error codes for internal SCSI midlayer use")
Kind regards,
Niklas

Signature · 2 lines

Damien Le Moal
Western Digital Research