[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

[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

[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