[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