ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

PATCH New Rosen Penev 2 participants 2 messages Sep 8 libata

Share & tools

Rosen Penev

author

2026-09-08 21:39 UTC #0 lore raw

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

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.

Fixes 0f48debdb906 ("ata: sata_dwc_460ex: add phy support")

Assisted-by opencode:big-pickle

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

---
v2: resend as standalone patch

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

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

index 8e3fc713891a..8a1d80ac906a 100644
@@ -851,10 +851,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;

@@ -880,6 +884,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:

@@ -897,6 +903,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;

@@ -1163,6 +1170,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 (!of_property_present(dev->of_node, "dmas")) {
 		err = sata_dwc_dma_init_old(ofdev, hsdev);

@@ -1171,29 +1182,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;
 }

@@ -1205,8 +1213,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

[email protected]

AI review

2026-09-08 21:55 UTC #1 lore raw

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

Pre-existing issues:
- [High] dma_release_channel() and dmaengine_terminate_sync() access a destroyed DMA controller during device removal and probe failure. --

commit 620563c58971e90128eb05275238e22226efc115
Author: Rosen Penev <[email protected]>

ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

This patch fixes the PHY lifecycle ordering in sata_dwc_remove() by ensuring phy_power_off() completes before phy_exit() is called. It moves phy_exit() into sata_dwc_port_stop() and reworks the error paths in probe accordingly.

Quoted 4 lines

drivers/ata/sata_dwc_460ex.c +0 -0
index 8e3fc713891af..8a1d80ac906a7 100644

[ ... ]

Quoted 1 line

drivers/ata/sata_dwc_460ex.c +0 -0

@@ -1171,29 +1182,26 @@ static int sata_dwc_probe(struct platform_device *ofdev)

[ ... ]

Quoted 7 lines

drivers/ata/sata_dwc_460ex.c +3 -1

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

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

Quoted 10 lines

drivers/ata/sata_dwc_460ex.c +4 -1

 	/* 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

[Severity: High]
This isn't a bug introduced by this patch, but this patch copies a broken teardown sequence into the probe error path in drivers/ata/sata_dwc_460ex.c.

If ata_host_activate() fails, the DMA controller is explicitly destroyed here by sata_dwc_dma_exit_old(). Devres will then later clean up the ATA host by calling ata_host_stop(), which invokes sata_dwc_port_stop().

sata_dwc_port_stop() calls dmaengine_terminate_sync() and dma_release_channel(). Will this result in a use-after-free and an invalid hardware access to the already suspended and destroyed DMA controller?

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

Quoted 12 lines

drivers/ata/sata_dwc_460ex.c +0 -2

 	return err;
 }

@@ -1205,8 +1213,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);

[Severity: High]
This is a pre-existing issue, but it is present in the device removal path within sata_dwc_remove() in drivers/ata/sata_dwc_460ex.c.

During device removal, sata_dwc_remove() explicitly tears down the DMA controller via sata_dwc_dma_exit_old(). When devres subsequently cleans up the ATA host, it invokes sata_dwc_port_stop() which calls dmaengine_terminate_sync() and dma_release_channel().

Could this cause a hardware fault and logical use-after-free by accessing the unregistered DMA structures?

Signature · 1 line

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