[PATCHv2 0/8] ata: sata_dwc_460ex: cleanups and interrupt ordering fix

[PATCHv2 0/8] ata: sata_dwc_460ex: cleanups and interrupt ordering fix

[Rosen Penev]

This series modernizes the sata_dwc_460ex driver by replacing legacy OF
APIs with their modern device-property and platform-device counterparts,
converting old DMA resource teardown to devm-managed cleanup, fixing a
latent interrupt-ordering bug, and dropping a redundant stack copy of
port_info.

Patch 4 fixes a real issue: sata_dwc_enable_interrupts() was called
before the IRQ handler was registered, so a probe failure or an
asserted interrupt could trigger an unhandled irq storm.

v2: sashiko fixes.

Rosen Penev (8):
  ata: sata_dwc_460ex: use device_property_present()
  ata: sata_dwc_460ex: use platform_get_irq()
  ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is
    registered
  ata: sata_dwc_460ex: drop redundant struct copy of port_info
  ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in
    interrupt handler
  ata: sata_dwc_460ex: disable SATA interrupts on device removal
  ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal
  ata: sata_dwc_460ex: use devm for old DMA resource lifetime management

 drivers/ata/sata_dwc_460ex.c | 99 ++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 50 deletions(-)

--
2.55.0

[PATCHv2 1/8] ata: sata_dwc_460ex: use device_property_present()

[Rosen Penev]

There's no need to use np here when dev is enough. Follows the similar
pattern elsewhere where of APIs are replaced with device ones,
especially when using platform_device.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 4fc22ce4bd9a..d4c554f0979d 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -21,6 +21,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/phy/phy.h>
 #include <linux/libata.h>
 #include <linux/slab.h>
@@ -809,7 +810,7 @@ static int sata_dwc_dma_get_channel(struct sata_dwc_device_port *hsdevp)
 	struct device *dev = hsdev->dev;
 
 #ifdef CONFIG_SATA_DWC_OLD_DMA
-	if (!of_property_present(dev->of_node, "dmas"))
+	if (!device_property_present(dev, "dmas"))
 		return sata_dwc_dma_get_channel_old(hsdevp);
 #endif
 
@@ -1180,7 +1181,7 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	}
 
 #ifdef CONFIG_SATA_DWC_OLD_DMA
