ata: sata_dwc_460ex: cleanups and interrupt ordering fix

PATCH Superseded Rosen Penev 3 participants 14 messages Jul 2 libata

Share & tools

Rosen Penev

author

2026-07-02 00:03 UTC #0 lore raw

v2 cover ata: sata_dwc_460ex: cleanups and interrupt ordering fix

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

Rosen Penev

author

2026-07-02 00:03 UTC #1 lore raw

PATCH v2 1/8 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.

Signed-off-by Rosen Penev <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/ata/sata_dwc_460ex.c +3 -2

index 4fc22ce4bd9a..d4c554f0979d 100644
@@ -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;

Signature · 1 line

2.55.0

Rosen Penev

author

2026-07-02 00:03 UTC #2 lore raw

PATCH v2 2/8 ata: sata_dwc_460ex: use platform_get_irq()

In reply to 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 <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

drivers/ata/sata_dwc_460ex.c +6 ~~-13~~

index d4c554f0979d..1561adea323a 100644
@@ -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")) {

Signature · 1 line

2.55.0

Rosen Penev

author

2026-07-02 00:03 UTC #3 lore raw

PATCH v2 3/8 ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered

In reply to 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 <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/ata/sata_dwc_460ex.c +2 -3

index 1561adea323a..43011789c75e 100644
@@ -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:

Signature · 1 line

2.55.0

Rosen Penev

author

2026-07-02 00:03 UTC #4 lore raw

PATCH v2 4/8 ata: sata_dwc_460ex: drop redundant struct copy of port_info

In reply to 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 <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/ata/sata_dwc_460ex.c +2 -2

index 43011789c75e..2a28e6655bba 100644
@@ -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 */

Signature · 1 line

2.55.0

[email protected]

AI review

2026-07-02 00:13 UTC #5 lore raw

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

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

Pre-existing issues:
- [High] Returning success (0) on ata_host_activate() failure causes a kernel panic on device removal.
- [High] Missing cleanup of DMA resources on probe error paths leads to Use-After-Free. --

commit 1231cd1ac0d6ac3ad6a4b61a9f9cf20df21198d9
Author: Rosen Penev <[email protected]>
Subject: ata: sata_dwc_460ex: drop redundant struct copy of port_info

This patch optimizes sata_dwc_probe() by dropping an unnecessary local copy of sata_dwc_port_info[0]. It directly points to the static data instead, saving stack space and correcting the const qualification.

Quoted 5 lines

drivers/ata/sata_dwc_460ex.c +0 -0
index 4fc22ce4bd9aa..f262b00384ac4 100644

drivers/ata/sata_dwc_460ex.c static int sata_dwc_probe(struct platform_device *ofdev) · line 1141

Quoted 10 lines

drivers/ata/sata_dwc_460ex.c +2 -2

@@ -1134,8 +1134,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 */

[Severity: High]
This is a pre-existing issue, but does sata_dwc_probe() correctly handle ata_host_activate() failures?

Looking further down in drivers/ata/sata_dwc_460ex.c:sata_dwc_probe(), the driver logs an error but returns 0 if activation fails:

	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
	if (err)
		dev_err(dev, "failed to activate host");

	return 0;

If ata_host_activate() fails (for example, if devm_request_irq() or scsi_host_alloc() fails), returning 0 falsely reports successful initialization to the driver core.

When the device is later unbound or removed, sata_dwc_remove() is invoked and calls ata_host_detach() which calls ata_port_detach(). Since the scsi_host was never properly allocated or was freed, scsi_remove_host() will dereference a null pointer and cause a kernel panic.

Could the return statement be updated to return the error code instead?

[Severity: High]
This is a pre-existing issue, but does sata_dwc_probe() leak dma resources on error paths?

If CONFIG_SATA_DWC_OLD_DMA is enabled and no "dmas" property is present, sata_dwc_dma_init_old() is called:

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

