Re: [PATCH 3/3] ata: sata_dwc_460ex: preserve sactive_issued state across ISR invocations

Thank you for your contribution! Sashiko AI review found 4 potential issue(=
s) to consider:

New issues:
- [Critical] Stale tags in `hsdev->sactive_issued` cause a permanent port l=
ockup after any command timeout.
- [High] Infinite re-processing of partially completed NCQ tags corrupts DM=
A interrupt tracking.
- [High] Time-Of-Check to Time-Of-Use race drops NCQ completions, causing c=
ommand timeouts.

Pre-existing issues:
- [Critical] Out-of-bounds array access in DMA callback when using multiple=
 NCQ tags.
--

--- Patch [3]: [PATCH 3/3] ata: sata_dwc_460ex: preserve sactive_issued sta=
te across ISR invocations ---
commit 2d2c8f6bc7683fcd41033d9263799c80943fc1d0
Author: Rosen Penev <[email protected]>

ata: sata_dwc_460ex: preserve sactive_issued state across ISR invocations

Zeroing hsdev->sactive_issued on every ISR entry destroys the NCQ tag
tracking that must persist across interrupts. This field is populated
in the NEWFP (DMA Setup FIS) handler and used in subsequent DMAT (DMA
Transfer Complete) interrupts to determine which tags have completed.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 73bacdfd0bd31..025b5d968c780 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -467,7 +467,6 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_in=
stance)
>  	struct sata_dwc_device_port *hsdevp;
> =20
>  	spin_lock_irqsave(&host->lock, flags);
> -	hsdev->sactive_issued =3D 0;

[Severity: Critical]
Since sactive_issued is no longer cleared here, does it accumulate stale ta=
gs?

If a tag completes and is never cleared from hsdev->sactive_issued, it could
trigger a permanent port lockup after any command timeout. The ISR loop wou=
ld
process the stale tag:

drivers/ata/sata_dwc_460ex.c:sata_dwc_isr() {
    ...
    while (tag_mask) {
        tag =3D __ffs(tag_mask);
        tag_mask &=3D ~(1U << tag);
        qc =3D ata_qc_from_tag(ap, tag);
        if (unlikely(!qc)) {
            dev_err(ap->dev, "failed to get qc");
            handled =3D 1;
            goto DONE;
        }
    ...
}

Will encountering a stale tag cause ata_qc_from_tag() to return NULL, hitti=
ng
this unlikely(!qc) path, and permanently drop all valid completions?

[ ... ]
> @@ -534,6 +536,12 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_i=
nstance)
>  			handled =3D 1;
>  			goto DONE;
>  		}
> +		if (ata_is_ncq(qc->tf.protocol)) {
> +			/* NCQ commands still in flight; no tag completed. */
> +			ap->ops->sff_check_status(ap);
> +			handled =3D 1;
> +			goto DONE;
> +		}

[Severity: High]
Does this create a Time-Of-Check to Time-Of-Use race?

By clearing the host interrupt with ap->ops->sff_check_status(ap) and
immediately jumping to DONE without re-evaluating SCR_ACTIVE, could this
silently drop NCQ completions that arrived just after the earlier SCR_ACTIVE
read, resulting in command timeouts?

[ ... ]

[Severity: High]
Does the ISR continually re-process partially completed NCQ tags?

drivers/ata/sata_dwc_460ex.c:sata_dwc_isr() {
    ...
    if (ata_is_dma(qc->tf.protocol)) {
        hsdevp->dma_interrupt_count++;
        ...
        if ((hsdevp->dma_interrupt_count % 2) =3D=3D 0)
            sata_dwc_dma_xfer_complete(ap);
    ...
}

If the DMA transfer isn't complete, it fails to clear the tag from the
tracking state. Will this asymmetric clearing routine force the ISR to
continually pop and double-increment the same tag on subsequent interrupts,
corrupting the DMA interrupt tracking?

[Severity: Critical]
This is a pre-existing issue, but does using the scalar ap->link.active_tag
in concurrent NCQ environments cause an out-of-bounds array access?

drivers/ata/sata_dwc_460ex.c:dma_dwc_xfer_done() {
    ...
    tag =3D ap->link.active_tag;
    ...
    sata_dwc_clear_dmacr(hsdevp, tag);
}

drivers/ata/sata_dwc_460ex.c:sata_dwc_clear_dmacr() {
    ...
    if (hsdevp->dma_pending[tag] =3D=3D SATA_DWC_DMA_PENDING_RX) {
    ...
}

When using multiple NCQ tags, ap->link.active_tag can be ATA_TAG_POISON
which has a value of 253. Will this evaluate an out-of-bounds index of 253
and execute an invalid memory read or write?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260723001208.1469=
[email protected]?part=3D3