[PATCH 1/3] drm/xe/pf: Disable PF restart worker on device removal

[Cavitt, Jonathan]

-----Original Message-----
From: Wajdeczko, Michal <Michal.Wajdeczko at intel.com> 
Sent: Wednesday, July 30, 2025 2:34 PM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-xe at lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/xe/pf: Disable PF restart worker on device removal
> 
> On 7/30/2025 11:08 PM, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Michal Wajdeczko
> > Sent: Wednesday, July 30, 2025 10:49 AM
> > To: intel-xe at lists.freedesktop.org
> > Cc: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
> > Subject: [PATCH 1/3] drm/xe/pf: Disable PF restart worker on device removal
> >>
> >> We can't let restart worker run once device is removed, since other
> >> data that it might want to access could be already released.
> >> Explicitly disable worker as part of device cleanup action.
> >>
> >> Fixes: a4d1c5d0b99b ("drm/xe/pf: Move VFs reprovisioning to worker")
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_gt_sriov_pf.c | 30 ++++++++++++++++++++++++++++-
> >>  1 file changed, 29 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf.c
> >> index 35489fa81825..2761319fdc26 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf.c
> >> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf.c
> >> @@ -50,6 +50,11 @@ static void pf_init_workers(struct xe_gt *gt)
> >>  	INIT_WORK(&gt->sriov.pf.workers.restart, pf_worker_restart_func);
> >>  }
> >>  
> >> +static void pf_fini_workers(struct xe_gt *gt)
> >> +{
> >> +	disable_work_sync(&gt->sriov.pf.workers.restart);
> >> +}
> >> +
> >>  /**
> >>   * xe_gt_sriov_pf_init_early - Prepare SR-IOV PF data structures on PF.
> >>   * @gt: the &xe_gt to initialize
> >> @@ -79,6 +84,21 @@ int xe_gt_sriov_pf_init_early(struct xe_gt *gt)
> >>  	return 0;
> >>  }
> >>  
> >> +static void pf_fini_action(void *arg)
> >> +{
> >> +	struct xe_gt *gt = arg;
> >> +
> >> +	pf_fini_workers(gt);
> >> +}
> >> +
> >> +static int pf_init_late(struct xe_gt *gt)
> >> +{
> >> +	struct xe_device *xe = gt_to_xe(gt);
> >> +
> >> +	xe_gt_assert(gt, IS_SRIOV_PF(xe));
> >> +	return devm_add_action_or_reset(xe->drm.dev, pf_fini_action, gt);
> >> +}
> >> +
> >>  /**
> >>   * xe_gt_sriov_pf_init - Prepare SR-IOV PF data structures on PF.
> >>   * @gt: the &xe_gt to initialize
> >> @@ -95,7 +115,15 @@ int xe_gt_sriov_pf_init(struct xe_gt *gt)
> >>  	if (err)
> >>  		return err;
> >>  
> 
> [1]
> 
> >> -	return xe_gt_sriov_pf_migration_init(gt);
> >> +	err = xe_gt_sriov_pf_migration_init(gt);
> >> +	if (err)
> >> +		return err;
> 
> [2]
> 
> >> +
> >> +	err = pf_init_late(gt);
> >> +	if (err)
> >> +		return err;
> > 
> > Everything else looks okay, but I don't think this conditional branch is necessary.
> > I think we can just return err here unconditionally, as if we fail this branch, err
> > would equal zero anyway, which is what we'd want to return.
> 
> this was done on purpose
> 
> I'm pretty sure modern compilers can optimize that and
> IMO it is clearer to leave success path fully visible
> 
> especially that in the future we might want to add more steps
> to this init() function, so we could have smaller diff and
> avoid diffs like [1-2] above

*shrugs*

I don't entirely agree, but I'm not going to cause a problem about it,
so you can take the Reviewed-by without any changes.

-Jonathan Cavitt
> 
> > 
> > With that change:
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > -Jonathan Cavitt
> > 
> >> +
> >> +	return 0;
> >>  }
> >>  
> >>  static bool pf_needs_enable_ggtt_guest_update(struct xe_device *xe)
> >> -- 
> >> 2.47.1
> >>
> >>
> 
>