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

[Igor Pylypiv <ipylypiv@google.com>]

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