This allocates a dma controller via devm_kzalloc, requests an interrupt, and registers it with the dma engine subsystem.

If a subsequent probe step fails, such as the phy retrieval:

	if (IS_ERR(hsdev->phy))
		return PTR_ERR(hsdev->phy);

or later in the main error path:

error_out:

	phy_exit(hsdev->phy);
	return err;

The function returns without calling sata_dwc_dma_exit_old(). The driver core will free the devm-managed memory, including the dma controller structures. However, because explicit teardown was bypassed, the dma engine subsystem and the interrupt layer will retain pointers to this freed memory. This can lead to a use-after-free if the shared interrupt fires or the dma subsystem accesses the controller.

Should the error paths be updated to call sata_dwc_dma_exit_old() instead?

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Rosen Penev

author

2026-07-02 00:28 UTC #6 lore raw

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

Quoted 6 lines · On Wed, Jul 1, 2026 at 5:13 PM <[email protected]> wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Returning success (0) on ata_host_activate() failure causes a kernel panic on device removal.
- [High] Missing cleanup of DMA resources on probe error paths leads to Use-After-Free.

Both fixed later in the series. This thing is still broken when changing [PATCH] to something else.

Quoted 87 lines

--
commit 1231cd1ac0d6ac3ad6a4b61a9f9cf20df21198d9
Author: Rosen Penev <[email protected]>
Subject: ata: sata_dwc_460ex: drop redundant struct copy of port_info
This patch optimizes sata_dwc_probe() by dropping an unnecessary local copy of sata_dwc_port_info[0]. It directly points to the static data instead, saving stack space and correcting the const qualification.
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c +0 -0
index 4fc22ce4bd9aa..f262b00384ac4 100644
drivers/ata/sata_dwc_460ex.c static int sata_dwc_probe(struct platform_device *ofdev) · line 1141
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c +2 -2
@@ -1134,8 +1134,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 */
[Severity: High]
This is a pre-existing issue, but does sata_dwc_probe() correctly handle ata_host_activate() failures?
Looking further down in drivers/ata/sata_dwc_460ex.c:sata_dwc_probe(), the driver logs an error but returns 0 if activation fails:
        err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
        if (err)
                dev_err(dev, "failed to activate host");

        return 0;
If ata_host_activate() fails (for example, if devm_request_irq() or scsi_host_alloc() fails), returning 0 falsely reports successful initialization to the driver core.
When the device is later unbound or removed, sata_dwc_remove() is invoked and calls ata_host_detach() which calls ata_port_detach(). Since the scsi_host was never properly allocated or was freed, scsi_remove_host() will dereference a null pointer and cause a kernel panic.
Could the return statement be updated to return the error code instead?
[Severity: High]
This is a pre-existing issue, but does sata_dwc_probe() leak dma resources on error paths?
If CONFIG_SATA_DWC_OLD_DMA is enabled and no "dmas" property is present, sata_dwc_dma_init_old() is called:
        err = sata_dwc_dma_init_old(ofdev, hsdev);
        if (err)
                return err;
This allocates a dma controller via devm_kzalloc, requests an interrupt, and registers it with the dma engine subsystem.
If a subsequent probe step fails, such as the phy retrieval:
        if (IS_ERR(hsdev->phy))
                return PTR_ERR(hsdev->phy);
or later in the main error path:
error_out:
        phy_exit(hsdev->phy);
        return err;
The function returns without calling sata_dwc_dma_exit_old(). The driver core will free the devm-managed memory, including the dma controller structures. However, because explicit teardown was bypassed, the dma engine subsystem and the interrupt layer will retain pointers to this freed memory. This can lead to a use-after-free if the shared interrupt fires or the dma subsystem accesses the controller.
Should the error paths be updated to call sata_dwc_dma_exit_old() instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Rosen Penev

author

2026-07-02 00:03 UTC #7 lore raw

PATCH v2 5/8 ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

In reply to 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 <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/ata/sata_dwc_460ex.c +1 -1

