[PATCHv2] ata: sata_dwc_460ex: preserve sactive_issued state across ISR invocations

[PATCHv2] ata: sata_dwc_460ex: preserve sactive_issued state across ISR invocations

[Rosen Penev]

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
via the formula tag_mask = (sactive_issued | sactive) ^ sactive.

With the zeroing in place, sactive_issued is always cleared before a
DMAT interrupt can read it, so the NCQ completion path never identifies
completed tags correctly.  The command completion then falls back to
the non-NCQ path using ap->link.active_tag, which works for a single
outstanding command but produces wrong results when multiple NCQ tags
are in flight.

Remove the spurious zeroing and fix the NCQ/non-NCQ discrimination:
when tag_mask is zero but the active command is NCQ, all tracked tags
are still in SCR_ACTIVE and no completion processing is needed.

Fixes: 2d20da00c324b ("ata: sata_dwc_460ex: get rid of global data")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: resend as standalone patch
 drivers/ata/sata_dwc_460ex.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8a1d80ac906a..6cce790516cc 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -465,7 +465,6 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 	int handled, port = 0;
 	uint intpr, sactive, sactive2, tag_mask;
 	struct sata_dwc_device_port *hsdevp;
-	hsdev->sactive_issued = 0;
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -517,8 +516,11 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 	sata_dwc_scr_read(&ap->link, SCR_ACTIVE, &sactive);
 	tag_mask = (hsdev->sactive_issued | sactive) ^ sactive;
 
