[PATCH 0/3] ata: sata_dwc_460ex: fix PHY lifecycle and sactive_issued races

[PATCH 0/3] ata: sata_dwc_460ex: fix PHY lifecycle and sactive_issued races

[Rosen Penev]

This series addresses two related issues in the sata_dwc_460ex driver:

1. PHY lifecycle ordering on device removal: The PHY was being powered off
   before the link was properly torn down, causing issues during device
   removal. The fix ensures proper ordering by tearing down the link first
   before powering off the PHY.

2. Data race on hsdev->sactive_issued in interrupt handler: A data race
   was identified on the sactive_issued field accessed from both the
   interrupt handler and the task context. This is fixed by using atomic
   operations and preserving the state across ISR invocations.

The third patch preserves the sactive_issued state across interrupt handler
invocations to prevent losing track of issued commands during interrupt
processing.

Rosen Penev (3):
  ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal
  ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in
    interrupt handler
  ata: sata_dwc_460ex: preserve sactive_issued state across ISR
    invocations

 drivers/ata/sata_dwc_460ex.c | 46 +++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 16 deletions(-)

---
 2.55.0

[PATCH 1/3] ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

[Rosen Penev]

sata_dwc_remove() calls phy_exit() while phy_power_off() is still
pending in sata_dwc_port_stop(), which runs later during device
teardown.  This violates the expected PHY sequencing of power_off
before exit.

Fixes: 0f48debdb906 ("ata: sata_dwc_460ex: add phy support")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8e3fc713891a..8a1d80ac906a 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -851,10 +851,14 @@ static int sata_dwc_port_start(struct ata_port *ap)
 	if (err)
 		goto CLEANUP_ALLOC;
 
-	err = phy_power_on(hsdev->phy);
+	err = phy_init(hsdev->phy);
 	if (err)
 		goto CLEANUP_ALLOC;
 
+	err = phy_power_on(hsdev->phy);
+	if (err)
+		goto CLEANUP_PHY;
+
 	for (i = 0; i < SATA_DWC_QCMD_MAX; i++)
 		hsdevp->cmd_issued[i] = SATA_DWC_CMD_ISSUED_NOT;
 
@@ -880,6 +884,8 @@ static int sata_dwc_port_start(struct ata_port *ap)
 	dev_dbg(ap->dev, "%s: done\n", __func__);
 	return 0;
 
+CLEANUP_PHY:
+	phy_exit(hsdev->phy);
 CLEANUP_ALLOC:
 	kfree(hsdevp);
 CLEANUP:
@@ -897,6 +903,7 @@ static void sata_dwc_port_stop(struct ata_port *ap)
 	dmaengine_terminate_sync(hsdevp->chan);
 	dma_release_channel(hsdevp->chan);
 	phy_power_off(hsdev->phy);
+	phy_exit(hsdev->phy);
 
 	kfree(hsdevp);
 	ap->private_data = NULL;
@@ -1163,6 +1170,10 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	if (irq < 0)
 		return irq;
 
+	hsdev->phy = devm_phy_optional_get(dev, "sata-phy");
+	if (IS_ERR(hsdev->phy))
+		return PTR_ERR(hsdev->phy);
+
 #ifdef CONFIG_SATA_DWC_OLD_DMA
 	if (!of_property_present(dev->of_node, "dmas")) {
 		err = sata_dwc_dma_init_old(ofdev, hsdev);
@@ -1171,29 +1182,26 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	}
 #endif
 
-	hsdev->phy = devm_phy_optional_get(dev, "sata-phy");
-	if (IS_ERR(hsdev->phy))
-		return PTR_ERR(hsdev->phy);
-
-	err = phy_init(hsdev->phy);
-	if (err)
-		goto error_out;
-
 	/*
 	 * Now, register with libATA core, this will also initiate the
 	 * device discovery process, invoking our port_start() handler &
 	 * error_handler() to execute a dummy Softreset EH session
 	 */
 	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
-	if (err)
+	if (err) {
 		dev_err(dev, "failed to activate host");
+		goto error_out;
+	}
 
 	/* Enable SATA Interrupts */
 	sata_dwc_enable_interrupts(hsdev);
 	return 0;
 
 error_out:
-	phy_exit(hsdev->phy);
+#ifdef CONFIG_SATA_DWC_OLD_DMA
+	if (!device_property_present(dev, "dmas"))
+		sata_dwc_dma_exit_old(hsdev);
+#endif
 	return err;
 }
 
@@ -1205,8 +1213,6 @@ static void sata_dwc_remove(struct platform_device *ofdev)
 
 	ata_host_detach(host);
 
-	phy_exit(hsdev->phy);
-
 #ifdef CONFIG_SATA_DWC_OLD_DMA
 	/* Free SATA DMA resources */
 	sata_dwc_dma_exit_old(hsdev);
-- 
2.55.0

Re: [PATCH 1/3] ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

[sashiko-bot]

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

Pre-existing issues:
- [High] Inverted teardown sequence destroys the legacy DMA controller before its channels are released, causing a use-after-free and crash during device removal and probe error handling.
--

--- Patch [1]: [PATCH 1/3] ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal ---
commit dd1760ffad9026f965244b679ddbacf4ae16f4e6
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

This commit fixes PHY teardown ordering by calling phy_exit() after
phy_power_off() instead of before, ensuring the PHY is properly powered
down prior to destruction. It also adjusts the error handling in
sata_dwc_probe() to clean up legacy DMA resources if activation fails.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8e3fc713891af..8a1d80ac906a7 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c

[ ... ]

> @@ -1171,29 +1182,26 @@ static int sata_dwc_probe(struct platform_device *ofdev)

