Re: [PATCH v2] 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:
- [Medium] Data race on `pll->delta_k` assignment outside lock.
- [Medium] Torn reads of PLL state in `pll_setting_show` due to missing locks.
- [Medium] Type truncation in `clamp_t` bypasses upper bounds on 32-bit platforms.
--

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

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

Add debugfs support for runtime tuning of the audio PLL K divider,
enabling fine-grained frequency adjustments for audio PLL. Two debug
interfaces are exported to userspace: delta_k and pll_parameter.

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index 39600ee22be30..b00529cb196a2 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
[ ... ]
> @@ -551,3 +566,95 @@ 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);
> +	unsigned long flags;
> +	u32 div_ctl1;
> +	s16 kdiv, delta_k;
> +
> +	delta_k = (s16)clamp_t(long, val, KDIV_MIN, KDIV_MAX);

Is it possible for this cast to long to cause truncation issues on 32-bit
platforms?

If a user writes a large positive value like 4294967295 (0xFFFFFFFF) to the
debugfs file, wouldn't it be truncated and sign-extended to -1?

Since -1 falls within the valid range, it seems this would bypass the max
bound check and apply an incorrect delta. Does this need to use s64 or long
long for the clamp type?

> +	pll->delta_k = delta_k;
> +
> +	spin_lock_irqsave(&pll->lock, flags);

Could this lockless assignment lead to state inconsistencies?

If two threads concurrently write to delta_k, could their execution interleave
such that both update pll->delta_k locklessly, but the lock is acquired
sequentially?

This might result in the hardware accumulating both changes while pll->delta_k
only reflects the last lockless assignment. Should the update of pll->delta_k
be moved inside the lock?

> +
> +	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> +	kdiv = (s16)FIELD_GET(KDIV_MASK, div_ctl1);
> +	kdiv = (s16)clamp_t(s32, (s32)kdiv + delta_k, KDIV_MIN, KDIV_MAX);
> +	writel_relaxed(FIELD_PREP(KDIV_MASK, kdiv), pll->base + DIV_CTL1);
> +
> +	spin_unlock_irqrestore(&pll->lock, flags);
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(delta_k_fops, pll_delta_k_get, pll_delta_k_set, "%lld\n");
> +
> +static int pll_setting_show(struct seq_file *s, void *data)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(s->private);
> +	u32 div_ctl0, div_ctl1;
> +	u32 mdiv, pdiv, sdiv, kdiv;
> +
> +	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> +	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);

Could this sequence result in torn reads if called concurrently with a rate
change?

Looking at clk_pll1443x_set_rate(), the hardware registers are updated
sequentially. DIV_CTL0 is written, followed by a locked write to DIV_CTL1:

    div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
    writel_relaxed(div_ctl0, pll->base + DIV_CTL0);

    spin_lock_irqsave(&pll->lock, flags);
    writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
    spin_unlock_irqrestore(&pll->lock, flags);

If pll_setting_show() runs concurrently without holding pll->lock, could it
interleave between those two writes, reading the updated dividers from
DIV_CTL0 but the stale K divider from DIV_CTL1?

> +
> +	mdiv = FIELD_GET(MDIV_MASK, div_ctl0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-imx8m_pll_debugfs-v2-1-20e1d88526b0@nxp.com?part=1