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

[sashiko-bot@kernel.org]

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