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

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

[Rosen Penev]

Fix various issues flagged by Sashiko against the original submission of this driver.

v3: Shrink series to Fixes on the initial commit.
v2: sashiko fixes.

Rosen Penev (5):
  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: fix clear_interrupt_bit() clearing all pending
    interrupts
  ata: sata_dwc_460ex: fix infinite loop in NCQ tag completion
    bit-scanning
  ata: sata_dwc_460ex: disable SATA interrupts on device removal

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

-- 
2.55.0

[PATCHv3 1/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.

Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, sata_dwc_460ex")
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 4fc22ce4bd9a..442c6e9d06e0 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 (!of_property_present(np, "dmas")) {
-- 
2.55.0

[PATCHv3 2/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.

Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, sata_dwc_460ex")
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 442c6e9d06e0..e206fbf6d2e9 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1164,9 +1164,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)
@@ -1197,6 +1194,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

[PATCHv3 3/5] ata: sata_dwc_460ex: fix clear_interrupt_bit() clearing all pending interrupts

[Rosen Penev]

clear_interrupt_bit() ignores the bit argument and performs a
read-write-back of the entire INTPR register.  If INTPR uses standard
Write-1-to-Clear semantics, this clears every pending interrupt bit,
not just the intended one.  Coalesced interrupts (e.g. DMAT + NEWFP)
would be cleared together, silently losing the second event.

Write only the specific bit to clear so that other pending interrupts
are preserved.

Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, sata_dwc_460ex")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index e206fbf6d2e9..097b060a9ee6 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -394,8 +394,7 @@ static void clear_serror(struct ata_port *ap)
 
 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));
+	sata_dwc_writel(&hsdev->sata_dwc_regs->intpr, bit);
 }
 
 static u32 qcmd_tag_to_mask(u8 tag)
-- 
2.55.0

[PATCHv3 4/5] ata: sata_dwc_460ex: fix infinite loop in NCQ tag completion bit-scanning

[Rosen Penev]

The hand-rolled bit-scanning loop in the NCQ completion path has an
infinite loop bug.  When tag_mask has only high bits set (e.g.
0x80000000), the inner while loop left-shifts tag_mask until it
overflows to 0.  At that point !(0 & 1) is always true and 0 <<= 1
stays 0, causing an infinite loop in hardirq context with a spinlock
held.

Replace the open-coded bit-scanning with __ffs() which correctly
finds the least significant set bit and is bounded by the width of
the argument.

Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, sata_dwc_460ex")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 097b060a9ee6..08d40e287bac 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -607,14 +607,9 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 	status = ap->ops->sff_check_status(ap);
 	dev_dbg(ap->dev, "%s ATA status register=0x%x\n", __func__, status);
 
