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

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

[Wentao Liang]

When reset_control_deassert() fails in brcm_ahci_probe(), the
function returns without calling reset_control_rearm() on the
previously asserted shared reset control. This leaves the
triggered count incremented, leaking the reset control reference.

All other error paths after the reset_control_reset() call properly
reach the out_reset label which performs the rearm. Rework the
deassert error path to go through out_reset to restore the
triggered count and ensure the reference is released properly.

Cc: stable@vger.kernel.org
Fixes: 1a0600d112e3 ("ata: ahci_brcm: Perform reset after obtaining resources")
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/ata/ahci_brcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 38c63d73d210..b58343f027cf 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -492,7 +492,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 		return ret;
 	ret = reset_control_deassert(priv->rcdev_ahci);
 	if (ret)
-		return ret;
+		goto out_reset;
 
 	ret = ahci_platform_enable_clks(hpriv);
 	if (ret)
-- 
2.34.1

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing shared reset control cleanup in brcm_ahci_remove() permanently leaks the triggered state.
- [High] Missing shared reset control cleanup in the error paths of brcm_ahci_resume() leads to a state leak.
- [High] Unconditional reset assertion in brcm_ahci_suspend() on failure paths leaves the device in an unrecoverable broken state.
--

commit cb11553edeb17c9d408e236defe15a4cb5e6ff9e
Author: Wentao Liang <vulab@iscas.ac.cn>

ata: ahci_brcm: fix refcount leak in brcm_ahci_probe()

This commit fixes a refcount leak by properly routing the error path of
reset_control_deassert() to the out_reset label. This ensures the shared
reset control is correctly rearmed in brcm_ahci_probe().

> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 29be74fedcf01..e759e99b54be3 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -491,7 +491,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>  		return ret;
>  	ret = reset_control_deassert(priv->rcdev_ahci);
>  	if (ret)
> -		return ret;
> +		goto out_reset;
>  
>  	ret = ahci_platform_enable_clks(hpriv);
>  	if (ret)

While reviewing this fix, I noticed a few other related pre-existing issues
in this driver.

[Severity: High]
This is a pre-existing issue, but missing shared reset control cleanup in
brcm_ahci_remove() appears to permanently leak the triggered state:

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

Does this function fail to symmetrically rearm the shared reset control
acquired during probe?

If any other driver on the SoC holds a reference to this shared reset line,
wouldn't the triggered count be permanently leaked and never drop back to
zero?

Subsequent bind attempts or other drivers might then silently skip issuing
the hardware reset pulse.

[Severity: High]
This is also a pre-existing issue, but I have a question about the error
handling in brcm_ahci_resume():

drivers/ata/ahci_brcm.c:brcm_ahci_resume() {
    ...
    ret = reset_control_reset(priv->rcdev_rescal);
    if (ret)
        return ret;

    /* Make sure clocks are turned on before re-configuration */
    ret = ahci_platform_enable_clks(hpriv);
    if (ret)
        return ret;

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

Do the error paths after reset_control_reset() return from the resume
sequence without properly rearming the shared reset control?

Like the issue addressed in brcm_ahci_probe(), could this cause the exact
same triggered count leak on the shared reset control, breaking the reset
functionality on subsequent resumes?

[Severity: High]
Finally, this is a pre-existing issue, but looking at brcm_ahci_suspend():

drivers/ata/ahci_brcm.c:brcm_ahci_suspend() {
    ...
    if (IS_ENABLED(CONFIG_PM_SLEEP))
        ret = ahci_platform_suspend(dev);
    else
        ret = 0;

    reset_control_assert(priv->rcdev_ahci);
    reset_control_rearm(priv->rcdev_rescal);

    return ret;
}

Are the reset controls unconditionally asserted and rearmed even when
ahci_platform_suspend() returns an error?

If the system suspend sequence is aborted (and resume won't be called),
does this forcefully leave the controller in reset while the rest of the
kernel expects it to be active?

Could this lead to I/O hangs if the root filesystem is on the affected drive?

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

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

[Niklas Cassel]

On Wed, Jun 03, 2026 at 10:30:08AM +0000, Wentao Liang wrote:
> When reset_control_deassert() fails in brcm_ahci_probe(), the
> function returns without calling reset_control_rearm() on the
> previously asserted shared reset control. This leaves the
> triggered count incremented, leaking the reset control reference.
> 
> All other error paths after the reset_control_reset() call properly
> reach the out_reset label which performs the rearm. Rework the
> deassert error path to go through out_reset to restore the
> triggered count and ensure the reference is released properly.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1a0600d112e3 ("ata: ahci_brcm: Perform reset after obtaining resources")
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/ata/ahci_brcm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 38c63d73d210..b58343f027cf 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -492,7 +492,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>  		return ret;
>  	ret = reset_control_deassert(priv->rcdev_ahci);
>  	if (ret)
> -		return ret;
> +		goto out_reset;
>  
>  	ret = ahci_platform_enable_clks(hpriv);
>  	if (ret)
> -- 
> 2.34.1
> 

The code in brcm_ahci_probe():

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

        ret = ahci_platform_enable_clks(hpriv);
        if (ret)


The code in brcm_ahci_resume():

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

        ret = ahci_platform_enable_clks(hpriv);
        if (ret)


This makes no sense.

The order of the resets should be the same in both functions, since it is
different reset handles.


It is probably easier to add a new jump label, so that there is one jump
label per reset handle.

That way you can jump to the correct jump label, so that you only perform
the opposite of the reset that was actually successful.


Kind regards,
Nilklas