Re: [PATCH v6 3/4] phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing `dev_err_probe()` when propagating `-EPROBE_DEFER` causes kernel log spam.

Pre-existing issues:
- [High] Clock refcount leak on power-down timeout in `ti_pipe3_exit()`.
- [High] Unreliable jiffies polling loop in DPLL wait lock logic may falsely fail due to preemption.
--

commit 89eeca270e66e8da376c4a66e5ffebb70d29ee84
Author: Hongling Zeng <zenghongling@kylinos.cn>

phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

This commit updates the clock retrieval logic in ti_pipe3_get_clk() to
properly propagate -EPROBE_DEFER and other error codes. It fixes an issue
where devm_clk_get() failures for sysclk were masked as -EINVAL, and updates
SATA refclk to use devm_clk_get_optional() so that deferrals are not ignored.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
[ ... ]
> @@ -608,14 +608,20 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
>  	/*
>  	 * refclk is optional for SATA PHY to support older DTBs, but
>  	 * required for other modes. Use devm_clk_get_optional() for SATA
>  	 * which returns NULL for -ENOENT, allowing us to propagate all
>  	 * other errors including -EPROBE_DEFER.
>  	 */
>  	if (phy->mode == PIPE3_MODE_SATA)
>  		phy->refclk = devm_clk_get_optional(dev, "refclk");
>  	else
>  		phy->refclk = devm_clk_get(dev, "refclk");
> +
>  	if (IS_ERR(phy->refclk)) {
>  		dev_err(dev, "unable to get refclk\n");
> -		/* older DTBs have missing refclk in SATA PHY
> -		 * so don't bail out in case of SATA PHY.
> -		 */
> -		if (phy->mode != PIPE3_MODE_SATA)
> -			return PTR_ERR(phy->refclk);
> +		return PTR_ERR(phy->refclk);
>  	}

[Severity: Medium]
Since PTR_ERR() can now return -EPROBE_DEFER, will this dev_err() call spam
the kernel log on every probe deferral?

Should this be updated to use dev_err_probe() instead, which gracefully
handles the deferral and suppresses the spam?

[ ... ]
> @@ -632,7 +638,7 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
>  		phy->sys_clk = devm_clk_get(dev, "sysclk");
>  		if (IS_ERR(phy->sys_clk)) {
>  			dev_err(dev, "unable to get sysclk\n");
> -			return -EINVAL;
> +			return PTR_ERR(phy->sys_clk);
>  		}
>  	}

[Severity: Medium]
Similarly, does this dev_err() need to be replaced with dev_err_probe() to
prevent log spam when sysclk defers?


While reviewing this driver, I also noticed a couple of related items:

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_exit() {
    ...
    if (!(val & PLL_TICOPWDN) || !(val & PLL_LDOPWDN)) {
        dev_err(phy->dev, "Failed to power down: PLL_STATUS 0x%x\n",
                val);
        return -EBUSY;
    }
    ...
}

[Severity: High]
This is a pre-existing issue, but does this early return bypass the final
ti_pipe3_disable_clocks(phy) cleanup?

If the PLL LDO and Oscillator fail to power down within the timeout, it
appears the reference counts for wkupclk, refclk, and div_clk will leak.
If initialization is later retried, would the refcount increase
indefinitely, leaving the clocks permanently enabled?


drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_dpll_wait_lock() {
    ...
    do {
        cpu_relax();
        val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
        if (val & PLL_LOCK)
            return 0;
    } while (!time_after(jiffies, timeout));

    dev_err(phy->dev, "DPLL failed to lock\n");
    return -EBUSY;
}

[Severity: High]
This is also a pre-existing issue, but can this polling loop falsely fail
due to preemption?

If the thread is preempted or interrupted after reading PLL_STATUS but
before evaluating time_after() for a duration exceeding the timeout, it
looks like it will terminate and return -EBUSY even if the hardware
successfully locked during the preemption interval.

Could this be fixed by converting it to use readl_poll_timeout(), which
ensures a final check is made after the timeout condition?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619030214.1779043-1-zenghongling@kylinos.cn?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy