[PATCH] ata: libata-scsi: fix requeue of deferred ATA PASS-THROUGH commands

[PATCH] ata: libata-scsi: fix requeue of deferred ATA PASS-THROUGH commands

[Igor Pylypiv]

Commit 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
introduced ata_scsi_requeue_deferred_qc() to handle commands deferred
during resets or NCQ failures. This deferral logic completed commands
with DID_SOFT_ERROR to trigger a retry in the SCSI mid-layer.

However, DID_SOFT_ERROR is subject to scsi_cmd_retry_allowed() checks.
ATA PASS-THROUGH commands sent via SG_IO ioctl have scmd->allowed set
to zero. This causes the mid-layer to fail the command immediately
instead of retrying, even though the command was never actually issued
to the hardware.

Switch to DID_REQUEUE to ensure these commands are inserted back into
the request queue regardless of retry limits.

Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 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 3b65df914ebb..0236394900cc 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1692,7 +1692,7 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
 	/*
 	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
 	 * do not try to be smart about what to do with this deferred command
-	 * and simply retry it by completing it with DID_SOFT_ERROR.
+	 * and simply requeue it by completing it with DID_REQUEUE.
 	 */
 	if (!qc)
 		return;
@@ -1701,7 +1701,7 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
 	ap->deferred_qc = NULL;
 	cancel_work(&ap->deferred_qc_work);
 	ata_qc_free(qc);
-	scmd->result = (DID_SOFT_ERROR << 16);
+	set_host_byte(scmd, DID_REQUEUE);
 	scsi_done(scmd);
 }
 
-- 
2.53.0.1213.gd9a14994de-goog

Re: [PATCH] ata: libata-scsi: fix requeue of deferred ATA PASS-THROUGH commands

[Damien Le Moal]

On 4/11/26 01:15, Igor Pylypiv wrote:
> Commit 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> introduced ata_scsi_requeue_deferred_qc() to handle commands deferred
> during resets or NCQ failures. This deferral logic completed commands
> with DID_SOFT_ERROR to trigger a retry in the SCSI mid-layer.
> 
> However, DID_SOFT_ERROR is subject to scsi_cmd_retry_allowed() checks.
> ATA PASS-THROUGH commands sent via SG_IO ioctl have scmd->allowed set
> to zero. This causes the mid-layer to fail the command immediately
> instead of retrying, even though the command was never actually issued
> to the hardware.
> 
> Switch to DID_REQUEUE to ensure these commands are inserted back into
> the request queue regardless of retry limits.

I really thought that DID_SOFT_ERROR was not decrementing the retry counter.
Checking the code again, I was wrong. Good catch !

> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: libata-scsi: fix requeue of deferred ATA PASS-THROUGH commands

[Niklas Cassel]

On Fri, Apr 10, 2026 at 04:15:19PM -0700, Igor Pylypiv wrote:
> Commit 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> introduced ata_scsi_requeue_deferred_qc() to handle commands deferred
> during resets or NCQ failures. This deferral logic completed commands
> with DID_SOFT_ERROR to trigger a retry in the SCSI mid-layer.
> 
> However, DID_SOFT_ERROR is subject to scsi_cmd_retry_allowed() checks.
> ATA PASS-THROUGH commands sent via SG_IO ioctl have scmd->allowed set
> to zero. This causes the mid-layer to fail the command immediately
> instead of retrying, even though the command was never actually issued
> to the hardware.
> 
> Switch to DID_REQUEUE to ensure these commands are inserted back into
> the request queue regardless of retry limits.
> 
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  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 3b65df914ebb..0236394900cc 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1692,7 +1692,7 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>  	/*
>  	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
>  	 * do not try to be smart about what to do with this deferred command
> -	 * and simply retry it by completing it with DID_SOFT_ERROR.
> +	 * and simply requeue it by completing it with DID_REQUEUE.
>  	 */
>  	if (!qc)
>  		return;
> @@ -1701,7 +1701,7 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>  	ap->deferred_qc = NULL;
>  	cancel_work(&ap->deferred_qc_work);
>  	ata_qc_free(qc);
> -	scmd->result = (DID_SOFT_ERROR << 16);
> +	set_host_byte(scmd, DID_REQUEUE);