index 2a28e6655bba..4c54c3ecd18e 100644
@@ -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);

Signature · 1 line

2.55.0

[email protected]

AI review

2026-07-02 00:16 UTC #8 lore raw

Re: [PATCHv2 5/8] ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

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

Pre-existing issues:
- [Critical] The NCQ tag completion logic contains an infinite loop due to a left shift instead of a right shift, which will cause a hard CPU hang inside a spinlock-protected hardirq context.
- [High] Unconditionally zeroing hsdev->sactive_issued on every interrupt destroys persistent NCQ state, breaking NCQ completion tracking.
- [High] The clear_interrupt_bit() function ignores the bit argument and inadvertently clears all pending hardware interrupts.
--

commit 2746dc992432ebc7a9253d28cee91b6dd821a51d
Author: Rosen Penev <[email protected]>

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

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

drivers/ata/sata_dwc_460ex.c static irqreturn_t sata_dwc_isr(int irq, void *dev_instance) · line 475

Quoted 12 lines

drivers/ata/sata_dwc_460ex.c +1 -1

index 4fc22ce4bd9aa..9a4fb911d3215 100644
@@ -470,9 +470,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 unconditionally zeroing hsdev->sactive_issued on every entry into sata_dwc_isr() destroy persistent NCQ state?

It looks like this breaks NCQ completion tracking by erasing tags set during SATA_DWC_INTPR_NEWFP handling.

Would this force a fallback to a non-standard NCQ completion path that handles commands individually?

drivers/ata/sata_dwc_460ex.c static irqreturn_t sata_dwc_isr(int irq, void *dev_instance) · line 475

Quoted 3 lines

drivers/ata/sata_dwc_460ex.c +0 -0

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

[Severity: Critical]
This is a pre-existing issue, but does the NCQ tag completion logic contain an infinite loop when processing completions?

Inside the NCQ completion block of sata_dwc_isr():

        while (!(tag_mask & 0x00000001)) {
                tag++;
                tag_mask <<= 1;
        }

Because tag_mask is left-shifted, bit 0 will remain 0 and tag_mask eventually evaluates to 0. At that point, !(0 & 1) is always true.

Would this cause the loop to spin forever inside a spinlock-protected hardirq context?

[Severity: High]
This isn't a bug introduced by this patch, but does the clear_interrupt_bit() function ignore the bit argument and inadvertently clear all pending hardware interrupts?

drivers/ata/sata_dwc_460ex.c:clear_interrupt_bit()

        sata_dwc_writel(&hsdev->sata_dwc_regs->intpr,
                        sata_dwc_readl(&hsdev->sata_dwc_regs->intpr));

Since it reads the intpr register and writes the exact read value back, assuming a standard Write-1-to-Clear (W1C) register model, would this clear all pending interrupts?

Could hardware interrupts that occur simultaneously with NEWFP interrupts be permanently lost?

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Rosen Penev

author

2026-07-02 00:04 UTC #9 lore raw

PATCH v2 6/8 ata: sata_dwc_460ex: disable SATA interrupts on device removal

In reply to 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 <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/ata/sata_dwc_460ex.c +8 -0

index 4c54c3ecd18e..f26220cac130 100644
@@ -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);

Signature · 1 line

2.55.0

[email protected]

AI review

2026-07-02 00:18 UTC #10 lore raw

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

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

New issues:
- [High] Masking hardware interrupts before calling ata_host_detach() breaks the SCSI teardown sequence, leading to command timeouts, shutdown hangs, and potentially unwritten disk caches.
- [Medium] Missing read-back to flush posted writes when disabling hardware interrupts in sata_dwc_disable_interrupts().

Pre-existing issues:
- [High] sata_dwc_probe() incorrectly returns 0 (success) even if ata_host_activate() fails, leading to unhandled error paths and crashes on remove.
- [High] Masking interrupts without calling synchronize_irq() allows an in-flight interrupt handler to execute concurrently with hardware teardown. --

