[PATCHv4 0/4] ata: sata_dwc_460ex: cleanups

[PATCHv4 0/4] ata: sata_dwc_460ex: cleanups

[Rosen Penev]

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

v4: remove interrupt fix
v3: Shrink series to Fixes on the initial commit.
v2: sashiko fixes.

Rosen Penev (4):
  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

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

-- 
2.55.0

[PATCHv4 1/4] 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.

Also fix unused variable when CONFIG_SATA_DWC_OLD_DMA is disabled.

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 | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 4fc22ce4bd9a..092272753749 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,14 +1168,12 @@ 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")) {
+	if (!of_property_present(dev->of_node, "dmas")) {
 		err = sata_dwc_dma_init_old(ofdev, hsdev);
 		if (err)
 			return err;
-- 
2.55.0

[PATCHv4 2/4] 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 092272753749..85c5e67e9175 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

[PATCHv4 3/4] 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 85c5e67e9175..bc543a408963 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

[PATCHv4 4/4] 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 bc543a408963..8e3fc713891a 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

Re: [PATCHv4 1/4] ata: sata_dwc_460ex: use platform_get_irq()

[sashiko-bot]

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

Pre-existing issues:
- [High] sata_dwc_probe() ignores the return value from ata_host_activate() and incorrectly returns 0 (success) on failure.
- [High] sata_dwc_probe() fails to clean up DMA resources initialized by sata_dwc_dma_init_old() on error paths, leading to a use-after-free.
- [High] Hardware interrupts are enabled early in probe without being disabled on error paths, potentially causing unhandled interrupt storms.
--

--- Patch [1]: [PATCHv4 1/4] ata: sata_dwc_460ex: use platform_get_irq() ---
commit c2c0e5be0a165f590877664b3703065d59cc686e
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() to improve error reporting. It
also removes the unnecessary of_irq.h include and fixes an unused variable
when CONFIG_SATA_DWC_OLD_DMA is disabled.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..092272753749c 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
[ ... ]
> @@ -1173,14 +1168,12 @@ 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;

[Severity: High]
This isn't a bug introduced by this patch, but hardware interrupts are enabled
just before this by calling sata_dwc_enable_interrupts(). 

If platform_get_irq() fails and returns an error code like -EPROBE_DEFER, the
function returns early without masking the hardware interrupts again.

Could this lead to an unhandled interrupt storm since the hardware registers
are unmasked but no handler is registered yet?

>  
>  #ifdef CONFIG_SATA_DWC_OLD_DMA
> -	if (!of_property_present(np, "dmas")) {
> +	if (!of_property_present(dev->of_node, "dmas")) {
>  		err = sata_dwc_dma_init_old(ofdev, hsdev);
>  		if (err)
>  			return err;

[Severity: High]
This is a pre-existing issue, but looking further down in sata_dwc_probe(),
there is a potential cleanup issue on error paths:

	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;

If getting or initializing the PHY fails, the function exits without cleaning
up the DMA resources that were just initialized by sata_dwc_dma_init_old().

Because sata_dwc_dma_init_old() sets up the DMA engine, does returning here
leave the DMA subsystem with dangling pointers, leading to a use-after-free
when devres cleans up hsdev->dma?

[Severity: High]
This is another pre-existing issue further down in sata_dwc_probe(). The
return value from ata_host_activate() is logged but then discarded:

	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, the function unconditionally returns 0, meaning
the driver core considers the device successfully bound. 

When the driver is unbound later, scsi_remove_host() will be called on an
uninitialized SCSI host. Does this return value need to be propagated instead
of returning 0 to prevent a NULL pointer dereference during teardown?

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

Re: [PATCHv4 0/4] ata: sata_dwc_460ex: cleanups

[Damien Le Moal]

On 7/13/26 06:37, Rosen Penev wrote:
> Fix various issues flagged by Sashiko against the original submission of this driver.
> 
> v4: remove interrupt fix
> v3: Shrink series to Fixes on the initial commit.
> v2: sashiko fixes.
> 
> Rosen Penev (4):
>   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
> 
>  drivers/ata/sata_dwc_460ex.c | 38 ++++++++++++------------------------
>  1 file changed, 12 insertions(+), 26 deletions(-)

I applied this to for-7.2-fixes, but I reversed the first 2 patches.
Thanks!

(if you have time, please send further cleanups to address the other issues
that sashiko signaled).


-- 
Damien Le Moal
Western Digital Research

Re: [PATCHv4 0/4] ata: sata_dwc_460ex: cleanups

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3056 bytes --]

Hello,

On Mon, Jul 13, 2026 at 04:31:53PM +0900, Damien Le Moal wrote:
> On 7/13/26 06:37, Rosen Penev wrote:
> > Fix various issues flagged by Sashiko against the original submission of this driver.
> > 
> > v4: remove interrupt fix
> > v3: Shrink series to Fixes on the initial commit.
> > v2: sashiko fixes.
> > 
> > Rosen Penev (4):
> >   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
> > 
> >  drivers/ata/sata_dwc_460ex.c | 38 ++++++++++++------------------------
> >  1 file changed, 12 insertions(+), 26 deletions(-)
> 
> I applied this to for-7.2-fixes, but I reversed the first 2 patches.
> Thanks!
> 
> (if you have time, please send further cleanups to address the other issues
> that sashiko signaled).

I think the analysis for the fourth patch is wrong (or incomplete), the
original code was (a bit simplified):

        unsigned char tag;
        unsigned int tag_mask;
        ...
        tag_mask = ...;
        ...
        tag = 0;
        while (tag_mask) {
                while (!(tag_mask & 0x1)) {
                        tag++;
                        tag_mask <<= 1;
                }

                tag_mask &= ~0x1;
                ...
        }

Given that tag_mask is shifted left (and not right) the inner while loop
yields an endless loop whenever tag_mask's least significant bit isn't
set initially. Given the outer loop this results in a hang if tag_mask
!= 1. So the issue doesn't only trigger for tag_mask = 0x80000000.

Either this never worked, or the problem doesn't trigger reaching that
code with tag_mask != 1 easily. And I also wonder if the change's
urgency was considered carefully enough to justify a commit in -rc4 to
fix a bug that is already roughly 16 years old.

And similar for the 3 parents of that change
(c2130f6553f4a5cbdc259de069600117a995f197):

For 4bbc16a353a98023e5ddfca7c1fc0e49971cf4d0 I wonder: Does
ata_host_activate() already need the irqs enabled? If yes, the commit
is wrong.

For a4af122106f73ea510bb35a9ea1dedd980fc0db7 I think it's bold to claim
"Also fix unused variable when CONFIG_SATA_DWC_OLD_DMA is disabled."
given that the unused variable warning (I guess about np) was only
introduced during development of this patch.

For 66c4e310ad71f41e41736d33dd8a1fb5eaaec7f3 it disturbs me that the
commit log has: "If INTPR uses standard Write-1-to-Clear semantics,
[...]". Without that the justification of the patch goes away, nobody
checked that?

All four commits have an Assisted-by tag, and I have the impression that
nobody involved in these commits has the hardware or even the hardware
documentation. But maybe I'm just to picky about changes that enter the
mainline in the stabilization phase. 🤷

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]