set_host_byte() will set the host byte, but it will keep the status byte
and the ML byte intact.

By using the assignment operator, I assumed that Damien intentionally
wanted to clear the status byte and the ML byte.

My point is that using set_host_byte() is a logical change.
If we want to stop clearing the status byte and the ML byte, then I think
that change should be in a separate commit, with a proper motivation/commit
message.

However, for the fix patch itself, I think we should just do:
-	scmd->result = (DID_SOFT_ERROR << 16);
+	scmd->result = (DID_REQUEUE << 16);


If that is sufficient to fix your observed problem.

I would also be happy to see a follow up patch that changes to use
set_host_byte(), if there is a motivation that can motivate why that change
is safe/valid.


Kind regards,
Niklas

Re: [PATCH] ata: libata-scsi: fix requeue of deferred ATA PASS-THROUGH commands

[Igor Pylypiv]

On Sun, Apr 12, 2026 at 12:42:46PM +0200, Niklas Cassel wrote:
> On Fri, Apr 10, 2026 at 04:15:19PM -0700, Igor Pylypiv wrote:
> > Commit 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> > introduced ata_scsi_requeue_deferred_qc() to handle commands deferred
> > during resets or NCQ failures. This deferral logic completed commands
> > with DID_SOFT_ERROR to trigger a retry in the SCSI mid-layer.
> > 
> > However, DID_SOFT_ERROR is subject to scsi_cmd_retry_allowed() checks.
> > ATA PASS-THROUGH commands sent via SG_IO ioctl have scmd->allowed set
> > to zero. This causes the mid-layer to fail the command immediately
> > instead of retrying, even though the command was never actually issued
> > to the hardware.
> > 
> > Switch to DID_REQUEUE to ensure these commands are inserted back into
> > the request queue regardless of retry limits.
> > 
> > Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  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 3b65df914ebb..0236394900cc 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1692,7 +1692,7 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
> >  	/*
> >  	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
> >  	 * do not try to be smart about what to do with this deferred command
> > -	 * and simply retry it by completing it with DID_SOFT_ERROR.
> > +	 * and simply requeue it by completing it with DID_REQUEUE.
> >  	 */
> >  	if (!qc)
> >  		return;
> > @@ -1701,7 +1701,7 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
> >  	ap->deferred_qc = NULL;
> >  	cancel_work(&ap->deferred_qc_work);
> >  	ata_qc_free(qc);
> > -	scmd->result = (DID_SOFT_ERROR << 16);
> > +	set_host_byte(scmd, DID_REQUEUE);
> 
> set_host_byte() will set the host byte, but it will keep the status byte
> and the ML byte intact.
> 
> By using the assignment operator, I assumed that Damien intentionally
> wanted to clear the status byte and the ML byte.
> 
> My point is that using set_host_byte() is a logical change.
> If we want to stop clearing the status byte and the ML byte, then I think
> that change should be in a separate commit, with a proper motivation/commit
> message.
> 
> However, for the fix patch itself, I think we should just do:
> -	scmd->result = (DID_SOFT_ERROR << 16);
> +	scmd->result = (DID_REQUEUE << 16);
> 

Hi Niklas,

Thank you for pointing it out. I agree. Switching to set_host_byte()
is logically a different change from the problem that this commit
is fixing. There is no particular need for using set_host_byte().

I'll send a v2 to drop set_host_byte().

Thanks,
Igor

> 
> If that is sufficient to fix your observed problem.
> 
> I would also be happy to see a follow up patch that changes to use
> set_host_byte(), if there is a motivation that can motivate why that change
> is safe/valid.
> 
> 
> Kind regards,
> Niklas