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

[sashiko-bot@kernel.org]

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