[PATCHv2] ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

[PATCHv2] ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

[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.

Fixes: 0f48debdb906 ("ata: sata_dwc_460ex: add phy support")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: resend as standalone patch
 drivers/ata/sata_dwc_460ex.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8e3fc713891a..8a1d80ac906a 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -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);
-- 
2.55.0

Re: [PATCHv2] ata: sata_dwc_460ex: fix PHY lifecycle ordering on device removal

[sashiko-bot]

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 <rosenp@gmail.com>

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.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8e3fc713891af..8a1d80ac906a7 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
[ ... ]
> @@ -1171,29 +1182,26 @@ static int sata_dwc_probe(struct platform_device *ofdev)
[ ... ]
>  	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

[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?

>  	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?

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