Re: [RFC PATCH 4/6] media: rockchip: rkvenc: Add frequency scaling and cooling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [High] media: rockchip: rkvenc: rkvenc_dvfs_init() leaves initial OPP
  and venc-supply unconfigured
- [High] media: rkvenc: UAF and teardown race in rkvenc_remove() and
  rkvenc_dvfs_exit()
- [Medium] media: rockchip: rkvenc: probe fails in
  rkvenc_dvfs_cooling_init() without THERMAL_OF
- [Medium] media: rockchip: rkvenc: thermal duty-cycle throttling
  bypassed in rkvenc_dvfs_start_allowed()
- [Medium] media: rockchip: rkvenc: devfreq updates stall in
  rkvenc_dvfs_job_end() when round_ctx idles
- [Medium] media: rockchip: rkvenc: unreset last_eval in
  rkvenc_dvfs_job_end() skews first round load
- [Low] media: rockchip: rkvenc: freq / HZ_PER_KHZ truncation in
  rkvenc_cdev_set_state()
--

commit f718f2271f482c079204fa578dffff0091dcfc44
Author: Daniel Golle <daniel@makrotopia.org>

media: rockchip: rkvenc: Add frequency scaling and cooling

This commit adds devfreq-based frequency scaling for the RKVENC core clock
andvenc-supply regulator, along with a thermal cooling device that caps the
maximum frequency via PM QoS and throttles job execution duty cycle.

> diff --git a/drivers/media/platform/rockchip/rkvenc/rkvenc-dvfs.c b/drivers/media/platform/rockchip/rkvenc/rkvenc-dvfs.c
> new file mode 100644
> index 0000000000000..8b59122d5d983
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rkvenc/rkvenc-dvfs.c
[ ... ]
> @@ -0,0 +1,364 @@
[ ... ]
> +bool rkvenc_dvfs_start_allowed(struct rkvenc_dev *rkvenc)
> +{
> +	struct rkvenc_dvfs *dvfs = &rkvenc->dvfs;
> +	unsigned long flags, delay;
> +	s64 delay_us;
> +	bool allowed;
> +
> +	spin_lock_irqsave(&dvfs->lock, flags);
> +	delay_us = ktime_to_us(ktime_sub(dvfs->next_start, ktime_get()));
> +	allowed = dvfs->duty_pct >= 100 || delay_us <= 0;
> +	delay = usecs_to_jiffies(clamp_t(s64, delay_us, 0, UINT_MAX));
> +	if (!allowed)
> +		queue_delayed_work(system_wq, &dvfs->throttle_work, delay);
> +	spin_unlock_irqrestore(&dvfs->lock, flags);
> +
> +	return allowed;
> +}

[Severity: Medium]
Can multiple contexts bypass duty-cycle throttling when dvfs->duty_pct < 100?

v4l2_m2m only checks .job_ready() in __v4l2_m2m_try_queue() before placing a
context onto m2m_dev->job_queue; once queued, v4l2_m2m_try_run() pops and
runs contexts via .device_run() without calling .job_ready() again. Because
dvfs->next_start is only updated when a job finishes in
rkvenc_dvfs_job_end() and rkvenc_dvfs_start_allowed() does not check
dvfs->busy, every context scheduled by rkvenc_dvfs_throttle_work() (or via
VIDIOC_QBUF while the first job is running) sees delay_us <= 0 and enters
m2m_dev->job_queue to run back-to-back without throttling.

In addition, when rkvenc_finish_job() retries a frame on bitstream overflow,
it calls rkvenc_dvfs_job_end() followed directly by
rkvenc_m2m_ops.device_run(ctx), bypassing rkvenc_dvfs_start_allowed()
completely.

