Re: [PATCH v6 08/12] clk: nuvoton: ma35d1: Retrieve HXT/LXT from DT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: missing IS_ERR check in ma35d1_clk_gate_parent() causes CCF crash
--

commit 7dc0319906dcefbcabf738c0cfa86cd02109de8b
Author: Miquel Raynal <miquel.raynal@bootlin.com>

clk: nuvoton: ma35d1: Retrieve HXT/LXT from DT

HXT and LXT are crystal oscillator inputs of the clock controller, they
are described in the DT, so retrieve them, in order, and store them in
their respective HXT/LXT hw table entries.

> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
> index ceebcbd8c18b3..d955d79abdd2d 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1.c

[ ... ]

> @@ -191,6 +192,15 @@ static struct clk_hw *ma35d1_clk_gate(struct device *dev, const char *name, cons
>  				    reg, shift, 0, &ma35d1_lock);
>  }
>  
> +static struct clk_hw *ma35d1_clk_gate_parent(struct device *dev, const char *name,
> +					     struct clk_hw *parent,
> +					     void __iomem *reg, u8 shift)
> +{
> +	return devm_clk_hw_register_gate_parent_hw(dev, name, parent,
> +						   CLK_SET_RATE_PARENT,
> +						   reg, shift, 0, &ma35d1_lock);
> +}
> +

[Severity: High]
Does this wrapper need to check if the parent clock is valid before passing
it to the core clock framework?

Looking at ma35d1_clocks_probe() below, if the fallback clock creation via
ma35d1_clk_fixed() fails, it returns an ERR_PTR which is stored in hws[LXT].

[ ... ]

> @@ -240,12 +252,27 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	hws[HXT] = ma35d1_clk_fixed("hxt", 24000000);
> -	hws[HXT_GATE] = ma35d1_clk_gate(dev, "hxt_gate", "hxt",
> -					clk_base + REG_CLK_PWRCTL, 0);
> -	hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
> -	hws[LXT_GATE] = ma35d1_clk_gate(dev, "lxt_gate", "lxt",
> -					clk_base + REG_CLK_PWRCTL, 1);
> +	num_clks = devm_clk_bulk_get_all(dev, &clks);
> +	if (num_clks < 0)
> +		return num_clks;
> +
> +	if (!num_clks) {
> +		dev_err(dev, "missing crystal input clocks\n");
> +		return -ENODEV;
> +	}
> +
> +	hws[HXT] = __clk_get_hw(clks[0].clk);
> +
> +	if (num_clks > 1)
> +		hws[LXT] = __clk_get_hw(clks[1].clk);
> +	else
> +		/* Old DTs do not describe the low-speed crystal */
> +		hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
> +
> +	hws[HXT_GATE] = ma35d1_clk_gate_parent(dev, "hxt_gate", hws[HXT],
> +					       clk_base + REG_CLK_PWRCTL, 0);
> +	hws[LXT_GATE] = ma35d1_clk_gate_parent(dev, "lxt_gate", hws[LXT],
> +					       clk_base + REG_CLK_PWRCTL, 1);

When hws[LXT] containing an ERR_PTR is passed to ma35d1_clk_gate_parent(),
it gets sent directly to devm_clk_hw_register_gate_parent_hw().

Because the Common Clock Framework does not validate parent arrays for error
pointers, this will cause a kernel panic when the core later resolves the
parent map and dereferences the invalid memory.

Could we add an IS_ERR() check either around the fixed clock allocation
or inside ma35d1_clk_gate_parent()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-perso-ma35d1-upstream-clk-v6-0-48937ee6c9bb@bootlin.com?part=8