| Message ID | 20240909154237.3656000-2-cassel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ata: libata: Clear DID_TIME_OUT for ATA PT commands with sense data | expand |
On Mon, Sep 09, 2024 at 05:42:38PM +0200, Niklas Cassel wrote: > 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). > > While at it, use the proper helper function to clear the host byte, rather > than open coding the clearing. > > 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 <ipylypiv@google.com> > Closes: https://lore.kernel.org/linux-ide/ZttIN8He8TOZ7Lct@google.com/ Tested-by: Igor Pylypiv <ipylypiv@google.com> Thanks, Igor > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > Changes since v2: use set_host_byte() as suggested by Damien. > > drivers/ata/libata-eh.c | 8 ++++++++ > drivers/ata/libata-scsi.c | 3 --- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 7de97ee8e78b..703f5ce26765 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -630,6 +630,14 @@ 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 DID_TIME_OUT, clear the host byte. > + */ > + set_host_byte(scmd, DID_OK); > + > ata_qc_for_each_raw(ap, qc, i) { > if (qc->flags & ATA_QCFLAG_ACTIVE && > qc->scsicmd == scmd) > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 3a442f564b0d..6a90062c8b55 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -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); > -- > 2.46.0 >
On 9/9/24 17:42, Niklas Cassel wrote: > 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). > > While at it, use the proper helper function to clear the host byte, rather > than open coding the clearing. > > 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 <ipylypiv@google.com> > Closes: https://lore.kernel.org/linux-ide/ZttIN8He8TOZ7Lct@google.com/ > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > Changes since v2: use set_host_byte() as suggested by Damien. > > drivers/ata/libata-eh.c | 8 ++++++++ > drivers/ata/libata-scsi.c | 3 --- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 7de97ee8e78b..703f5ce26765 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -630,6 +630,14 @@ 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 DID_TIME_OUT, clear the host byte. > + */ > + set_host_byte(scmd, DID_OK); > + > ata_qc_for_each_raw(ap, qc, i) { > if (qc->flags & ATA_QCFLAG_ACTIVE && > qc->scsicmd == scmd) > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 3a442f564b0d..6a90062c8b55 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -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); So all these accessors have not been in vain ... Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 9/10/24 00:42, Niklas Cassel wrote: > 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). > > While at it, use the proper helper function to clear the host byte, rather > than open coding the clearing. > > 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 <ipylypiv@google.com> > Closes: https://lore.kernel.org/linux-ide/ZttIN8He8TOZ7Lct@google.com/ > Signed-off-by: Niklas Cassel <cassel@kernel.org> Applied to for-6.12. Thanks !
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 7de97ee8e78b..703f5ce26765 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -630,6 +630,14 @@ 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 DID_TIME_OUT, clear the host byte. + */ + set_host_byte(scmd, DID_OK); + ata_qc_for_each_raw(ap, qc, i) { if (qc->flags & ATA_QCFLAG_ACTIVE && qc->scsicmd == scmd) diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 3a442f564b0d..6a90062c8b55 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -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);
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). While at it, use the proper helper function to clear the host byte, rather than open coding the clearing. 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 <ipylypiv@google.com> Closes: https://lore.kernel.org/linux-ide/ZttIN8He8TOZ7Lct@google.com/ Signed-off-by: Niklas Cassel <cassel@kernel.org> --- Changes since v2: use set_host_byte() as suggested by Damien. drivers/ata/libata-eh.c | 8 ++++++++ drivers/ata/libata-scsi.c | 3 --- 2 files changed, 8 insertions(+), 3 deletions(-)