> +
> +void rkvenc_dvfs_job_end(struct rkvenc_dev *rkvenc, struct rkvenc_ctx *ctx)
> +{
> +	struct rkvenc_dvfs *dvfs = &rkvenc->dvfs;
> +	struct devfreq *devfreq = dvfs->devfreq;
> +	unsigned long flags;
> +	bool evaluate;
> +	ktime_t now;
> +
> +	spin_lock_irqsave(&dvfs->lock, flags);
> +	now = ktime_get();
> +	dvfs->busy_total =
> +		ktime_add(dvfs->busy_total, ktime_sub(now, dvfs->busy_since));
> +	dvfs->next_start = ktime_add_ns(
> +		dvfs->job_start,
> +		div_u64(ktime_to_ns(ktime_sub(now, dvfs->job_start)) * 100,
> +			dvfs->duty_pct));
> +	dvfs->busy = false;
> +	/* A round ends with the context that opened it, covering every other one */
> +	evaluate = dvfs->round_ctx == ctx;
> +	if (!dvfs->round_ctx)
> +		dvfs->round_ctx = ctx;

[Severity: Medium]
Does opening a new evaluation round when dvfs->round_ctx is NULL leave a
stale dvfs->last_eval from before the encoder was idle?

dvfs->last_eval is only initialized in rkvenc_dvfs_init() and updated in
rkvenc_dvfs_status(). When the first job after boot or after VIDIOC_STREAMOFF
sets dvfs->round_ctx = ctx, dvfs->last_eval and dvfs->busy_total are not
reset, so on the next job completion rkvenc_dvfs_status() computes
stat->total_time across the entire preceding idle period and drops the clock
to the lowest OPP.

> +	spin_unlock_irqrestore(&dvfs->lock, flags);
> +	wake_up(&dvfs->idle_wq);
> +
> +	if (!devfreq || !evaluate)
> +		return;
> +
> +	mutex_lock(&devfreq->lock);
> +	update_devfreq(devfreq);
> +	mutex_unlock(&devfreq->lock);
> +}

[Severity: Medium]
What happens to frequency scaling if the context stored in dvfs->round_ctx
stops submitting jobs without calling VIDIOC_STREAMOFF or closing the fd?

dvfs->round_ctx is only cleared in rkvenc_dvfs_ctx_idle() from
rkvenc_stop_streaming() and rkvenc_release(). If that context finishes
draining via v4l2_m2m_mark_stopped() or hits vb2_queue_error() in
rkvenc_finish_job() (which then calls rkvenc_dvfs_job_end() and keeps or
assigns dvfs->round_ctx = ctx), or if userspace simply pauses queueing
frames on that context while other contexts keep streaming, evaluate will
never be true again and update_devfreq() stops running.

[ ... ]

> +static int rkvenc_cdev_set_state(struct thermal_cooling_device *cdev,
> +				 unsigned long state)
> +{
> +	struct rkvenc_dvfs *dvfs = cdev->devdata;
> +	unsigned long idx = min_t(unsigned long, state, dvfs->num_freqs - 1);
> +	unsigned long freq = dvfs->freqs[dvfs->num_freqs - 1 - idx];
> +	unsigned long flags;
> +	int ret;
> +
> +	ret = dev_pm_qos_update_request(&dvfs->max_freq_req, freq / HZ_PER_KHZ);
> +	if (ret < 0)
> +		return ret;
> +
> +	spin_lock_irqsave(&dvfs->lock, flags);
> +	dvfs->duty_pct = 100 - RKVENC_THROTTLE_STEP_PCT * (state - idx);
> +	spin_unlock_irqrestore(&dvfs->lock, flags);
> +	dvfs->cooling_state = state;
> +
> +	return 0;
> +}

[Severity: Low]
Should freq / HZ_PER_KHZ use DIV_ROUND_UP(freq, HZ_PER_KHZ) here?

devfreq converts the PM QoS maximum frequency back to Hz via
HZ_PER_KHZ * qos_max_freq and looks up the target OPP with
DEVFREQ_FLAG_LEAST_UPPER_BOUND (floor), so truncating an OPP frequency that
is not an exact multiple of 1000 Hz drops the limit below that OPP and
selects the next lower one. Also, state is not bounds-checked against
dvfs->num_freqs - 1 + RKVENC_THROTTLE_STEPS before computing dvfs->duty_pct.

[ ... ]

