On Wed, Sep 02, 2026 at 10:19:22AM +0530, Yadav, Arvind wrote:
>
> On 01-09-2026 02:13, Rodrigo Vivi wrote:
> > On Thu, Aug 27, 2026 at 03:47:51PM +0530, Arvind Yadav wrote:
> > > Page-fault work may still be queued when PCI error recovery starts or
> > > the device becomes permanently wedged. Servicing these faults can migrate
> > > memory or update page tables after device I/O has been blocked.
> > >
> > > Check the device state before and after fault servicing to cover a reset
> > > racing with the worker. Drop the active fault and its chained faults
> > > without sending a hardware response, and invalidate the cached fault
> > > state.
> > >
> > > Cc: Matthew Brost <[email protected]>
> > > Cc: Thomas Hellström <[email protected]>
> > > Cc: Himal Prasad Ghimiray <[email protected]>
> > > Cc: Rodrigo Vivi <[email protected]>
> > > Assisted-by: Claude:claude-opus-4-8
> > > Signed-off-by: Arvind Yadav <[email protected]>
> > > ---
> > > drivers/gpu/drm/xe/xe_pagefault.c | 40 +++++++++++++++++++++++++++++++
> > > 1 file changed, 40 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_pagefault.c
> > > b/drivers/gpu/drm/xe/xe_pagefault.c
> > > index 2e415995f067..f486165f2323 100644
> > > --- a/drivers/gpu/drm/xe/xe_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> > > @@ -592,6 +592,38 @@ static void xe_pagefault_save_to_vm(struct xe_device
> > > *xe, struct xe_pagefault *p
> > > xe_vm_put(vm);
> > > }
> > > +static bool
> > > +xe_pagefault_drop_if_blocked(struct xe_pagefault_queue *pf_queue,
> > > + struct xe_pagefault_work *pf_work,
> > > + struct xe_pagefault *pf,
> > > + u64 *cache_start)
> > > +{
> > > + struct xe_pagefault *next;
> > > +
> > > + if (!xe_device_io_blocked(pf_work->xe))
> > > + return false;
> > > +
> > > + /*
> > > + * cache_start is private to this worker invocation. pf_work->cache is
> > > + * shared with fault producers and must be invalidated under the queue
> > > + * lock.
> > > + */
> > > + xe_pagefault_cache_start_invalidate(*cache_start);
> > > +
> > > + guard(spinlock_irq)(&pf_queue->lock);
> > > +
> > > + xe_pagefault_cache_invalidate(pf_queue, pf_work);
> > > +
> > > + while (pf) {
> > > + next = pf->consumer.next;
> > > + pf->consumer.next = NULL;
> > > + pf->consumer.alloc_state = XE_PAGEFAULT_ALLOC_STATE_FREE;
> > > + pf = next;
> > is there some helper we could re-use? I know we cannot use the pop directly,
> > because it would deadlock, but perhaps we can unify some code here...
Why would it deadlock... xe_device_io_blocked is just an couple of
atomics, right?
So why wouldn't this work to squash all faults.
diff --git a/drivers/gpu/drm/xe/xe_pagefault.c
b/drivers/gpu/drm/xe/xe_pagefault.c
index 2e415995f067..1c9e5539b90d 100644
--- a/drivers/gpu/drm/xe/xe_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_pagefault.c
@@ -477,6 +477,12 @@ static bool xe_pagefault_queue_pop(struct
xe_pagefault_queue *pf_queue,
lpf = xe_pagefault_queue_tail_fault(pf_queue);
xe_pagefault_queue_advance(pf_queue);
+ if (xe_device_io_blocked(xe)) {
+ lpf->consumer.alloc_state =
+ XE_PAGEFAULT_ALLOC_STATE_FREE;
+ continue;
+ }
+
if (lpf->consumer.alloc_state !=
XE_PAGEFAULT_ALLOC_STATE_QUEUED)
continue;
>
>
> Noted,
>
> >
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > static void xe_pagefault_queue_work(struct work_struct *w)
> > > {
> > > struct xe_pagefault_work *pf_work =
> > > @@ -615,6 +647,10 @@ static void xe_pagefault_queue_work(struct
> > > work_struct *w)
> > > int err = 0;
> > > bool invalidated = false;
> > > + if (xe_pagefault_drop_if_blocked(pf_queue, pf_work, pf,
> > > + &cache_start))
> > > + continue;
> > do we really need to loop inside the loop or only check and let this main
> > loop goes?
>
>
> Yes, The inner walk is needed. Chained faults have already been removed from
> the queue and linked through consumer.next. The outer pop loop will not see
> them.
>
> >
> > > +
> > > /* Last fault same address, ack immediately */
> > > if (xe_pagefault_match(pf, cache_start, cache_end,
> > > cache_asid)) {
> > > xe_gt_stats_incr(gt,
> > > XE_GT_STATS_ID_LAST_PAGEFAULT_COUNT, 1);
> > > @@ -623,6 +659,10 @@ static void xe_pagefault_queue_work(struct
> > > work_struct *w)
> > > err = xe_pagefault_service(pf);
> > > + if (xe_pagefault_drop_if_blocked(pf_queue, pf_work, pf,
> > > + &cache_start))
> > do we really need to check after?
>
>
> Yes. The device can enter reset or become wedged while
> xe_pagefault_service() is running. The first check alone does not cover that
> race.
>
But this is still toctou - really everything in patch is as immediately
after either check the state can change. I can't say I'm a fan of any
weak checks in this code...
But just for thought: after this point all we do is CT which should be
down and we'd drop it there before touching the hardware.
Matt
> >
> > > + continue;
> > > +
> > > if (err) {
> > now this err far from where it is collected is pretty bad...
>
>
> Agreed. i will keeps err next to its use.
>
> Thanks,
> Arvind
>
> >
> > > if (!(pf->consumer.access_type &
> > > XE_PAGEFAULT_ACCESS_PREFETCH)) {
> > > xe_pagefault_save_to_vm(gt_to_xe(gt),
> > > pf);
> > > --
> > > 2.43.0
> > >
