Re: [PATCH v5 2/2] ata: libata-scsi: avoid Non-NCQ command starvation

[Niklas Cassel <cassel@kernel.org>]

On Sun, Jan 04, 2026 at 05:22:03PM +0900, Damien Le Moal wrote:
> @@ -1658,8 +1658,73 @@ static void ata_qc_done(struct ata_queued_cmd *qc)
>  	done(cmd);
>  }
>  
> +void ata_scsi_deferred_qc_work(struct work_struct *work)
> +{
> +	struct ata_port *ap =
> +		container_of(work, struct ata_port, deferred_qc_work);
> +	struct ata_queued_cmd *qc;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(ap->lock, flags);
> +
> +	/*
> +	 * If we still have a deferred qc and we are not in EH, issue it. In
> +	 * such case, we should not need any more deferring the qc, so warn if
> +	 * qc_defer() says otherwise.
> +	 */
> +	qc = ap->deferred_qc;
> +	if (qc && !ata_port_eh_scheduled(ap)) {
> +		WARN_ON_ONCE(ap->ops->qc_defer(qc));
> +		ap->deferred_qc = NULL;
> +		ata_qc_issue(qc);
> +	}
> +
> +	spin_unlock_irqrestore(ap->lock, flags);
> +}
> +
> +void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
> +{
> +	struct ata_queued_cmd *qc = ap->deferred_qc;
> +	struct scsi_cmnd *scmd;

Since ap->deferred_qc needs to be checked while holding the spinlock,
in order to synchronize with ata_scsi_deferred_qc_work(), please add a
lockdep_assert_held(ap->lock) here.

Yes, I know that you had multiple lockdep_assert_held() in V2.

But I think that a single lockdep_assert_held() in either 
ata_scsi_requeue_deferred_qc() or ata_scsi_qc_complete() is warranted
(and yes, it is a no-op on production systems (where lockdep is disabled)).

Other than that, looks good to me, no need for a resend, you canfixup when
applying:

Reviewed-by: Niklas Cassel <cassel@kernel.org>


> +
> +	/*
> +	 * 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.
> +	 */
> +	if (!qc)
> +		return;
> +
> +	scmd = qc->scsicmd;
> +	ap->deferred_qc = NULL;
> +	ata_qc_free(qc);
> +	scmd->result = (DID_SOFT_ERROR << 16);
> +	scsi_done(scmd);
> +}