[PATCH v5 0/6] ata: Do not release the host resources twice on probe() failure

[PATCH v5 0/6] ata: Do not release the host resources twice on probe() failure

[Niklas Cassel]

ata_host_start() registers ata_host_stop() as a devres action as soon as
it has succeeded:

	if (have_stop) {
		start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
	...
	if (start_dr)
		devres_add(host->dev, start_dr);
	host->flags |= ATA_HOST_STARTED;

From that point on, releasing the host resources is owned by devres: when
probe() fails, the driver core calls devres_release_all(), which calls
ata_host_stop(), which calls ->port_stop() and ->host_stop().

ata_host_activate() and ahci_host_activate_multi_irqs() can however fail
after ata_host_start() has succeeded - devm_kasprintf(),
devm_request_irq() (a shared IRQ conflict) and ata_host_register()
(scsi_add_host(), ata_tport_add()) can all fail - and they return the
error with the devres action still registered. Since the caller cannot
tell whether ata_host_start() succeeded, and since it has to release the
resources for the failures happening before that, all the ahci-platform
drivers release the host resources in their probe() error path, e.g.
ahci_probe() calls ahci_platform_disable_resources() while
ahci_host_stop() does the same through devres.

The clocks, regulators, resets and PHYs of the host are therefore
released twice, which gives refcount underflow warnings from the clk,
regulator and phy cores and, for shared resources, can disable resources
which are still in use by other devices.

Patch 5 adds ata_host_undo_start(), which stops the ports and drops the
devres action without calling ->host_stop(), and calls it from both
activation helpers when they fail, so that "on failure, the caller
releases what it acquired" holds for all of them. sata_qstor and sata_fsl
are the only drivers which implement ->host_stop() while having no error
handling at all for the activate host call, so they get some.

Patches 2 to 4 have to come first, as they fix error paths which patch 5
builds upon, and which are worth fixing on their own:

- ahci_st is the only ahci-platform driver whose ->host_stop() does more
  than its probe() error path, as it also asserts the "pwr-dwn" reset,
  so patch 5 would stop that reset from being asserted (patch 2).
- sata_fsl releases hcr_base and host_priv in its probe() error path
  although sata_fsl_host_stop() does that as well, which is a
  use-after-free that the two device_create_file() calls after
  ata_host_activate() can trigger today (patch 3).
- ahci_host_activate_multi_irqs() never frees the IRQs which it
  requested when it fails, so the IRQ handlers stay registered while the
  caller releases the resources of the host (patch 4).

Patch 1 is an independent ahci_st fix, reported by Sashiko:
st_ahci_probe_resets() treats every error from devm_reset_control_get()
as "reset control not defined", including -EPROBE_DEFER, so probe()
continues with the SATA IP held in reset and powered down instead of
being deferred.

Patch 6 is an unrelated kdoc fix that I noticed while documenting the
above.

Changes since v4:
- New patch 1, which stops ahci_st from ignoring the errors returned
  when getting its reset controls, most notably -EPROBE_DEFER. The other
  patches are renumbered accordingly.
- Patch 3 no longer touches the ata_host_activate() error path, it only
  fixes the use-after-free which the two device_create_file() calls can
  trigger, so it no longer depends on the rest of the series. Checking
  the return value of ata_host_activate(), and releasing hcr_base and
  host_priv when activating the host fails, is now done by patch 5,
  where it can be done without leaking them.
- Drop the extern from the ata_host_undo_start() declaration in patch 5.

Niklas Cassel (6):
  ata: ahci_st: Do not ignore errors when getting the reset controls
  ata: ahci_st: Assert the power down reset in the probe() error path
  ata: sata_fsl: Fix use-after-free of host_priv on probe() failure
  ata: libahci: Free the IRQs when activating a multi-IRQ host fails
  ata: libata: Do not leave ata_host_stop() registered when activation
    fails
  ata: libata-core: Fix the ata_host_register() kdoc

 drivers/ata/ahci_st.c          | 78 +++++++++++++++++---------------
 drivers/ata/libahci.c          | 28 +++++++++++-
 drivers/ata/libahci_platform.c |  4 ++
 drivers/ata/libata-core.c      | 81 ++++++++++++++++++++++++++++++----
 drivers/ata/libata-sff.c       |  5 +++
 drivers/ata/sata_fsl.c         | 24 +++++++---
 drivers/ata/sata_qstor.c       |  8 +++-
 include/linux/libata.h         |  1 +
 8 files changed, 173 insertions(+), 56 deletions(-)

-- 
2.55.0

[PATCH v5 1/6] ata: ahci_st: Do not ignore errors when getting the reset controls

[Niklas Cassel]

st_ahci_probe_resets() treats any error from devm_reset_control_get() as
"reset control not defined" and continues with a NULL reset control:

	drv_data->pwr = devm_reset_control_get(dev, "pwr-dwn");
	if (IS_ERR(drv_data->pwr)) {
		dev_info(dev, "power reset control not defined\n");
		drv_data->pwr = NULL;
	}

This also swallows -EPROBE_DEFER, which is returned when the reset
controller providing the reset has not been probed yet. probe() then
continues as if the device tree did not specify any reset, so the resets
of the SATA IP are never deasserted, and st_ahci_configure_oob() and
ahci_platform_init_host() access the MMIO of an IP which is still held in
reset and powered down, which can result in an external abort.

The resets are optional in the binding, as they are not part of its
required properties, so use devm_reset_control_get_optional(), which
returns NULL if the reset is not specified in the device tree, and
propagate all other errors.

Note that an absent reset is now indicated by a NULL reset control
instead of an error, so the "reset control not defined" messages are
dropped.

Fixes: 76884cb2f7da ("ahci: st: Add support for ST's SATA IP")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci_st.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c
index 4336c8a6e208..819269a022ef 100644
--- a/drivers/ata/ahci_st.c
+++ b/drivers/ata/ahci_st.c
@@ -104,23 +104,20 @@ static int st_ahci_probe_resets(struct ahci_host_priv *hpriv,
 {
 	struct st_ahci_drv_data *drv_data = hpriv->plat_data;
 
-	drv_data->pwr = devm_reset_control_get(dev, "pwr-dwn");
-	if (IS_ERR(drv_data->pwr)) {
-		dev_info(dev, "power reset control not defined\n");
-		drv_data->pwr = NULL;
-	}
-
-	drv_data->sw_rst = devm_reset_control_get(dev, "sw-rst");
-	if (IS_ERR(drv_data->sw_rst)) {
-		dev_info(dev, "soft reset control not defined\n");
-		drv_data->sw_rst = NULL;
-	}
-
-	drv_data->pwr_rst = devm_reset_control_get(dev, "pwr-rst");
-	if (IS_ERR(drv_data->pwr_rst)) {
-		dev_dbg(dev, "power soft reset control not defined\n");
-		drv_data->pwr_rst = NULL;
-	}
+	drv_data->pwr = devm_reset_control_get_optional(dev, "pwr-dwn");
+	if (IS_ERR(drv_data->pwr))
+		return dev_err_probe(dev, PTR_ERR(drv_data->pwr),
+				     "failed to get pwr-dwn reset\n");
+
+	drv_data->sw_rst = devm_reset_control_get_optional(dev, "sw-rst");
+	if (IS_ERR(drv_data->sw_rst))
+		return dev_err_probe(dev, PTR_ERR(drv_data->sw_rst),
+				     "failed to get sw-rst reset\n");
+
+	drv_data->pwr_rst = devm_reset_control_get_optional(dev, "pwr-rst");
+	if (IS_ERR(drv_data->pwr_rst))
+		return dev_err_probe(dev, PTR_ERR(drv_data->pwr_rst),
+				     "failed to get pwr-rst reset\n");
 
 	return st_ahci_deassert_resets(hpriv, dev);
 }
-- 
2.55.0

[PATCH v5 2/6] ata: ahci_st: Assert the power down reset in the probe() error path

[Niklas Cassel]

st_ahci_probe_resets() deasserts the "pwr-dwn" reset, but the probe()
error path of st_ahci_probe() only releases the host resources, so the
SATA IP is left powered up when probe() fails after
st_ahci_probe_resets() has succeeded, e.g. when
ahci_platform_enable_resources() fails.

The reset is asserted by st_ahci_host_stop(), however ->host_stop() is
only called through the ata_host_stop() devres action registered by
ata_host_start(), so it does not cover any failure happening before the
host has been started.

Factor the assert out into st_ahci_assert_pwrdwn() and call it when
either ahci_platform_enable_resources() or ahci_platform_init_host()
fails. The "pwr-dwn" reset control is an exclusive one, so asserting it
once more from st_ahci_host_stop() is harmless.

No functional change intended for ->host_stop() and st_ahci_suspend().

Fixes: 76884cb2f7da ("ahci: st: Add support for ST's SATA IP")
Cc: stable@vger.kernel.org
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci_st.c | 51 +++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c
index 819269a022ef..eea75961038c 100644
--- a/drivers/ata/ahci_st.c
+++ b/drivers/ata/ahci_st.c
@@ -83,18 +83,27 @@ static int st_ahci_deassert_resets(struct ahci_host_priv *hpriv,
 	return 0;
 }
 
-static void st_ahci_host_stop(struct ata_host *host)
+static int st_ahci_assert_pwrdwn(struct ahci_host_priv *hpriv,
+				 struct device *dev)
 {
-	struct ahci_host_priv *hpriv = host->private_data;
 	struct st_ahci_drv_data *drv_data = hpriv->plat_data;
-	struct device *dev = host->dev;
 	int err;
 
-	if (drv_data->pwr) {
-		err = reset_control_assert(drv_data->pwr);
-		if (err)
-			dev_err(dev, "unable to pwrdwn\n");
-	}
+	if (!drv_data->pwr)
+		return 0;
+
+	err = reset_control_assert(drv_data->pwr);
+	if (err)
+		dev_err(dev, "unable to pwrdwn\n");
+
+	return err;
+}
+
+static void st_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	st_ahci_assert_pwrdwn(hpriv, host->dev);
 
 	ahci_platform_disable_resources(hpriv);
 }
@@ -159,38 +168,38 @@ static int st_ahci_probe(struct platform_device *pdev)
 
 	err = ahci_platform_enable_resources(hpriv);
 	if (err)
-		return err;
+		goto assert_pwrdwn;
 
 	st_ahci_configure_oob(hpriv->mmio);
 
 	err = ahci_platform_init_host(pdev, hpriv, &st_ahci_port_info,
 				      &ahci_platform_sht);
-	if (err) {
-		ahci_platform_disable_resources(hpriv);
-		return err;
-	}
+	if (err)
+		goto disable_resources;
 
 	return 0;
+
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+assert_pwrdwn:
+	st_ahci_assert_pwrdwn(hpriv, &pdev->dev);
+
+	return err;
 }
 
 static int st_ahci_suspend(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
-	struct st_ahci_drv_data *drv_data = hpriv->plat_data;
 	int err;
 
 	err = ahci_platform_suspend_host(dev);
 	if (err)
 		return err;
 
-	if (drv_data->pwr) {
-		err = reset_control_assert(drv_data->pwr);
-		if (err) {
-			dev_err(dev, "unable to pwrdwn");
-			return err;
-		}
-	}
+	err = st_ahci_assert_pwrdwn(hpriv, dev);
+	if (err)
+		return err;
 
 	ahci_platform_disable_resources(hpriv);
 
-- 
2.55.0

Re: [PATCH v5 1/6] ata: ahci_st: Do not ignore errors when getting the reset controls

[Damien Le Moal]

On 2026/09/15 15:41, Niklas Cassel wrote:
> st_ahci_probe_resets() treats any error from devm_reset_control_get() as
> "reset control not defined" and continues with a NULL reset control:
> 
> 	drv_data->pwr = devm_reset_control_get(dev, "pwr-dwn");
> 	if (IS_ERR(drv_data->pwr)) {
> 		dev_info(dev, "power reset control not defined\n");
> 		drv_data->pwr = NULL;
> 	}
> 
> This also swallows -EPROBE_DEFER, which is returned when the reset
> controller providing the reset has not been probed yet. probe() then
> continues as if the device tree did not specify any reset, so the resets
> of the SATA IP are never deasserted, and st_ahci_configure_oob() and
> ahci_platform_init_host() access the MMIO of an IP which is still held in
> reset and powered down, which can result in an external abort.
> 
> The resets are optional in the binding, as they are not part of its
> required properties, so use devm_reset_control_get_optional(), which
> returns NULL if the reset is not specified in the device tree, and
> propagate all other errors.
> 
> Note that an absent reset is now indicated by a NULL reset control
> instead of an error, so the "reset control not defined" messages are
> dropped.
> 
> Fixes: 76884cb2f7da ("ahci: st: Add support for ST's SATA IP")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 0/6] ata: Do not release the host resources twice on probe() failure

