Re: [PATCH 2/2] thermal/drivers/amlogic: switch to clk_bulk API to support multiple clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] thermal/amlogic: kernel panic on unclocked register access if DT lacks clocks
--

commit ea5439a5f68769e10d58c93a949a283bf6984a5f
Author: Ronald Claveau <linux-kernel-dev@aliel.fr>

thermal/drivers/amlogic: switch to clk_bulk API to support multiple clocks

The t7-thermal variant now exposes two clocks (bus and core) in its
devicetree binding, but the driver only knows how to get and manage
a single struct clk. Using devm_clk_get() would silently ignore the
second clock, so switch to devm_clk_bulk_get_all() and the matching
clk_bulk_prepare_enable()/clk_bulk_disable_unprepare() calls.

> diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
> index a0b530624b60c..2e8d042067079 100644
> --- a/drivers/thermal/amlogic_thermal.c
> +++ b/drivers/thermal/amlogic_thermal.c
[ ... ]
> @@ -323,9 +324,10 @@ static int amlogic_thermal_probe(struct platform_device *pdev)
>  	if (IS_ERR(pdata->regmap))
>  		return PTR_ERR(pdata->regmap);
>  
> -	pdata->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(pdata->clk))
> -		return dev_err_probe(dev, PTR_ERR(pdata->clk), "failed to get clock\n");
> +	ret = devm_clk_bulk_get_all(dev, &pdata->clks);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> +	pdata->num_clks = ret;

[Severity: High]
Does this allow the probe to succeed without any clocks, leading to a crash
during register access if the device tree lacks clocks?

While device trees are typically trusted, missing required properties must
result in a graceful probe failure, not a system crash. The original
devm_clk_get() cleanly failed with -ENOENT if the clock was missing.

By changing this to devm_clk_bulk_get_all(), a missing clocks property now
returns 0. The driver sets pdata->num_clks = 0 and proceeds without error.

Later in probe, amlogic_thermal_enable() calls clk_bulk_prepare_enable()
(which becomes a no-op for 0 clocks) and then writes to TSENSOR_CFG_REG1.

Accessing hardware registers without first enabling the necessary bus or core
clocks usually results in a bus fault (synchronous external abort), causing a
kernel panic. Should there be an explicit check for ret == 0 to return an
error?

>  
>  	if (pdata->data->use_sm)
>  		ret = amlogic_thermal_probe_sm(pdev, pdata);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-fix-t7-thermal-clock-v1-0-7d4c4682f182@aliel.fr?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic