Re: [PATCH v8 7/7] drm/xe: Open-code GGTT MMIO access protection

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

LGTM
Reviewed-by: Zhanjun Dong <zhanjun.dong@intel.com>

On 2026-02-24 11:35 a.m., Zhanjun Dong wrote:
> From: Matthew Brost <matthew.brost@intel.com>
> 
> GGTT MMIO access is currently protected by hotplug (drm_dev_enter),
> which works correctly when the driver loads successfully and is later
> unbound or unloaded. However, if driver load fails, this protection is
> insufficient because drm_dev_unplug() is never called.
> 
> Additionally, devm release functions cannot guarantee that all BOs with
> GGTT mappings are destroyed before the GGTT MMIO region is removed, as
> some BOs may be freed asynchronously by worker threads.
> 
> To address this, introduce an open-coded flag, protected by the GGTT
> lock, that guards GGTT MMIO access. The flag is cleared during the
> dev_fini_ggtt devm release function to ensure MMIO access is disabled
> once teardown begins.
> 
> Cc: stable@vger.kernel.org
> Fixes: 919bb54e989c ("drm/xe: Fix missing runtime outer protection for ggtt_remove_node")
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_ggtt.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 79310f565fe3..cf3311f5d140 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -66,6 +66,9 @@
>    * give us the correct placement for free.
>    */
>   
> +#define XE_GGTT_FLAGS_64K	BIT(0)
> +#define XE_GGTT_FLAGS_ONLINE	BIT(1)
> +
>   /**
>    * struct xe_ggtt_node - A node in GGTT.
>    *
> @@ -117,6 +120,8 @@ struct xe_ggtt {
>   	 * @flags: Flags for this GGTT
>   	 * Acceptable flags:
>   	 * - %XE_GGTT_FLAGS_64K - if PTE size is 64K. Otherwise, regular is 4K.
> +	 * - %XE_GGTT_FLAGS_ONLINE - is GGTT online, protected by ggtt->lock
> +	 *   after init
>   	 */
>   	unsigned int flags;
>   	/** @scratch: Internal object allocation used as a scratch page */
> @@ -367,6 +372,8 @@ static void dev_fini_ggtt(void *arg)
>   {
>   	struct xe_ggtt *ggtt = arg;
>   
> +	scoped_guard(mutex, &ggtt->lock)
> +		ggtt->flags &= ~XE_GGTT_FLAGS_ONLINE;
>   	drain_workqueue(ggtt->wq);
>   }
>   
> @@ -437,6 +444,7 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>   	if (err)
>   		return err;
>   
> +	ggtt->flags |= XE_GGTT_FLAGS_ONLINE;
>   	return devm_add_action_or_reset(xe->drm.dev, dev_fini_ggtt, ggtt);
>   }
>   ALLOW_ERROR_INJECTION(xe_ggtt_init_early, ERRNO); /* See xe_pci_probe() */
> @@ -465,13 +473,10 @@ static void ggtt_node_fini(struct xe_ggtt_node *node)
>   static void ggtt_node_remove(struct xe_ggtt_node *node)
>   {
>   	struct xe_ggtt *ggtt = node->ggtt;
> -	struct xe_device *xe = tile_to_xe(ggtt->tile);
>   	bool bound;
> -	int idx;
> -
> -	bound = drm_dev_enter(&xe->drm, &idx);
>   
>   	mutex_lock(&ggtt->lock);
> +	bound = ggtt->flags & XE_GGTT_FLAGS_ONLINE;
>   	if (bound)
>   		xe_ggtt_clear(ggtt, xe_ggtt_node_addr(node), xe_ggtt_node_size(node));
>   	drm_mm_remove_node(&node->base);
> @@ -484,8 +489,6 @@ static void ggtt_node_remove(struct xe_ggtt_node *node)
>   	if (node->invalidate_on_remove)
>   		xe_ggtt_invalidate(ggtt);
>   
> -	drm_dev_exit(idx);
> -
>   free_node:
>   	ggtt_node_fini(node);
>   }