Re: [PATCH 1/1] drm/xe/vf: Reset recovery_queued after issuing RESFIX_START

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Dec 05, 2025 at 09:01:16PM +0100, Michal Wajdeczko wrote:
> 
> 
> On 12/5/2025 9:26 AM, Satyanarayana K V P wrote:
> > During VF_RESTORE or VF_RESUME, the GuC sends a migration interrupt and
> > clears the RESFIX_START marker. If migration or resume occurs before the
> > VF issues its own RESFIX_START, VF KMD may receive two back-to-back
> > migration interrupts. VF then sends RESFIX_START to indicate the beginning
> > of fixups and RESFIX_DONE to mark completion. However, the second
> > RESFIX_START fails because the GuC is already in the RUNNING state.
> > 
> > To prevent VF KMD from queuing additional recovery work items when extra
> > interrupts arrive, move the clearing of recovery_queued from
> > vf_post_migration_shutdown() to vf_post_migration_resfix_start().
> 
> hmm, it's not about moving the code from one function to other, as this is
> implementation detail clear from the diff below, but rather we want to say
> "clear the flag after sending a RESFIX_START message to ignore duplicated
> IRQs seen before we start actual recovery"
> 
> > This ensures the state is reset only after the fixup process begins,
> > avoiding redundant work item queuing.
> > 
> > Fixes: b5fbb94341a2 ("drm/xe/vf: Introduce RESFIX start marker support")
> > Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Tomasz Lis <tomasz.lis@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > index 3c806c8e5f3e..90f2ef1772f2 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > @@ -781,7 +781,7 @@ static void vf_start_migration_recovery(struct xe_gt *gt)
> >  
> >  	spin_lock(&gt->sriov.vf.migration.lock);
> >  
> > -	if (!gt->sriov.vf.migration.recovery_queued ||
> > +	if (!gt->sriov.vf.migration.recovery_queued &&
> 
> this is a different fix that deserves its own separate patch with
> a proper commit message
> 

Yes, this fixes my original implementation of VF migration. That was
definitely wrong—I inverted || and &&. So your change is correct, but I
agree with Michal that this should be in its own patch so we can
backport it independently. 

> >  	    !gt->sriov.vf.migration.recovery_teardown) {
> >  		gt->sriov.vf.migration.recovery_queued = true;
> >  		WRITE_ONCE(gt->sriov.vf.migration.recovery_inprogress, true);
> > @@ -1171,10 +1171,6 @@ static bool vf_post_migration_shutdown(struct xe_gt *gt)
> >  			return true;
> >  	}
> >  
> > -	spin_lock_irq(&gt->sriov.vf.migration.lock);
> > -	gt->sriov.vf.migration.recovery_queued = false;
> > -	spin_unlock_irq(&gt->sriov.vf.migration.lock);
> > -
> >  	xe_guc_ct_flush_and_stop(&gt->uc.guc.ct);
> >  	xe_guc_submit_pause_vf(&gt->uc.guc);
> >  	xe_tlb_inval_reset(&gt->tlb_inval);
> > @@ -1258,7 +1254,15 @@ static int vf_post_migration_resfix_done(struct xe_gt *gt, u16 marker)
> >  
> >  static int vf_post_migration_resfix_start(struct xe_gt *gt, u16 marker)
> >  {
> > -	return vf_resfix_start(gt, marker);
> > +	int err;
> > +
> > +	err = vf_resfix_start(gt, marker);
> > +
> > +	spin_lock_irq(&gt->sriov.vf.migration.lock);
> > +	gt->sriov.vf.migration.recovery_queued = false;
> > +	spin_unlock_irq(&gt->sriov.vf.migration.lock);
> 
> we may want to use
> 
> 	scoped_guard(spinlock_irq, &gt->sriov.vf.migration.lock)
> 		gt->sriov.vf.migration.recovery_queued = false;
> 

It doesn't even need to be scoped as we immediatly return.

Moving to this function looks right, btw.

Matt

> > +
> > +	return err;
> >  }
> >  
> >  static u16 vf_post_migration_next_resfix_marker(struct xe_gt *gt)
>