Re: [PATCH v15 06/10] drm/xe/migrate: Introduce xe_migrate_reinit() and xe_cpu_bind_reinit()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/xe/migrate: race between xe_migrate_flr_prepare() and
  xe_migrate_ulls_enter()
--

commit 7621242dd72f22e276d3072cbfb172bd04d6eb8f
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe/migrate: Introduce xe_migrate_reinit() and xe_cpu_bind_reinit()

This commit adds helpers to prepare and re-initialize the migration and
CPU bind contexts across a PCIe FLR.

> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 3c65b251b907e..fe71cb011d3a0 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -913,6 +913,13 @@ static int do_gt_restart(struct xe_gt *gt)
>  void xe_gt_flr_prepare(struct xe_gt *gt)
>  {
>  	xe_gt_sanitize(gt);
> +
> +	if (xe_gt_is_main_type(gt)) {
> +		struct xe_tile *tile = gt_to_tile(gt);
> +
> +		xe_migrate_flr_prepare(tile->migrate);
> +	}
> +
>  	xe_uc_flr_prepare(&gt->uc);
>  	xe_pagefault_reset(gt_to_xe(gt), gt);
>  	xe_tlb_inval_reset(&gt->tlb_inval);

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 19c37eb315893..d8f5744b0eb7c 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -272,9 +272,8 @@ static void xe_migrate_ulls_toggle_enable(struct xe_migrate *m, bool enabled)
>  	m->ulls.enabled = enabled;
>  }
>  
> -static void xe_migrate_fini(void *arg)
> +static void xe_migrate_ulls_fini(struct xe_migrate *m)
>  {
> -	struct xe_migrate *m = arg;
>  	struct xe_device *xe = tile_to_xe(m->tile);
>  
>  	disable_delayed_work_sync(&m->ulls.exit_work);
> @@ -284,6 +283,13 @@ static void xe_migrate_fini(void *arg)
>  			xe_migrate_ulls_toggle_enable(m, false);
>  		}
>  	}
> +}

[ ... ]

> @@ -778,6 +784,35 @@ static void xe_migrate_ulls_exit(struct work_struct *work)
>  	drm_dev_exit(idx);
>  }
>  
> +/**
> + * xe_migrate_flr_prepare() - Prepare migrate context for FLR
> + * @m: The migration context
> + */
> +void xe_migrate_flr_prepare(struct xe_migrate *m)
> +{
> +	xe_migrate_ulls_fini(m);
> +}
> +
> +/**
> + * xe_migrate_reinit() - Re-initialize a migrate context
> + * @m: The migration context
> + *
> + * Returns: 0 on success, negative error code otherwise.
> + */
> +int xe_migrate_reinit(struct xe_migrate *m)
> +{
> +	int err;
> +
> +	xe_migrate_prepare_vm(m->tile, m, m->q->vm, NULL);
> +
> +	err = xe_exec_queue_reinit(m->q);
> +	if (err)
> +		return err;
> +
> +	enable_delayed_work(&m->ulls.exit_work);
> +	return 0;
> +}

[Severity: Medium]
Can an in-flight xe_migrate_ulls_enter() race with xe_migrate_flr_prepare()
and leak a runtime PM reference while leaving m->ulls.enabled stuck true?

xe_gt_flr_prepare() calls xe_migrate_flr_prepare() while pagefault workers
can still run, and xe_migrate_ulls_enter() drops m->job_mutex to allocate a
sched job before re-acquiring it:

CPU0                                  CPU1
xe_gt_flr_prepare()                   xe_pagefault_service()
  xe_migrate_flr_prepare()              ...
    xe_migrate_ulls_fini()                xe_migrate_ulls_enter()
      disable_delayed_work_sync(            job = xe_sched_job_create(...)
        &m->ulls.exit_work)
      mutex_lock(&m->job_mutex)
      m->ulls.enabled = false
      mutex_unlock(&m->job_mutex)
                                            mutex_lock(&m->job_mutex)
                                            xe_pm_runtime_get_noresume(xe)
                                            m->ulls.enabled = true
                                            mod_delayed_work(...,
                                              &m->ulls.exit_work, ...)
                                              <-- fails, work is disabled
                                            mutex_unlock(&m->job_mutex)
...
xe_gt_reinit()
  xe_migrate_reinit()
    xe_exec_queue_reinit(m->q)
    enable_delayed_work(&m->ulls.exit_work)

Because m->ulls.exit_work is disabled when xe_migrate_ulls_enter() calls
mod_delayed_work(), the exit worker is not queued.  When xe_migrate_reinit()
later calls enable_delayed_work(), it only decrements the disable depth
without scheduling the work or resetting m->ulls.enabled, so the runtime PM
reference taken in xe_migrate_ulls_enter() is leaked and m->ulls.enabled
remains true on the re-initialized queue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112091033.1813803-1-raag.jadav@intel.com?part=6