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

[sashiko-bot@kernel.org]

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.

[ ... ]
> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513151359.1075403-6-cassel@kernel.org?part=4