Re: [PATCH v7 7/9] media: chips-media: wave6: Add Wave6 thermal cooling device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Initial thermal state commands from the framework are silently discarded during registration due to a premature check on `thermal->cooling`.
- [Medium] The cooling device acts as a silent placebo if PM domain performance states are unsupported.
- [Medium] The driver incorrectly uses `dev_pm_genpd_set_performance_state()` with a raw frequency in kHz, violating the OPP/genpd abstraction and risking hardware malfunction on platforms with abstract performance levels.
- [Low] Defensive out-of-bounds check returns success (0) instead of an error, masking invalid requests and caching wrong states.
--

commit 99914a36008f0afa814ae979e8bbaf4616a8b747
Author: Nas Chung <nas.chung@chipsnmedia.com>

media: chips-media: wave6: Add Wave6 thermal cooling device

Add a thermal cooling device for the Wave6 VPU.
The device operates within the Linux thermal framework,
adjusting the VPU performance state based on thermal conditions.

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu-thermal.c b/drivers/media/platform/chips-media/wave6/wave6-vpu-thermal.c
> new file mode 100644
> index 0000000000000..1e67943b213bd
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu-thermal.c
> @@ -0,0 +1,143 @@

[ ... ]

> +static int wave6_vpu_thermal_cooling_update(struct vpu_thermal_cooling *thermal,
> +					    int state)
> +{
> +	unsigned long new_clock_rate;
> +	int ret;
> +
> +	if (state > thermal->thermal_max || !thermal->cooling)
> +		return 0;

[Severity: Low]
Does returning 0 for an invalid state request (state > thermal->thermal_max)
mask the error from the thermal core? It seems this would incorrectly allow
the caller wave6_vpu_cooling_set_cur_state() to cache this invalid state in
thermal->thermal_event.

[Severity: Medium]
Will the !thermal->cooling check incorrectly trigger during the initial
synchronous state assignment?

When devm_thermal_of_child_cooling_device_register() is called in
wave6_vpu_cooling_init(), it synchronously calls
wave6_cooling_ops.set_cur_state(). The registration function has not yet
returned to assign the pointer to thermal->cooling, so this check evaluates
to true. This would silently discard the initial cooling state set by the
thermal governor.

> +
> +	new_clock_rate = DIV_ROUND_UP(thermal->freq_table[state], HZ_PER_KHZ);
> +	dev_dbg(thermal->dev, "receive cooling state: %d, new clock rate %ld\n",
> +		state, new_clock_rate);
> +
> +	ret = dev_pm_genpd_set_performance_state(thermal->dev, new_clock_rate);

[Severity: Medium]
Is it correct to use dev_pm_genpd_set_performance_state() with a raw
frequency in kHz?

The dev_pm_genpd_set_performance_state() function expects a performance
state index (OPP level). Calculating
DIV_ROUND_UP(thermal->freq_table[state], HZ_PER_KHZ) instead of retrieving
the actual OPP level via dev_pm_opp_get_level() could violate the OPP/genpd
abstraction. On platforms where the performance domain uses abstract levels,
dividing the frequency by 1000 could result in an invalid or zero performance
state, breaking the cooling mitigation.

> +	if (ret && !((ret == -ENODEV) || (ret == -EOPNOTSUPP))) {

[Severity: Medium]
Does ignoring -ENODEV and -EOPNOTSUPP errors create a situation where the
cooling device acts as a silent placebo?

If the platform is missing genpd performance state support for the VPU,
these errors are caught and 0 (success) is returned. The thermal governor
will believe the device was throttled and will keep trying, but temperatures
will not drop since the device is not actually being cooled.

> +		dev_err(thermal->dev, "failed to set perf to %lu, ret = %d\n",
> +			new_clock_rate, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +int wave6_vpu_cooling_init(struct device *dev, struct vpu_thermal_cooling *thermal)
> +{

[ ... ]

> +	thermal->thermal_event = 0;
> +	thermal->cooling = devm_thermal_of_child_cooling_device_register(dev,
> +									 dev->of_node,
> +									 dev_name(thermal->dev),
> +									 thermal,
> +									 &wave6_cooling_ops);

[Severity: Medium]
As noted above in wave6_vpu_thermal_cooling_update(), since this is where
registration happens, thermal->cooling is only assigned after the framework
might have already attempted to set an initial cooling state. This causes
that initial command to be silently discarded.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788496816.git.nas.chung@chipsnmedia.com?part=7