-	if (!of_property_present(np, "dmas")) {
+	if (!device_property_present(dev, "dmas")) {
 		err = sata_dwc_dma_init_old(ofdev, hsdev);
 		if (err)
 			return err;
-- 
2.55.0

[PATCHv2 2/8] ata: sata_dwc_460ex: use platform_get_irq()

[Rosen Penev]

Replace irq_of_parse_and_map() with platform_get_irq() in both
sata_dwc_dma_init_old() and sata_dwc_probe(). This is the preferred
way to obtain IRQs for platform devices and provides better error
reporting.  Remove the now-unnecessary #include <linux/of_irq.h>.

irq_of_parse_and_map() requires irq_dispose_mapping(), which is missing.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index d4c554f0979d..1561adea323a 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -19,7 +19,6 @@
 #include <linux/device.h>
 #include <linux/dmaengine.h>
 #include <linux/of.h>
-#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/phy/phy.h>
@@ -227,7 +226,6 @@ static int sata_dwc_dma_init_old(struct platform_device *pdev,
 				 struct sata_dwc_device *hsdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
 
 	hsdev->dma = devm_kzalloc(dev, sizeof(*hsdev->dma), GFP_KERNEL);
 	if (!hsdev->dma)
@@ -237,11 +235,9 @@ static int sata_dwc_dma_init_old(struct platform_device *pdev,
 	hsdev->dma->id = pdev->id;
 
 	/* Get SATA DMA interrupt number */
-	hsdev->dma->irq = irq_of_parse_and_map(np, 1);
-	if (!hsdev->dma->irq) {
-		dev_err(dev, "no SATA DMA irq\n");
-		return -ENODEV;
-	}
+	hsdev->dma->irq = platform_get_irq(pdev, 1);
+	if (hsdev->dma->irq < 0)
+		return hsdev->dma->irq;
 
 	/* Get physical SATA DMA register base address */
 	hsdev->dma->regs = devm_platform_ioremap_resource(pdev, 1);
@@ -1127,7 +1123,6 @@ static const struct ata_port_info sata_dwc_port_info[] = {
 static int sata_dwc_probe(struct platform_device *ofdev)
 {
 	struct device *dev = &ofdev->dev;
-	struct device_node *np = dev->of_node;
 	struct sata_dwc_device *hsdev;
 	u32 idr, versionr;
 	char *ver = (char *)&versionr;
@@ -1174,11 +1169,9 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	sata_dwc_enable_interrupts(hsdev);
 
 	/* Get SATA interrupt number */
-	irq = irq_of_parse_and_map(np, 0);
-	if (!irq) {
-		dev_err(dev, "no SATA DMA irq\n");
-		return -ENODEV;
-	}
+	irq = platform_get_irq(ofdev, 0);
+	if (irq < 0)
+		return irq;
 
 #ifdef CONFIG_SATA_DWC_OLD_DMA
 	if (!device_property_present(dev, "dmas")) {
-- 
2.55.0

[PATCHv2 3/8] ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered

[Rosen Penev]

sata_dwc_enable_interrupts() is called before platform_get_irq() and
ata_host_activate(), leaving the SATA controller's interrupt mask
enabled without a registered handler.  If a later step fails (irq
request, phy init, etc.) or if the controller asserts an interrupt
during probe, the irq line may fire with no handler, causing a
spurious interrupt storm.

Move sata_dwc_enable_interrupts() after ata_host_activate() so that
interrupts are only unmasked once the handler is registered and the
core is fully initialized.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 1561adea323a..43011789c75e 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1165,9 +1165,6 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	/* Save dev for later use in dev_xxx() routines */
 	hsdev->dev = dev;
 
-	/* Enable SATA Interrupts */
-	sata_dwc_enable_interrupts(hsdev);
-
 	/* Get SATA interrupt number */
 	irq = platform_get_irq(ofdev, 0);
 	if (irq < 0)
@@ -1198,6 +1195,8 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	if (err)
 		dev_err(dev, "failed to activate host");
 
+	/* Enable SATA Interrupts */
+	sata_dwc_enable_interrupts(hsdev);
 	return 0;
 
 error_out:
-- 
2.55.0

[PATCHv2 4/8] ata: sata_dwc_460ex: drop redundant struct copy of port_info

[Rosen Penev]

sata_dwc_port_info[0] was copied to a local auto variable before being
referenced via a pointer array.  The local copy is never modified, so
drop it and point ppi directly at the static data.  This saves ~104
bytes of stack and makes the const qualification consistent.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 43011789c75e..2a28e6655bba 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1130,8 +1130,8 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	int err = 0;
 	int irq;
 	struct ata_host *host;
-	struct ata_port_info pi = sata_dwc_port_info[0];
-	const struct ata_port_info *ppi[] = { &pi, NULL };
+	const struct ata_port_info *pi = &sata_dwc_port_info[0];
+	const struct ata_port_info *ppi[] = { pi, NULL };
 	struct resource *res;
 
 	/* Allocate DWC SATA device */
-- 
2.55.0

[PATCHv2 5/8] 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.

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 2a28e6655bba..4c54c3ecd18e 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -467,9 +467,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

[PATCHv2 6/8] ata: sata_dwc_460ex: disable SATA interrupts on device removal

[Rosen Penev]

sata_dwc_remove() fails to mask the controller's INTMR and ERRMR
registers before tearing down the host.  If the hardware asserts an
interrupt during teardown (after ata_host_detach() but before the
IRQ handler is unregistered by devres), this can trigger an unhandled
interrupt storm and potentially lock up the system.

Add sata_dwc_disable_interrupts() and call it at the beginning of
sata_dwc_remove(), before ata_host_detach().

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 4c54c3ecd18e..f26220cac130 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -759,6 +759,12 @@ static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
 	return 0;
 }
 
+static void sata_dwc_disable_interrupts(struct sata_dwc_device *hsdev)
+{
+	sata_dwc_writel(&hsdev->sata_dwc_regs->intmr, 0);
+	sata_dwc_writel(&hsdev->sata_dwc_regs->errmr, 0);
+}
+
 static void sata_dwc_enable_interrupts(struct sata_dwc_device *hsdev)
 {
 	/* Enable selective interrupts by setting the interrupt maskregister*/
@@ -1210,6 +1216,8 @@ static void sata_dwc_remove(struct platform_device *ofdev)
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct sata_dwc_device *hsdev = host->private_data;
 
+	sata_dwc_disable_interrupts(hsdev);
+
 	ata_host_detach(host);
 
 	phy_exit(hsdev->phy);
-- 
2.55.0

[PATCHv2 7/8] 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.

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 f26220cac130..e8e2790be1d6 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -864,10 +864,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;
 
@@ -893,6 +897,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:
@@ -910,6 +916,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;
@@ -1176,6 +1183,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 (!device_property_present(dev, "dmas")) {
 		err = sata_dwc_dma_init_old(ofdev, hsdev);
@@ -1184,29 +1195,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;
 }
 
@@ -1220,8 +1228,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

[PATCHv2 8/8] ata: sata_dwc_460ex: use devm for old DMA resource lifetime management

[Rosen Penev]

Convert sata_dwc_dma_exit_old() to a devm action callback and register
it via devm_add_action_or_reset() after dw_dma_probe() succeeds.  This
lets the devm framework handle DMA controller teardown automatically on
probe failure and driver removal.

As a result the probe function can use plain return-on-error for all
failure paths - the dma_initialized flag, the goto labels, and the
explicit sata_dwc_dma_exit_old() call in sata_dwc_remove() are all
eliminated.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index e8e2790be1d6..352cd7f02163 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -222,10 +222,18 @@ static int sata_dwc_dma_get_channel_old(struct sata_dwc_device_port *hsdevp)
 	return 0;
 }
 
+static void sata_dwc_dma_exit_old(void *data)
+{
+	struct sata_dwc_device *hsdev = data;
+
+	dw_dma_remove(hsdev->dma);
+}
+
 static int sata_dwc_dma_init_old(struct platform_device *pdev,
 				 struct sata_dwc_device *hsdev)
 {
 	struct device *dev = &pdev->dev;
+	int err;
 
 	hsdev->dma = devm_kzalloc(dev, sizeof(*hsdev->dma), GFP_KERNEL);
 	if (!hsdev->dma)
@@ -245,15 +253,11 @@ static int sata_dwc_dma_init_old(struct platform_device *pdev,
 		return PTR_ERR(hsdev->dma->regs);
 
 	/* Initialize AHB DMAC */
-	return dw_dma_probe(hsdev->dma);
-}
-
-static void sata_dwc_dma_exit_old(struct sata_dwc_device *hsdev)
-{
-	if (!hsdev->dma)
-		return;
+	err = dw_dma_probe(hsdev->dma);
+	if (err)
+		return err;
 
-	dw_dma_remove(hsdev->dma);
+	return devm_add_action_or_reset(dev, sata_dwc_dma_exit_old, hsdev);
 }
 
 #endif
@@ -1203,19 +1207,12 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
 	if (err) {
 		dev_err(dev, "failed to activate host");
-		goto error_out;
+		return err;
 	}
 
 	/* Enable SATA Interrupts */
 	sata_dwc_enable_interrupts(hsdev);
 	return 0;
-
-error_out:
-#ifdef CONFIG_SATA_DWC_OLD_DMA
-	if (!device_property_present(dev, "dmas"))
-		sata_dwc_dma_exit_old(hsdev);
-#endif
-	return err;
 }
 
 static void sata_dwc_remove(struct platform_device *ofdev)
@@ -1228,11 +1225,6 @@ static void sata_dwc_remove(struct platform_device *ofdev)
 
 	ata_host_detach(host);
 
-#ifdef CONFIG_SATA_DWC_OLD_DMA
-	/* Free SATA DMA resources */
-	sata_dwc_dma_exit_old(hsdev);
-#endif
-
 	dev_dbg(dev, "done\n");
 }
 
-- 
2.55.0

Re: [PATCHv2 0/8] ata: sata_dwc_460ex: cleanups and interrupt ordering fix

[Damien Le Moal]

On 7/2/26 09:03, Rosen Penev wrote:
> This series modernizes the sata_dwc_460ex driver by replacing legacy OF
> APIs with their modern device-property and platform-device counterparts,
> converting old DMA resource teardown to devm-managed cleanup, fixing a
> latent interrupt-ordering bug, and dropping a redundant stack copy of
> port_info.
> 
> Patch 4 fixes a real issue: sata_dwc_enable_interrupts() was called
> before the IRQ handler was registered, so a probe failure or an
> asserted interrupt could trigger an unhandled irq storm.
> 
> v2: sashiko fixes.

Several patches in this series are clearly bug fixes that should come first in
the series and have a fixes tag and cc-stable. Can you please reorganize the
series with the fixes first ?

Also please try to avoid growing the patch series with more patches in response
to sachiko "not introduced by this patch" comments on valid problems. Send
incremental patches instead. Otherwise, making progress will take a longer time,
and bigger series are harder to review.

> 
> Rosen Penev (8):
>   ata: sata_dwc_460ex: use device_property_present()
>   ata: sata_dwc_460ex: use platform_get_irq()
>   ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is
>     registered
>   ata: sata_dwc_460ex: drop redundant struct copy of port_info
>   ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in
>     interrupt handler
>   ata: sata_dwc_460ex: disable SATA interrupts on device removal
>   ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal
>   ata: sata_dwc_460ex: use devm for old DMA resource lifetime management
> 
>  drivers/ata/sata_dwc_460ex.c | 99 ++++++++++++++++++------------------
>  1 file changed, 49 insertions(+), 50 deletions(-)
> 
> --
> 2.55.0
> 


-- 
Damien Le Moal
Western Digital Research