Re: [PATCH v2 2/8] drm/sched: Disallow initializing entities with un-initialized schedulers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/sched: drm_sched_entity_init() breaks v3d_open() on V3D 3.3
--

commit 607520d70e20fa56f0a8fa9eade2db9f019b764f
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

drm/sched: Disallow initializing entities with un-initialized schedulers

This patch updates drm_sched_entity_init() to reject schedulers whose
run-queues have not been initialized, returning -EINVAL instead of logging
a warning and continuing.

> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index e12709b7bd0c3..95bb79b3d003a 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -119,9 +119,16 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>  			  unsigned int num_sched_list,
>  			  atomic_t *guilty)
>  {
> -	if (!entity || !sched_list || !num_sched_list || !sched_list[0])
> +	unsigned int i;
> +
> +	if (!entity || !sched_list || !num_sched_list)
>  		return -EINVAL;
>  
> +	for (i = 0; i < num_sched_list; i++) {
> +		if (!sched_list[i] || !sched_list[i]->sched_rq)
> +			return -EINVAL;
> +	}

[Severity: High]
Does returning -EINVAL here when !sched_list[i]->sched_rq break
v3d_open() on V3D 3.3 hardware (V3D_GEN_33)?

During probe, v3d_sched_init() in drivers/gpu/drm/v3d/v3d_sched.c only
initializes the V3D_CSD and V3D_CACHE_CLEAN schedulers when
v3d_has_csd(v3d) is true:

drivers/gpu/drm/v3d/v3d_sched.c:v3d_sched_init() {
	...
	if (v3d_has_csd(v3d)) {
		ret = v3d_queue_sched_init(v3d, &v3d_csd_sched_ops, V3D_CSD,
					   "v3d_csd");
		if (ret)
			goto fail;

		ret = v3d_queue_sched_init(v3d, &v3d_cache_clean_sched_ops,
					   V3D_CACHE_CLEAN, "v3d_cache_clean");
		if (ret)
			goto fail;
	}
	...
}

On V3D 3.3 platforms where !v3d_has_csd(v3d), those two schedulers are
never initialized via drm_sched_init(), leaving their sched_rq pointers
NULL.

However, when userspace opens the DRM device node, v3d_open() in
drivers/gpu/drm/v3d/v3d_drv.c unconditionally loops over all
V3D_MAX_QUEUES queues and fails if drm_sched_entity_init() returns an
error:

drivers/gpu/drm/v3d/v3d_drv.c:v3d_open() {
	...
	for (i = 0; i < V3D_MAX_QUEUES; i++) {
		v3d_priv->stats[i] = v3d_stats_alloc();
		if (!v3d_priv->stats[i]) {
			ret = -ENOMEM;
			goto err_stats;
		}

		sched = &v3d->queue[i].sched;
		ret = drm_sched_entity_init(&v3d_priv->sched_entity[i],
					    DRM_SCHED_PRIORITY_NORMAL, &sched,
					    1, NULL);
		if (ret)
			goto err_sched;
	}
	...
}

Previously, drm_sched_entity_init() logged a warning via pr_warn() and
returned 0, allowing v3d_open() to succeed. With this change, won't
every v3d_open() call on V3D 3.3 fail with -EINVAL?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020154123.33631-1-tvrtko.ursulin@igalia.com?part=2