[Niklas Cassel]

On Tue, 15 Sep 2026 10:41:26 +0200, Niklas Cassel wrote:
> ata_host_start() registers ata_host_stop() as a devres action as soon as
> it has succeeded:
> 
> 	if (have_stop) {
> 		start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
> 	...
> 	if (start_dr)
> 		devres_add(host->dev, start_dr);
> 	host->flags |= ATA_HOST_STARTED;
> 
> [...]

Applied to libata/linux.git (for-7.4), thanks!

[1/6] ata: ahci_st: Do not ignore errors when getting the reset controls
      https://git.kernel.org/libata/linux/c/c74ee923
[2/6] ata: ahci_st: Assert the power down reset in the probe() error path
      https://git.kernel.org/libata/linux/c/91e0a745
[3/6] ata: sata_fsl: Fix use-after-free of host_priv on probe() failure
      https://git.kernel.org/libata/linux/c/70b11967
[4/6] ata: libahci: Free the IRQs when activating a multi-IRQ host fails
      https://git.kernel.org/libata/linux/c/34a4a344
[5/6] ata: libata: Do not leave ata_host_stop() registered when activation fails
      https://git.kernel.org/libata/linux/c/d2f97700
[6/6] ata: libata-core: Fix the ata_host_register() kdoc
      https://git.kernel.org/libata/linux/c/52d19459

Kind regards,
Niklas