Re: [PATCH v4 1/5] drm/xe/gt: Use GT ordered workqueue for wedging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] workqueue: uninitialized work_struct queued in PCI error handler
- [Medium] locking: Missing memory barriers for gt->initialized flag
--

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

drm/xe/gt: Use GT ordered workqueue for wedging

This commit introduces a worker for GT specific wedge handling and queues the
teardown on the GT ordered workqueue to avoid disrupting the scheduler for
in-flight jobs. It also adds a for_each_initialized_gt() macro to prevent
wedging uninitialized GTs during early probe.

> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
[ ... ]
> @@ -139,6 +139,10 @@ static inline bool xe_device_uc_enabled(struct xe_device *xe)
>  	for_each_gt((gt__), (tile__)->xe, (id__)) \
>  		for_each_if((gt__)->tile == (tile__))
>  
> +#define for_each_initialized_gt(gt__, xe__, id__) \
> +	for_each_gt((gt__), (xe__), (id__)) \
> +		for_each_if((gt__)->initialized)

[Severity: Medium]
Does this lockless read require an acquire barrier, such as
smp_load_acquire(), to ensure prior GT initialization stores are visible?

Without an acquire barrier here and a corresponding release barrier in
xe_gt_init(), the CPU or compiler can reorder the store to gt->initialized
before preceding hardware initialization stores.

A concurrent caller of xe_device_declare_wedged() (e.g., from a timeout
handler) could observe gt->initialized == true before the GT is fully
initialized, queueing the worker on a partially initialized GT.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
[ ... ]
> @@ -769,6 +773,8 @@ int xe_gt_init(struct xe_gt *gt)
>  			return err;
>  	}
>  
> +	/* Must be last */
> +	gt->initialized = true;

[Severity: Medium]
Does this plain store require a release barrier, such as smp_store_release()?

This flag is read locklessly in for_each_initialized_gt(). Without a release
barrier, architectural or compiler reordering of the preceding GT
initialization sequence could occur, causing concurrent readers to see the
flag set before initialization is actually complete.

[ ... ]
> @@ -1200,6 +1214,5 @@ void xe_gt_declare_wedged(struct xe_gt *gt)
>  {
>  	xe_gt_assert(gt, gt_to_xe(gt)->wedged.mode);
>  
> -	xe_uc_declare_wedged(&gt->uc);
> -	xe_tlb_inval_reset(&gt->tlb_inval);
> +	queue_work(gt->ordered_wq, &gt->wedge.worker);
>  }

[Severity: High]
Does this change break the PCI error handler in xe_pci_error.c?

This patch changes xe_gt_declare_wedged() to execute asynchronously by
queueing gt->wedge.worker. To avoid crashes when wedging happens during
early probe, xe_device_declare_wedged() was updated to use
for_each_initialized_gt().

However, prepare_device_for_reset() in drivers/gpu/drm/xe/xe_pci_error.c
still uses for_each_gt() and manually calls xe_gt_declare_wedged().

If a PCIe AER event triggers during early probe before xe_gt_init() completes,
it will pass an uninitialized GT to the wedge logic. This leads to
queue_work() acting on a zero-filled work_struct, causing list corruption
and a kernel panic.

Verification confirmed this bug remains unfixed at the end of the patch
series. Should prepare_device_for_reset() also be updated to use
for_each_initialized_gt()?

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