-	tag = 0;
 	while (tag_mask) {
-		while (!(tag_mask & 0x00000001)) {
-			tag++;
-			tag_mask <<= 1;
-		}
-
-		tag_mask &= (~0x00000001);
+		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");
-- 
2.55.0

[PATCHv3 5/5] 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().

Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, sata_dwc_460ex"
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/sata_dwc_460ex.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 08d40e287bac..381ace5d33e1 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -752,6 +752,14 @@ 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);
+	/* Flush posted writes */
+	sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);
+}
+
 static void sata_dwc_enable_interrupts(struct sata_dwc_device *hsdev)
 {
 	/* Enable selective interrupts by setting the interrupt maskregister*/
@@ -1203,6 +1211,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

Re: [PATCHv3 1/5] ata: sata_dwc_460ex: use platform_get_irq()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch removes the local variable `np` from `sata_dwc_probe()`, causing a build failure when `CONFIG_SATA_DWC_OLD_DMA` is enabled.
--

--- Patch [1]: [PATCHv3 1/5] ata: sata_dwc_460ex: use platform_get_irq() ---
commit 74bc1be9c4385029a4079d000bf78ff4452fc266
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: use platform_get_irq()

This commit replaces irq_of_parse_and_map() with platform_get_irq() in both
sata_dwc_dma_init_old() and sata_dwc_probe(). It improves error handling for
IRQs and removes the unnecessary include of of_irq.h.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..442c6e9d06e0a 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c

[ ... ]

> @@ -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 (!of_property_present(np, "dmas")) {

[Severity: Low]
Will this cause a build failure when compiling with CONFIG_SATA_DWC_OLD_DMA
enabled?

The earlier hunk removes the declaration of the np variable, but it appears
to still be referenced here in sata_dwc_probe(). Could dev->of_node be passed
to of_property_present() instead to avoid an undeclared identifier error?

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

Re: [PATCHv3 5/5] ata: sata_dwc_460ex: disable SATA interrupts on device removal

[sashiko-bot]

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

New issues:
- [High] Disabling hardware interrupts before `ata_host_detach()` prevents pending I/O and device unload commands from completing, resulting in command timeouts during teardown.
- [High] `sata_dwc_disable_interrupts()` is called in the remove path without synchronization, creating a race with the Error Handling (EH) thread that can cause interrupts to be incorrectly re-enabled.

Pre-existing issues:
- [High] Pre-existing race condition missing IRQ synchronization before freeing DMA and PHY resources during device removal, risking Use-After-Free.
- [High] `clear_interrupt_bit()` unintentionally clears all pending interrupts, causing lost interrupts due to a flawed read-modify-write on a W1C register.
--

--- Patch [1]: [PATCHv3 5/5] ata: sata_dwc_460ex: disable SATA interrupts on device removal ---
commit 65387714eba3b122c58a0cdaf1a76af59f78680c
Author: Rosen Penev <rosenp@gmail.com>
Subject: 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() to prevent unhandled interrupt storms
during host teardown.

Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, sata_dwc_460ex"

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..7c858a7992c9c 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c

[ ... ]

> @@ -1217,6 +1225,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 this cause teardown commands to time out?

When sata_dwc_remove() calls ata_host_detach(), the SCSI devices are
detached, which typically issues teardown commands like SYNCHRONIZE CACHE.
Because interrupts are already masked by sata_dwc_disable_interrupts(), will
these commands fail to receive their completion IRQs and hang until they time
out?

[Severity: High]
Does masking interrupts here also create a race with the libata error handler?

If the teardown commands time out as mentioned above, the libata error
handling (EH) thread will execute. The EH thread calls sata_dwc_hardreset(),
which explicitly calls sata_dwc_enable_interrupts(). Could this inadvertently
re-enable interrupts while sata_dwc_remove() is still running, recreating the
interrupt storm this patch is trying to fix?

>  	ata_host_detach(host);
>  
>  	phy_exit(hsdev->phy);

[Severity: High]
This is a pre-existing issue, but does device removal leave a use-after-free
window?

In sata_dwc_remove(), the PHY and DMA resources are destroyed before the
devres-managed IRQ handler is unregistered. If an unexpected interrupt fires
and sata_dwc_isr() executes, could it access these freed resources before
devres cleans up the IRQ?

[ ... ]

While reviewing this driver, I noticed another pre-existing issue not touched
by this patch:

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));
}

[Severity: High]
This is a pre-existing issue, but does clear_interrupt_bit() unintentionally
clear all pending interrupts?

Looking at clear_interrupt_bit(), the function ignores its 'bit' argument and
reads then writes back the intpr register. Because intpr is a write-1-to-clear
(W1C) register, does writing back the read value clear every currently pending
interrupt, including those that have arrived since the ISR started but haven't
been processed yet?

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

Re: [PATCHv3 1/5] ata: sata_dwc_460ex: use platform_get_irq()

[Rosen Penev]

On Wed, Jul 8, 2026 at 7:47 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The patch removes the local variable `np` from `sata_dwc_probe()`, causing a build failure when `CONFIG_SATA_DWC_OLD_DMA` is enabled.
Rebasing error. Will fix.
> --
>
> --- Patch [1]: [PATCHv3 1/5] ata: sata_dwc_460ex: use platform_get_irq() ---
> commit 74bc1be9c4385029a4079d000bf78ff4452fc266
> Author: Rosen Penev <rosenp@gmail.com>
>
> ata: sata_dwc_460ex: use platform_get_irq()
>
> This commit replaces irq_of_parse_and_map() with platform_get_irq() in both
> sata_dwc_dma_init_old() and sata_dwc_probe(). It improves error handling for
> IRQs and removes the unnecessary include of of_irq.h.
>
> > diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> > index 4fc22ce4bd9aa..442c6e9d06e0a 100644
> > --- a/drivers/ata/sata_dwc_460ex.c
> > +++ b/drivers/ata/sata_dwc_460ex.c
>
> [ ... ]
>
> > @@ -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 (!of_property_present(np, "dmas")) {
>
> [Severity: Low]
> Will this cause a build failure when compiling with CONFIG_SATA_DWC_OLD_DMA
> enabled?
>
> The earlier hunk removes the declaration of the np variable, but it appears
> to still be referenced here in sata_dwc_probe(). Could dev->of_node be passed
> to of_property_present() instead to avoid an undeclared identifier error?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260709023202.2132165-2-rosenp@gmail.com?part=1