Nouveau - Feb 2024 - [PATCH] drm/nouveau: nouveau_sched_fini() should check for init failure

On 1/31/24 22:39, Timur Tabi wrote:
> If initialization fails, Nouveau can still call the _fini() function
> to clean up, with the expectation that the function can handle if its
> corresponding _init() function was never called or exited with error.
> 
> Such is not the case with nouveau_sched_fini(), which still attempts
> to wait for jobs to finish even if the underlying data structures were
> never initialized.
Good catch!
> 
> Fixes: 5f03a507b29e ("drm/nouveau: implement 1:1 scheduler - entity
relationship")
> Signed-off-by: Timur Tabi <ttabi at nvidia.com>
> ---
>   drivers/gpu/drm/nouveau/nouveau_sched.c | 5 +++++
>   drivers/gpu/drm/nouveau/nouveau_sched.h | 2 ++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c
b/drivers/gpu/drm/nouveau/nouveau_sched.c
> index dd98f6910f9c..9c771bc0e332 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sched.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
> @@ -443,6 +443,8 @@ nouveau_sched_init(struct nouveau_sched *sched, struct
nouveau_drm *drm,
>   	INIT_LIST_HEAD(&sched->job.list.head);
>   	init_waitqueue_head(&sched->job.wq);
>   
> +	sched->initialized = true;
I wonder if we should allocate struct nouveau_sched dynamically instead and just
check for NULL in the corresponding *_fini() functions.

Actually, in nouveau_abi16_ioctl_channel_alloc() we can omit creating a
scheduler
instance entirely if !nouveau_cli_uvmm().

- Danilo
> +
>   	return 0;
>   
>   fail_sched:
> @@ -459,6 +461,9 @@ nouveau_sched_fini(struct nouveau_sched *sched)
>   	struct drm_gpu_scheduler *drm_sched = &sched->base;
>   	struct drm_sched_entity *entity = &sched->entity;
>   
> +	if (!sched->initialized)
> +		return;
> +
>   	rmb(); /* for list_empty to work without lock */
>   	wait_event(sched->job.wq, list_empty(&sched->job.list.head));
>   
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.h
b/drivers/gpu/drm/nouveau/nouveau_sched.h
> index a6528f5981e6..351931c706aa 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sched.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_sched.h
> @@ -109,6 +109,8 @@ struct nouveau_sched {
>   		} list;
>   		struct wait_queue_head wq;
>   	} job;
> +
> +	bool initialized;
>   };
>   
>   int nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm
*drm,

On Thu, 2024-02-01 at 13:58 +0100, Danilo Krummrich
wrote:
> I wonder if we should allocate struct nouveau_sched dynamically instead and
> just
> check for NULL in the corresponding *_fini() functions.
> 
> Actually, in nouveau_abi16_ioctl_channel_alloc() we can omit creating a
> scheduler
> instance entirely if !nouveau_cli_uvmm().
I am all for a better solution if you can think of one.  I don't really like
the idea of "bool initialized", but I couldn't think of a more
reliable test.