Re: [PATCH] clk: imx: Add audio PLL debugfs for K-divider control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unchecked ERR_PTR dereference in `audio_pll_debug_init()` causes kernel panic if PLL registration fails.
- [High] Lockless read-modify-write of PLL registers causes a race condition with the Common Clock Framework.
- [Medium] Missing bounds checking allows the K-divider to wrap around, causing massive frequency jumps.
--

commit 654083d958b35cc8cdcae8664a30b757df4a7f7a
Author: Jacky Bai <ping.bai@nxp.com>

clk: imx: Add audio PLL debugfs for K-divider control

This commit adds debugfs support for runtime tuning of the audio PLL K
divider on i.MX platforms. It exposes delta_k and pll_parameter interfaces
to adjust and monitor the fractional dividers.

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index 39600ee22be30..59f126f354741 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c

[ ... ]

> @@ -551,3 +552,81 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,

[ ... ]

> +static int pll_delta_k_set(void *data, u64 val)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(data);
> +	s16 delta_k = (s16)val;
> +	u32 div_ctl1;
> +	s16 kdiv;
> +
> +	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> +	kdiv = FIELD_GET(KDIV_MASK, div_ctl1) + delta_k;

If a user provides a large positive delta_k that pushes kdiv beyond 32767,
will this s16 addition wrap around to a large negative number? Does this
need a clamp_val() check to prevent abrupt frequency discontinuities?

> +	writel_relaxed(FIELD_PREP(KDIV_MASK, kdiv), pll->base + DIV_CTL1);

Because this read-modify-write sequence is performed without acquiring the
CCF prepare lock or a hardware spinlock, can it race with a concurrent
clk_set_rate() operation?

If CCF is updating DIV_CTL1 at the same time, could this overwrite the
CCF's register changes and desynchronize the hardware state?

> +
> +	return 0;
> +}

[ ... ]

> +void audio_pll_debug_init(struct clk_hw *hws[], unsigned int num_plls)
> +{
> +	struct dentry *rootdir, *audio_pll_dir;
> +	const char *pll_name;
> +	int i;
> +
> +	rootdir = debugfs_create_dir("audio_pll_monitor", NULL);
> +
> +	for (i = 0; i < num_plls; i++) {
> +		pll_name = clk_hw_get_name(hws[i]);

In imx8mm_clocks_probe(), if the PLL fails to register, the hws array might
contain an error pointer. When clk_hw_get_name() is called here, it directly
dereferences hw->core:

clk_hw_get_name(const struct clk_hw *hw)
{
	return hw->core->name;
}

Could this code cause a kernel panic by dereferencing an error pointer if
the PLL registration failed earlier?

> +		audio_pll_dir = debugfs_create_dir(pll_name, rootdir);
> +		debugfs_create_file_unsafe("delta_k", 0200, audio_pll_dir,
> +				hws[i], &delta_k_fops);
> +		debugfs_create_file("pll_parameter", 0444, audio_pll_dir,
> +				hws[i], &pll_setting_fops);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-imx8m_pll_debugfs-v1-1-e1e44b21be90@nxp.com?part=1