[PATCH 0/5] ata: sata_dwc_460ex: cleanups and interrupt ordering fix

[PATCH 0/5] 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.

Rosen Penev (5):
  ata: sata_dwc_460ex: use device_property_present()
  ata: sata_dwc_460ex: use platform_get_irq()
  ata: sata_dwc_460ex: use devm for old DMA resource lifetime management
  ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is
    registered
  ata: sata_dwc_460ex: drop redundant struct copy of port_info

 drivers/ata/sata_dwc_460ex.c | 66 +++++++++++++++---------------------
 1 file changed, 28 insertions(+), 38 deletions(-)

-- 
2.55.0

[PATCH 1/5] 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 | 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 4fc22ce4bd9a..2bcb3ac5ae21 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1180,7 +1180,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

Re: [PATCH 1/5] ata: sata_dwc_460ex: use device_property_present()

[sashiko-bot]

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

New issues:
- [Low] Incomplete refactoring: missed identical `of_property_present` pattern.
- [Low] Direct usage of `device_property_present()` without including `<linux/property.h>`.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `clear_interrupt_bit()` ignores its `bit` argument and accidentally clears all pending interrupts.
--

commit 3c350702aea5f359b0516233c160076946c26124
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: use device_property_present()

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.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..2bcb3ac5ae211 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -1180,7 +1180,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")) {

[Severity: Low]
Does this require including linux/property.h? The build currently succeeds
due to transitive includes, but it might be safer to include the header
directly to prevent future build breakages if the include chain changes.

[Severity: Low]
Is the intention to replace all of_property_present usages? Looking at
sata_dwc_dma_get_channel() in the same file, it still uses the old function:

	if (!of_property_present(dev->of_node, "dmas"))

Should this be updated as well for consistency?

[Severity: High]
This is a pre-existing issue, but clear_interrupt_bit() seems to ignore its
bit argument and accidentally clears all pending interrupts:

static void clear_interrupt_bit(struct sata_dwc_device *hsdev, u32 bit)
{
	sata_dwc_writel(&hsdev->sata_dwc_regs->intpr,
			sata_dwc_readl(&hsdev->sata_dwc_regs->intpr));
}

Because it reads the entire intpr register and writes it back unmodified,
does this acknowledge and clear all pending interrupts in the write-1-to-clear
register instead of just the requested bit? This could drop device
interrupts when multiple interrupts are asserted simultaneously.

>  		err = sata_dwc_dma_init_old(ofdev, hsdev);
>  		if (err)
>  			return err;

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

[PATCH 2/5] 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 2bcb3ac5ae21..215b5a65527d 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/phy/phy.h>
 #include <linux/libata.h>
@@ -226,7 +225,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)
@@ -236,11 +234,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);
@@ -1126,7 +1122,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;
@@ -1173,11 +1168,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

[PATCH 3/5] 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 | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 215b5a65527d..34d6d0534e30 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -221,10 +221,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)
@@ -244,15 +252,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
@@ -1186,7 +1190,7 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 
 	err = phy_init(hsdev->phy);
 	if (err)
-		goto error_out;
+		return err;
 
 	/*
 	 * Now, register with libATA core, this will also initiate the
@@ -1194,14 +1198,13 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	 * 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");
+		phy_exit(hsdev->phy);
+		return err;
+	}
 
 	return 0;
-
-error_out:
-	phy_exit(hsdev->phy);
-	return err;
 }
 
 static void sata_dwc_remove(struct platform_device *ofdev)
@@ -1214,11 +1217,6 @@ static void sata_dwc_remove(struct platform_device *ofdev)
 
 	phy_exit(hsdev->phy);
 
-#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

[PATCH 4/5] 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 34d6d0534e30..fb6042b9ac27 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1168,9 +1168,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)
@@ -1204,6 +1201,8 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 		return err;
 	}
 
+	/* Enable SATA Interrupts */
+	sata_dwc_enable_interrupts(hsdev);
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH 4/5] ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing SATA interrupt disable in remove path leads to potential interrupt storm.
- [High] Lifecycle inversion: `phy_power_off()` is executed on an already exited PHY during teardown.
- [High] Data race on `hsdev->sactive_issued` causing state corruption.
--

commit 6c390f90accf9771f2e071160279ee0671a1ecfe
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered

This patch moves sata_dwc_enable_interrupts() after ata_host_activate()
in sata_dwc_probe(). This ensures that the controller's interrupt mask is
only enabled once the interrupt handler is successfully registered and the
core is fully initialized.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 34d6d0534e308..fb6042b9ac273 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -1168,9 +1168,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)
> @@ -1204,6 +1201,8 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  		return err;
>  	}
>  
> +	/* Enable SATA Interrupts */
> +	sata_dwc_enable_interrupts(hsdev);
>  	return 0;
>  }

While reviewing the interrupt handling changes in sata_dwc_probe(), I
noticed a few other pre-existing issues in the driver that might be worth
addressing.

[Severity: High]
This isn't a bug introduced by this patch, but looking at sata_dwc_remove(),
is there a missing SATA interrupt disable call?

drivers/ata/sata_dwc_460ex.c:sata_dwc_remove() {
        ...
        ata_host_detach(host);
        
        phy_exit(hsdev->phy);
        ...
}

The driver detaches the host and exits the PHY, but fails to mask the
controller's interrupts (intmr and errmr registers). When remove() returns,
the driver core cleans up devres allocations, which unregisters the IRQ
handler.

If the hardware asserts an interrupt during teardown, can this lead to an
unhandled interrupt storm and potentially lock up the system or disable a
shared IRQ line?

[Severity: High]
This is a pre-existing issue, but does sata_dwc_remove() have a PHY lifecycle
inversion?

drivers/ata/sata_dwc_460ex.c:sata_dwc_remove() {
        ...
        phy_exit(hsdev->phy);
        ...
}

The driver explicitly calls phy_exit() here. However, phy_power_off() is
handled inside sata_dwc_port_stop(), which is executed later by devres
cleanup:

drivers/ata/sata_dwc_460ex.c:sata_dwc_port_stop() {
        ...
        phy_power_off(hsdev->phy);
        ...
}

Because ata_host_stop() is registered as a devres callback (via
ata_host_start()), it runs after sata_dwc_remove() returns. Does this result
in phy_power_off() operating on a PHY that has already been exited?

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race on
hsdev->sactive_issued in sata_dwc_isr()?

drivers/ata/sata_dwc_460ex.c:sata_dwc_isr() {
        ...
        hsdev->sactive_issued = 0;
        
        spin_lock_irqsave(&host->lock, flags);
        ...
}

The assignment to hsdev->sactive_issued is executed locklessly before
acquiring host->lock. If a DMA completion interrupt fires concurrently on
another CPU, dma_dwc_xfer_done() acquires host->lock and calls
sata_dwc_qc_complete().

Since sata_dwc_qc_complete() performs a read-modify-write on this same
state under the lock, can this race corrupt the NCQ tag tracking state
and desynchronize the state machine?

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

[PATCH 5/5] 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 fb6042b9ac27..af00acf155d4 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1133,8 +1133,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