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

Igor Pylypiv
2026-04-12T15:24:45Z

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