-	/* If no sactive issued and tag_mask is zero then this is not NCQ */
-	if (hsdev->sactive_issued == 0 && tag_mask == 0) {
+	/*
+	 * If tag_mask is zero and the active command is not NCQ this is a
+	 * non-NCQ completion.
+	 */
+	if (tag_mask == 0) {
 		if (ap->link.active_tag == ATA_TAG_POISON)
 			tag = 0;
 		else
@@ -534,6 +536,12 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 			handled = 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 = 1;
+			goto DONE;
+		}
 		status = ap->ops->sff_check_status(ap);
 
 		qc->ap->link.active_tag = tag;
@@ -612,9 +620,9 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 		tag_mask &= ~(1U << tag);
 		qc = ata_qc_from_tag(ap, tag);
 		if (unlikely(!qc)) {
-			dev_err(ap->dev, "failed to get qc");
-			handled = 1;
-			goto DONE;
+			dev_err(ap->dev, "stale tag %d in NCQ completion",
+				tag);
+			continue;
 		}
 
 		/* To be picked up by completion functions */
@@ -1051,6 +1059,8 @@ static int sata_dwc_hardreset(struct ata_link *link, unsigned int *class,
 
 	ret = sata_sff_hardreset(link, class, deadline);
 
+	hsdev->sactive_issued = 0;
+
 	sata_dwc_enable_interrupts(hsdev);
 
 	/* Reconfigure the DMA control register */
-- 
2.55.0

Re: [PATCHv2] ata: sata_dwc_460ex: preserve sactive_issued state across ISR invocations

[sashiko-bot]

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

New issues:
- [Critical] Preserving `sactive_issued` across ISR invocations causes repeated re-processing of completed DEV phases, prematurely completing NCQ commands.
- [High] NCQ DMAC interrupts are dropped when arriving before DEV interrupts, causing hangs.
- [High] Missing cleanup of `sactive_issued` for stale tags leads to infinite log spam and permanently breaks subsequent non-NCQ commands.

Pre-existing issues:
- [High] Out-of-bounds heap read when `ap->link.active_tag` is `ATA_TAG_POISON`.
--

commit 7a3914f790a5ca1a682aa2a68e7936ab121bdb65
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: preserve sactive_issued state across ISR invocations

This patch stops zeroing hsdev->sactive_issued on every ISR entry to 
preserve NCQ tag tracking across interrupts. It also updates the NCQ/non-NCQ 
discrimination logic to prevent completion processing when tag_mask is zero 
but the active command is NCQ.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8e3fc713891af..a8eec3dc7eb17 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
[ ... ]
> @@ -517,8 +516,11 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
>  	sata_dwc_scr_read(&ap->link, SCR_ACTIVE, &sactive);
>  	tag_mask = (hsdev->sactive_issued | sactive) ^ sactive;
>  
> -	/* If no sactive issued and tag_mask is zero then this is not NCQ */
> -	if (hsdev->sactive_issued == 0 && tag_mask == 0) {
> +	/*
> +	 * If tag_mask is zero and the active command is not NCQ this is a
> +	 * non-NCQ completion.
> +	 */
> +	if (tag_mask == 0) {
>  		if (ap->link.active_tag == ATA_TAG_POISON)
>  			tag = 0;
>  		else

[Severity: High]
This is a pre-existing issue, but since you are checking for ATA_TAG_POISON
here, I noticed that sata_dwc_error_intr() doesn't seem to do this.

When an error interrupt fires while no command is active, active_tag is
ATA_TAG_POISON (253). Does this out-of-bounds heap read leak memory into the
kernel log?

drivers/ata/sata_dwc_460ex.c:sata_dwc_error_intr() {
    ...
    tag = ap->link.active_tag;

    dev_err(ap->dev,
        "... pending=%d issued=%d",
        ..., hsdevp->dma_pending[tag], hsdevp->cmd_issued[tag]);
    ...
}

> @@ -534,6 +536,12 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
>  			handled = 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 = 1;
> +			goto DONE;
> +		}

[Severity: High]
Does this early exit path drop NCQ DMAC interrupts when they arrive before DEV
interrupts?

In SATA DMA transactions, the DMAC (transfer complete) interrupt can arrive
before the device sends the SDB FIS (DEV interrupt). When this happens,
sactive still matches sactive_issued, so tag_mask is 0.

By exiting early here, it appears dma_interrupt_count will not be incremented
for the DMAC interrupt. When the DEV interrupt later arrives, the count will
only reach 1, potentially causing the NCQ command to permanently hang.

[ ... ]
> @@ -612,9 +620,9 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
>  		tag_mask &= ~(1U << tag);
>  		qc = ata_qc_from_tag(ap, tag);
>  		if (unlikely(!qc)) {
> -			dev_err(ap->dev, "failed to get qc");
> -			handled = 1;
> -			goto DONE;
> +			dev_err(ap->dev, "stale tag %d in NCQ completion",
> +				tag);
> +			continue;
>  		}

[Severity: High]
Because sactive_issued is persistent across ISR invocations now, does skipping
cleanup here permanently break the port?

If continue is taken (e.g., from a spurious hardware interrupt with an invalid
tag), the stale tag is never cleared from hsdev->sactive_issued. This seems
to cause tag_mask to evaluate to non-zero on all subsequent interrupts,
permanently breaking the non-NCQ early fallback path (which expects tag_mask
== 0) and potentially causing endless log spam.

[Severity: Critical]
Since sactive_issued is no longer cleared on ISR entry, does this loop need
to clear the completed tags from hsdev->sactive_issued?

Because pending tags are not cleared from sactive_issued until they fully
complete, they appear to be repeatedly processed on subsequent interrupts. If
multiple NCQ tags are active, a DEV interrupt for Tag A increments the global
dma_interrupt_count to 1 but does not clear sactive_issued.

If a DEV interrupt for Tag B arrives next, tag_mask still contains Tag A. The
loop processes Tag A again, increments the count to 2, and erroneously
completes Tag A before its DMAC interrupt arrives:

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) == 0)
            sata_dwc_dma_xfer_complete(ap);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908214536.598823-1-rosenp@gmail.com?part=1

Re: [PATCHv2] ata: sata_dwc_460ex: preserve sactive_issued state across ISR invocations

[Niklas Cassel]

Hello Rosen,

On Tue, Sep 08, 2026 at 02:45:36PM -0700, Rosen Penev wrote:
> 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
> via the formula tag_mask = (sactive_issued | sactive) ^ sactive.
> 
> With the zeroing in place, sactive_issued is always cleared before a
> DMAT interrupt can read it, so the NCQ completion path never identifies
> completed tags correctly.  The command completion then falls back to
> the non-NCQ path using ap->link.active_tag, which works for a single
> outstanding command but produces wrong results when multiple NCQ tags
> are in flight.
> 
> Remove the spurious zeroing and fix the NCQ/non-NCQ discrimination:
> when tag_mask is zero but the active command is NCQ, all tracked tags
> are still in SCR_ACTIVE and no completion processing is needed.
> 
> Fixes: 2d20da00c324b ("ata: sata_dwc_460ex: get rid of global data")
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

With all due respect, you've already said that you don't have any hardware
that uses the sata_dwc_460ex driver.

So you have not encountered a problem in the real world, and you have not
been able to test that your LLM-assisted patch solves that real world
problem.

The risk of accepting LLM-assisted patches for problems that no one has
been able to reproduce on real hardware, is that the fix itself might
introduce another problem, and then we just trade one problem for another.

AFAICT, this seems to have been the problem at least twice so far:
https://lore.kernel.org/linux-ide/anw-k62wK-GhkHSI@monoceros/
https://lore.kernel.org/linux-ide/20260908141538.731237-1-nik.borisov@suse.com/

It was years since I saw anyone with a Synopsys email contribute to their
PCIe controller driver, or their Ethernet controller drivers.

sata_dwc_460ex is an older DWC (Synopsys) SATA controller, newer DWC SATA
controllers are AHCI compliant and is using drivers/ata/ahci_dwc.c.

So that probably leaves us with the company who submitted this driver.
They don't seem to have sent anything since 2011, so I doubt that they
care about this driver either.

That said, we are happy to accept any patch for any driver for which you
actually have the hardware to test.


Kind regards,
Niklas