Re: [PATCH] clk: zynq: return -ETIMEDOUT if the PLL never locks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing hardware state rollback on PLL lock timeout.
--

commit 91f1719daf95abfb29082ef831a0b53acd17a349
Author: Linkai Gong <gonglinkai@kylinos.cn>

clk: zynq: return -ETIMEDOUT if the PLL never locks

zynq_pll_enable() waits for lock under a spinlock with no timeout. Poll
with a 1ms bound and return the error.

> diff --git a/drivers/clk/zynq/pll.c b/drivers/clk/zynq/pll.c
> index fe90b50e1545d..4e3a1cb40be74 100644
> --- a/drivers/clk/zynq/pll.c
> +++ b/drivers/clk/zynq/pll.c
[ ... ]
> @@ -131,12 +133,12 @@ static int zynq_pll_enable(struct clk_hw *hw)
>  	reg = readl(clk->pll_ctrl);
>  	reg &= ~(PLLCTRL_RESET_MASK | PLLCTRL_PWRDWN_MASK);
>  	writel(reg, clk->pll_ctrl);
> -	while (!(readl(clk->pll_status) & (1 << clk->lockbit)))
> -		;
> +	ret = readl_poll_timeout_atomic(clk->pll_status, reg,
> +					reg & (1 << clk->lockbit), 10, 1000);
>  
>  	spin_unlock_irqrestore(clk->lock, flags);
>  
> -	return 0;
> +	return ret;

[Severity: High]
Does this leave the hardware state desynchronized if the PLL fails to lock?

If readl_poll_timeout_atomic() returns -ETIMEDOUT, the function returns the
error directly without restoring the PLLCTRL_RESET_MASK and
PLLCTRL_PWRDWN_MASK bits in the control register.

Because zynq_pll_is_enabled() checks those same bits to determine if the
PLL is enabled:

drivers/clk/zynq/pll.c:zynq_pll_is_enabled() {
    ...
	reg = readl(clk->pll_ctrl);
    ...
	return !(reg & (PLLCTRL_RESET_MASK | PLLCTRL_PWRDWN_MASK));
}

A subsequent call to zynq_pll_enable() will falsely succeed since
zynq_pll_is_enabled() will return true, hiding the lock failure. Should we
re-assert the reset and power-down bits in the error path before returning?

>  }
>

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