Re: [PATCH v4 4/4] ata: libata-scsi: do not needlessly defer commands when using PMP with FBS

[Niklas Cassel <cassel@kernel.org>]

On Thu, May 14, 2026 at 10:05:29AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Hardware exclusivity violation and race condition due to removed port-wide barrier.
> - [High] Cross-link command starvation due to per-link deferred_qc wakeups.
> --
> 
> commit ec58a121e5df098567314ab23ec810f87424e7ab
> Author: Niklas Cassel <cassel@kernel.org>
> 
> ata: libata-scsi: do not needlessly defer commands when using PMP with FBS
> 
> This patch moves deferred_qc from struct ata_port to struct ata_link.
> This allows mixing non-NCQ and NCQ commands on the same port across
> different links when using Port Multipliers that support FIS-Based Switching.
> 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 02f2d7416c839..bd09d48d718da 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> [ ... ]
> > @@ -1676,10 +1677,10 @@ void ata_scsi_deferred_qc_work(struct work_struct *work)
> >  	 * such case, we should not need any more deferring the qc, so warn if
> >  	 * qc_defer() says otherwise.
> >  	 */
> > -	qc = ap->deferred_qc;
> > +	qc = link->deferred_qc;
> >  	if (qc && !ata_port_eh_scheduled(ap)) {
> >  		WARN_ON_ONCE(ap->ops->qc_defer(qc));
> > -		ap->deferred_qc = NULL;
> > +		link->deferred_qc = NULL;
> >  		ata_qc_issue(qc);
> >  	}
> 
> Could this lead to a hardware exclusivity violation?
> 
> Before this patch, ap->deferred_qc acted as a port-wide barrier. Now that
> it's scoped to the link, it allows other links to issue commands while one
> link has a deferred command pending.
> 
> If Link A defers a non-NCQ command, its workqueue gets scheduled. In the
> window before the workqueue executes, Link B can receive and issue a command
> since linkB->deferred_qc is NULL.
> 
> When Link A's workqueue runs, it evaluates the WARN_ON_ONCE above. Because
> Link B is now active, qc_defer() would correctly flag a conflict and return
> non-zero. However, the WARN_ON_ONCE does not abort execution, so the code
> proceeds to unconditionally call ata_qc_issue(qc). This would issue Link A's
> non-NCQ command while Link B's command is already active.

Thank you Sashiko, but being able to issue commands to multiple links at
the same time is the whole point of this series.

With PMP FBS, it is allowed to mix NCQ and non-NCQ to *different drives*.

Mixing NCQ and non-NCQ commands to the *the same drive* will still not
be allowed.

So this change in behavior is very much intended.


> 
> [ ... ]
> > @@ -1759,11 +1765,12 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  
> >  	ata_scsi_qc_done(qc, false, 0);
> >  
> > -	ata_scsi_schedule_deferred_qc(ap);
> > +	ata_scsi_schedule_deferred_qc(link);
> >  }
> 
> Can this cause cross-link command starvation for deferred commands?
> 
> In ata_scsi_qc_issue(), if a CBS PMP link is blocked because another link
> holds exclusive port access, it explicitly stores the command in
> linkA->deferred_qc. When the active link (Link B) completes its commands,
> ata_scsi_qc_complete() now only evaluates the completing link (Link B).
> 
> Since Link A's deferred_qc is completely ignored during Link B's completion,
> and Link A has no active commands to trigger its own completion, will its
> deferred command be permanently stranded, eventually causing a SCSI timeout?

This review comment is correct, but this was because of a mistake in
V4 which accidentally did "goto defer_qc" instead of "goto free_qc".

This is fixed in V5. In V5 PMP CBS will never make use of the deferred QC
issuing via workqueue feature, so this review command can be disregarded.


Kind regards,
Niklas