[PATCH] ata: ahci_brcm: fix reset refcount leak in brcm_ahci_resume()

[PATCH] ata: ahci_brcm: fix reset refcount leak in brcm_ahci_resume()

[Wentao Liang]

When brcm_ahci_resume() succeeds with reset_control_reset(), any
subsequent failure in ahci_platform_enable_clks(),
ahci_platform_enable_regulators(), ahci_platform_enable_phys(),
or ahci_platform_resume_host() leaves the shared reset line's
triggered_count incremented by one. On the next attempt to reset
the hardware, atomic_inc_return() sees a count greater than one
and the reset is silently skipped, potentially causing data
corruption or device malfunction.

Add a reset_control_rearm() call in the common error path after
brcm_sata_phys_disable() and ahci_platform_disable_regulators()
to properly balance the triggered_count, matching what the probe
error path already does.

Fixes: c0cdf2ac4b5b ("ata: ahci_brcm: Fix AHCI resources management")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/ata/ahci_brcm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 29be74fedcf0..38c63d73d210 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -415,6 +415,7 @@ static int __maybe_unused brcm_ahci_resume(struct device *dev)
 out_disable_phys:
 	brcm_sata_phys_disable(priv);
 	ahci_platform_disable_regulators(hpriv);
+	reset_control_rearm(priv->rcdev_rescal);
 out_disable_clks:
 	ahci_platform_disable_clks(hpriv);
 	return ret;
-- 
2.34.1

Re: [PATCH] ata: ahci_brcm: fix reset refcount leak in brcm_ahci_resume()

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch fails to fix the `reset_control_rearm()` leak for the `ahci_platform_enable_clks()` and `ahci_platform_enable_regulators()` error paths in `brcm_ahci_resume()`, explicitly contradicting its commit message.
- [High] Error paths in `brcm_ahci_resume()` fail to call `reset_control_assert(priv->rcdev_ahci)`.
- [High] Missing `reset_control_rearm(priv->rcdev_rescal)` in `brcm_ahci_probe()` if `reset_control_deassert(priv->rcdev_ahci)` fails.
- [High] `brcm_ahci_remove()` fails to call `reset_control_rearm()` and `reset_control_assert()`, leading to permanent resource leaks on module unload.
--

commit 075a88f5a8b3bff5899fc69114efba28b24bf30d
Author: Wentao Liang <vulab@iscas.ac.cn>
Subject: ata: ahci_brcm: fix reset refcount leak in brcm_ahci_resume()

This patch attempts to fix a reset refcount leak in the error path of
brcm_ahci_resume(). It adds a call to reset_control_rearm() to balance
the triggered_count when subsequent initialization steps fail after a
successful reset.

> When brcm_ahci_resume() succeeds with reset_control_reset(), any
> subsequent failure in ahci_platform_enable_clks(),
> ahci_platform_enable_regulators(), ahci_platform_enable_phys(),
> or ahci_platform_resume_host() leaves the shared reset line's
> triggered_count incremented by one.

[ ... ]

> Add a reset_control_rearm() call in the common error path after
> brcm_sata_phys_disable() and ahci_platform_disable_regulators()
> to properly balance the triggered_count, matching what the probe
> error path already does.

[Severity: High]
Does this patch actually resolve the leak for all the functions mentioned
in the commit message?

Looking at the code in brcm_ahci_resume(), if ahci_platform_enable_clks()
fails, it returns directly:

	ret = ahci_platform_enable_clks(hpriv);
	if (ret)
		return ret;

And if ahci_platform_enable_regulators() fails, it jumps to out_disable_clks:

	ret = ahci_platform_enable_regulators(hpriv);
	if (ret)
		goto out_disable_clks;

Both of these error paths bypass the newly added reset_control_rearm() call,
leaving the triggered_count leak unfixed for these conditions.

> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 29be74fedcf01..38c63d73d210b 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -415,6 +415,7 @@ static int __maybe_unused brcm_ahci_resume(struct device *dev)
>  out_disable_phys:
>  	brcm_sata_phys_disable(priv);
>  	ahci_platform_disable_regulators(hpriv);
> +	reset_control_rearm(priv->rcdev_rescal);
>  out_disable_clks:
>  	ahci_platform_disable_clks(hpriv);
>  	return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does brcm_ahci_resume() also
leak the assertion state of rcdev_ahci on error?

Earlier in brcm_ahci_resume(), the reset control is deasserted:

	ret = reset_control_deassert(priv->rcdev_ahci);
	if (ret)
		return ret;

However, the entire error handling block at the end of the function (shown in
the diff above) lacks a corresponding call to
reset_control_assert(priv->rcdev_ahci), which leaves the line permanently
deasserted if a failure occurs later in the resume process.

[Severity: High]
This is a pre-existing issue, but is there a similar missing rearm in
brcm_ahci_probe()?