[ ... ]

>  error_out:
> -	phy_exit(hsdev->phy);
> +#ifdef CONFIG_SATA_DWC_OLD_DMA
> +	if (!device_property_present(dev, "dmas"))
> +		sata_dwc_dma_exit_old(hsdev);

[Severity: High]
This is a pre-existing issue, but does this manual teardown sequence create a
use-after-free for the legacy DMA controller?

By calling sata_dwc_dma_exit_old() here in the probe error path (and
similarly in sata_dwc_remove()), the legacy DMA controller is explicitly
destroyed before the devres unwind actions run.

However, when devres later triggers the ata_host_stop action, it calls
sata_dwc_port_stop(), which does this:

    dmaengine_terminate_sync(hsdevp->chan);
    dma_release_channel(hsdevp->chan);

Since the legacy DMA controller has already been deactivated and removed by
sata_dwc_dma_exit_old(), won't these channel operations in
sata_dwc_port_stop() access the destroyed controller and cause a crash?

> +#endif
>  	return err;
>  }

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

[PATCH 2/3] ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

[Rosen Penev]

hsdev->sactive_issued is written locklessly in sata_dwc_isr() before
acquiring host->lock, while sata_dwc_qc_complete() performs a
read-modify-write on the same field under the lock.  This creates a
data race that can corrupt NCQ tag tracking state.

Move the zero assignment inside the critical section so all accesses
to sactive_issued are serialized by host->lock.

Fixes: 2d20da00c324b ("ata: sata_dwc_460ex: get rid of global data")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8a1d80ac906a..73bacdfd0bd3 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -465,9 +465,9 @@ 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);
+	hsdev->sactive_issued = 0;
 
 	/* Read the interrupt register */
 	intpr = sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);
-- 
2.55.0

Re: [PATCH 2/3] ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

[sashiko-bot]

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

Pre-existing issues:
- [High] Unconditionally clearing hsdev->sactive_issued to 0 on every ISR entry breaks NCQ completion tracking, which the patch fails to address.
- [High] The driver unconditionally reads ap->link.active_tag without checking for ATA_TAG_POISON, causing out-of-bounds array accesses in error and DMA paths.
--

--- Patch [2]: [PATCH 2/3] ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler ---
commit 2519582df93a7a74dcbe39c71c593d6dad63fb71
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

This commit moves the zeroing of hsdev->sactive_issued inside the host
spinlock critical section in sata_dwc_isr() to prevent a data race with
sata_dwc_qc_complete().

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8a1d80ac906a7..73bacdfd0bd31 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -465,9 +465,9 @@ 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);
> +	hsdev->sactive_issued = 0;

[Severity: High]
This is a pre-existing issue, but does moving this assignment actually
resolve the underlying problem? Unconditionally clearing
hsdev->sactive_issued to 0 on every ISR entry seems to break NCQ
completion tracking.

When an NCQ completion interrupt fires, sactive_issued will be 0. The
driver then calculates tag_mask = (0 | sactive) ^ sactive, which always
evaluates to 0 because sactive clears the bit for completed commands.

With tag_mask == 0, will the driver skip the NCQ completion block and
leave the command to time out?

>  
>  	/* Read the interrupt register */
>  	intpr = sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);


[Severity: High]
This is also a pre-existing issue, but I noticed another potential problem
in sata_dwc_error_intr() and the DMA completion paths.

Does this code potentially perform out-of-bounds array accesses?

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

        dev_err(ap->dev,
                "%s SCR_ERROR=0x%08x intpr=0x%08x status=0x%08x dma_intp=%d pending=%d issued=%d",
                __func__, serror, intpr, status, hsdevp->dma_interrupt_count,
                hsdevp->dma_pending[tag], hsdevp->cmd_issued[tag]);
        ...
}

When an error interrupt occurs while no command is active,
ap->link.active_tag is set to ATA_TAG_POISON (0xfafbfcfdU).

The driver truncates this to a u8 (253) and uses it directly as an index
into the hsdevp->dma_pending and hsdevp->cmd_issued arrays, which are
sized SATA_DWC_QCMD_MAX (33).

Could this result in leaking arbitrary memory up to ~1000 bytes past the
arrays into dmesg via dev_err()?

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

[PATCH 3/3] 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>
---
 drivers/ata/sata_dwc_460ex.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 73bacdfd0bd3..025b5d968c78 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_instance)
 	struct sata_dwc_device_port *hsdevp;
 
 	spin_lock_irqsave(&host->lock, flags);
-	hsdev->sactive_issued = 0;
 
 	/* Read the interrupt register */
 	intpr = sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);
@@ -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;
-- 
2.55.0

Re: [PATCH 3/3] 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] Stale tags in `hsdev->sactive_issued` cause a permanent port lockup after any command timeout.
- [High] Infinite re-processing of partially completed NCQ tags corrupts DMA interrupt tracking.
- [High] Time-Of-Check to Time-Of-Use race drops NCQ completions, causing command 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 state across ISR invocations ---
commit 2d2c8f6bc7683fcd41033d9263799c80943fc1d0
Author: Rosen Penev <rosenp@gmail.com>

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_instance)
>  	struct sata_dwc_device_port *hsdevp;
>  
>  	spin_lock_irqsave(&host->lock, flags);
> -	hsdev->sactive_issued = 0;

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

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 would
process the stale tag:

drivers/ata/sata_dwc_460ex.c:sata_dwc_isr() {
    ...
    while (tag_mask) {
        tag = __ffs(tag_mask);
        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;
        }
    ...
}

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

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

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