On Tue, Sep 01, 2026 at 10:30:41PM -0700, Matthew Brost wrote:
> 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;
> + }
> +
^^^ This should actually go after the XE_PAGEFAULT_ALLOC_STATE_QUEUED check.
Matt
> 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
> > > >