> +static int rkvenc_dvfs_cooling_init(struct rkvenc_dev *rkvenc)
> +{
> +	struct rkvenc_dvfs *dvfs = &rkvenc->dvfs;
> +	struct device *dev = rkvenc->dev;
> +	int ret;
> +
> +	if (!of_property_present(dev->of_node, "#cooling-cells"))
> +		return 0;
> +
> +	ret = dev_pm_qos_add_request(dev, &dvfs->max_freq_req,
> +				     DEV_PM_QOS_MAX_FREQUENCY,
> +				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> +	if (ret < 0)
> +		return ret;
> +	dvfs->qos_added = true;
> +
> +	dvfs->cdev = thermal_of_cooling_device_register(
> +		dev->of_node, 0, "rkvenc", dvfs, &rkvenc_cdev_ops);
> +	if (IS_ERR(dvfs->cdev)) {
> +		ret = PTR_ERR(dvfs->cdev);
> +		dvfs->cdev = NULL;
> +		return dev_err_probe(dev, ret,
> +				     "could not add the cooling device\n");
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does rkvenc_probe() fail with -ENODEV when CONFIG_THERMAL_OF is disabled?

VIDEO_ROCKCHIP_RKVENC does not depend on or select THERMAL or THERMAL_OF in
Kconfig. When CONFIG_THERMAL_OF is disabled, the
thermal_of_cooling_device_register() stub returns ERR_PTR(-ENODEV), which
aborts probe on any board whose device tree node has #cooling-cells.

> +
> +int rkvenc_dvfs_init(struct rkvenc_dev *rkvenc)
> +{
[ ... ]
> +	if (of_property_present(dev->of_node, "venc-supply"))
> +		opp_config.regulator_names = supply_names;
> +
> +	ret = devm_pm_opp_set_config(dev, &opp_config);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = rkvenc_dvfs_freqs_get(rkvenc);
> +	if (ret)
> +		return ret;
> +
> +	dvfs->profile.target = rkvenc_dvfs_target;
> +	dvfs->profile.get_dev_status = rkvenc_dvfs_status;
> +	dvfs->profile.get_cur_freq = rkvenc_dvfs_cur_freq;
> +	dvfs->profile.freq_table = dvfs->freqs;
> +	dvfs->profile.max_state = dvfs->num_freqs;
> +	rkvenc_dvfs_cur_freq(dev, &dvfs->profile.initial_freq);
> +
> +	devfreq = devfreq_add_device(dev, &dvfs->profile,
> +				     DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);

[Severity: High]
Does anything enable and configure venc-supply or set an initial OPP before
the first round of encoding jobs completes?

devm_pm_opp_set_config() only acquires venc-supply via
regulator_get_optional(); the OPP core does not enable the regulator or
configure its voltage until dev_pm_opp_set_rate() or dev_pm_opp_set_opp() is
called. Because polling is disabled, devfreq_add_device() does not invoke
rkvenc_dvfs_target() at registration time, so RKVENC_VERSION is read in
rkvenc_probe() and the first round of jobs runs on the hardware before
dev_pm_opp_set_rate() is ever called (and regulator_init_complete() can
disable a boot-on venc-supply as unused). In addition, setting
dvfs->profile.initial_freq directly from clk_get_rate() without rounding it
to an OPP entry causes devfreq_update_status() to fail with -EINVAL on the
first transition if the boot clock rate does not exactly match an OPP.

> diff --git a/drivers/media/platform/rockchip/rkvenc/rkvenc.c b/drivers/media/platform/rockchip/rkvenc/rkvenc.c
> index 00778e97686d7..f15a0922059f1 100644
> --- a/drivers/media/platform/rockchip/rkvenc/rkvenc.c
> +++ b/drivers/media/platform/rockchip/rkvenc/rkvenc.c
[ ... ]
> @@ -442,6 +450,7 @@ static void rkvenc_remove(struct platform_device *pdev)
>  	struct rkvenc_dev *rkvenc = platform_get_drvdata(pdev);
>  
>  	cancel_delayed_work_sync(&rkvenc->watchdog);
> +	rkvenc_dvfs_exit(rkvenc);
>  	rkvenc_v4l2_cleanup(rkvenc);
>  	v4l2_m2m_release(rkvenc->m2m_dev);
>  	v4l2_device_unregister(&rkvenc->v4l2_dev);

[Severity: High]
Can calling rkvenc_dvfs_exit() before rkvenc_v4l2_cleanup() and
v4l2_m2m_release() cause a use-after-free during driver teardown?

While V4L2 contexts and an in-flight job are still active, rkvenc_dvfs_exit()
frees dvfs->devfreq before clearing the pointer and cancels
dvfs->throttle_work while dvfs->duty_pct can still be below 100:

CPU0                                  CPU1
rkvenc_remove()
  rkvenc_dvfs_exit()
    devfreq_remove_device(dvfs->devfreq)
                                      rkvenc_irq_thread()
                                        rkvenc_finish_job()
                                          rkvenc_dvfs_job_end()
                                            mutex_lock(&devfreq->lock)
                                              <-- use-after-free
    cancel_delayed_work_sync(&dvfs->throttle_work)
                                          v4l2_m2m_buf_done_and_job_finish()
                                            ...
                                              rkvenc_dvfs_start_allowed()
                                                queue_delayed_work(...,
                                                  &dvfs->throttle_work, ...)
  ...
  (devm frees rkvenc)
                                      rkvenc_dvfs_throttle_work()
                                        <-- use-after-free on rkvenc

Also, in rkvenc_dvfs_exit(), calling dev_pm_qos_remove_request() before
devfreq_remove_device() fires devfreq's max-frequency QoS notifier and runs
update_devfreq() -> rkvenc_dvfs_target() during teardown right before
devfreq is removed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794666330.git.daniel@makrotopia.org?part=4