commit df81e2008992f864aa17e308c42b08931c2b48f5
Author: Rosen Penev <[email protected]>

ata: sata_dwc_460ex: disable SATA interrupts on device removal

This commit adds sata_dwc_disable_interrupts() and calls it at the beginning of sata_dwc_remove(). It masks the controller's INTMR and ERRMR registers to prevent unhandled interrupt storms during teardown.

drivers/ata/sata_dwc_460ex.c static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc) · line 768

Quoted 12 lines

drivers/ata/sata_dwc_460ex.c +4 -0

index 4fc22ce4bd9aa..62580a58ad76e 100644
@@ -762,6 +762,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);

[Severity: Medium]
Will these posted writes reach the hardware predictably before the CPU moves on to tear down the device?

Since sata_dwc_writel() wraps writel_relaxed(), writes to the memory-mapped registers over the system bus can be posted. Without a subsequent dummy read to a controller register to flush them, the interrupt mask configuration might be delayed in write buffers. This could allow an interrupt to fire during teardown, which is exactly what this change is intended to prevent.

Quoted 2 lines

drivers/ata/sata_dwc_460ex.c +2 -0
+}
+

[ ... ]

drivers/ata/sata_dwc_460ex.c static void sata_dwc_remove(struct platform_device *ofdev) · line 1227

Quoted 6 lines

drivers/ata/sata_dwc_460ex.c +2 -0

@@ -1217,6 +1223,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);
+

[Severity: High]
Does masking interrupts here break the SCSI teardown sequence?

When ata_host_detach() is called, it instructs the SCSI layer to issue cache flush and spindown commands. If hardware interrupts are already masked, won't these commands fail to complete and eventually time out? This could lead to a system hang during device removal and potential data loss if caches are not successfully flushed.

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence leave a race window with the still-registered ISR?

The interrupt handler is registered using devm_request_irq() and is only freed by devres after sata_dwc_remove() returns. While the new masking stops new interrupts, it does not wait for an already-running ISR on another CPU to finish. Since synchronize_irq() isn't called, could the ISR execute concurrently with ata_host_detach(), phy_exit(), and sata_dwc_dma_exit_old(), accessing freed memory or hardware in an invalid state?

[Severity: High]
This is a pre-existing issue, but can sata_dwc_remove() crash if the initial device probe failed?

Looking at sata_dwc_probe():

    err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
    if (err)
        dev_err(dev, "failed to activate host");

    return 0;

If ata_host_activate() fails, probe still returns 0 (success). When the driver is later unbound, sata_dwc_remove() will unconditionally call ata_host_detach() and attempt to tear down host structures that were never successfully initialized, potentially causing a NULL pointer dereference or list corruption.

Quoted 1 line

drivers/ata/sata_dwc_460ex.c +0 -0
 	ata_host_detach(host);

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Rosen Penev

author

2026-07-02 00:04 UTC #11 lore raw

PATCH v2 7/8 ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

In reply to 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 <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

drivers/ata/sata_dwc_460ex.c +19 ~~-13~~

index f26220cac130..e8e2790be1d6 100644
@@ -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);

Signature · 1 line

2.55.0

Rosen Penev

author

2026-07-02 00:04 UTC #12 lore raw

PATCH v2 8/8 ata: sata_dwc_460ex: use devm for old DMA resource lifetime management

In reply to 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 <[email protected]>

---
 drivers/ata/sata_dwc_460ex.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

drivers/ata/sata_dwc_460ex.c +13 ~~-21~~

index e8e2790be1d6..352cd7f02163 100644
@@ -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");
 }

Signature · 1 line

2.55.0

Damien Le Moal

maintainer

2026-07-02 02:02 UTC #13 lore raw

In reply to Rosen Penev

Quoted 11 lines · 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.

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.

Quoted 19 lines

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

Signature · 2 lines

Damien Le Moal
Western Digital Research