If reset_control_deassert() fails in the probe function, it returns directly:

	ret = reset_control_reset(priv->rcdev_rescal);
	if (ret)
		return ret;
	ret = reset_control_deassert(priv->rcdev_ahci);
	if (ret)
		return ret;

This skips jumping to the out_reset label, leaking the previously incremented
triggered_count of rcdev_rescal. Could this be updated to properly jump to
the cleanup path?

[Severity: High]
This is a pre-existing issue, but does module removal permanently leak these
reset resources?

When the module is unloaded, brcm_ahci_remove() only disables the PHYs and
removes the platform device:

drivers/ata/ahci_brcm.c:brcm_ahci_remove()
{
	struct ata_host *host = dev_get_drvdata(&pdev->dev);
	struct ahci_host_priv *hpriv = host->private_data;
	struct brcm_ahci_priv *priv = hpriv->plat_data;

	brcm_sata_phys_disable(priv);

	ata_platform_remove_one(pdev);
}

Should this function also call reset_control_rearm() and
reset_control_assert() to balance the operations from probe? Without them,
the triggered_count leaks permanently because devm cleanup only calls
reset_control_put().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603102420.3735032-1-vulab@iscas.ac.cn?part=1

Re: [PATCH] ata: ahci_brcm: fix reset refcount leak in brcm_ahci_resume()

[Niklas Cassel]

On Wed, Jun 03, 2026 at 10:24:20AM +0000, Wentao Liang wrote:
> When brcm_ahci_resume() succeeds with reset_control_reset(), any
> subsequent failure in ahci_platform_enable_clks(),
> ahci_platform_enable_regulators(), ahci_platform_enable_phys(),
> or ahci_platform_resume_host() leaves the shared reset line's
> triggered_count incremented by one. On the next attempt to reset
> the hardware, atomic_inc_return() sees a count greater than one
> and the reset is silently skipped, potentially causing data
> corruption or device malfunction.
> 
> Add a reset_control_rearm() call in the common error path after
> brcm_sata_phys_disable() and ahci_platform_disable_regulators()
> to properly balance the triggered_count, matching what the probe
> error path already does.
> 
> Fixes: c0cdf2ac4b5b ("ata: ahci_brcm: Fix AHCI resources management")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/ata/ahci_brcm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 29be74fedcf0..38c63d73d210 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -415,6 +415,7 @@ static int __maybe_unused brcm_ahci_resume(struct device *dev)
>  out_disable_phys:
>  	brcm_sata_phys_disable(priv);
>  	ahci_platform_disable_regulators(hpriv);
> +	reset_control_rearm(priv->rcdev_rescal);
>  out_disable_clks:
>  	ahci_platform_disable_clks(hpriv);
>  	return ret;

When writing a fix that has a Fixes-tag, please CC the author of the offending
commit. Note that if you use ./scripts/get_maintainer.pn on a a patch, it will
automatically include the author of the offending commit in the list of people
to CC.


Now you are adding reset_control_rearm() before disabling clocks to the error
handling in brcm_ahci_resume(), however in the error handing in
brcm_ahci_probe(), we call reset_control_rearm() after disabling clocks.
Why should the error handling in brcm_ahci_resume() not match that of
brcm_ahci_probe() ?


I am confused as to why the error handling in brcm_ahci_probe() is calling both
reset_control_rearm() and reset_control_reset().

The documentation for reset_control, explicitly says not to do this:
https://github.com/torvalds/linux/blob/v7.1-rc6/drivers/reset/core.c#L365-L366

And in libahci_platform.c, we always do either:
return reset_control_rearm() or return reset_control_reset():
https://github.com/torvalds/linux/blob/v7.1-rc6/drivers/ata/libahci_platform.c#L188-L193

Too bad that this driver is not using the generic AHCI functions in
libahci_platform.c.


Kind regards,
Niklas

Re: [PATCH] ata: ahci_brcm: fix reset refcount leak in brcm_ahci_resume()

[Niklas Cassel]

On Thu, Jun 04, 2026 at 12:03:53PM +0200, Niklas Cassel wrote:

(snip)

> The documentation for reset_control, explicitly says not to do this:
> https://github.com/torvalds/linux/blob/v7.1-rc6/drivers/reset/core.c#L365-L366
> 
> And in libahci_platform.c, we always do either:
> return reset_control_rearm() or return reset_control_reset():
> https://github.com/torvalds/linux/blob/v7.1-rc6/drivers/ata/libahci_platform.c#L188-L193

I realize that I am stupid...

The code is doing it on two different reset handles.

My comment that the cleanup in brcm_ahci_resume() should match that in
brcm_ahci_resume() probe still stands.

i.e. you should also add a call to:
reset_control_assert(priv->rcdev_ahci);

And you should do it after disabling clocks, just as brcm_ahci_resume().


Kind regards,
Niklas