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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jul 30, 2025 at 09:40:14PM +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Wajdeczko, Michal <Michal.Wajdeczko@intel.com> 
> Sent: Wednesday, July 30, 2025 2:34 PM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-xe@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@lists.freedesktop.org> On Behalf Of Michal Wajdeczko
> > > Sent: Wednesday, July 30, 2025 10:49 AM
> > > To: intel-xe@lists.freedesktop.org
> > > Cc: Wajdeczko, Michal <Michal.Wajdeczko@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@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.

Next time please wait for the answer before stating your reviewed-by
so it comes with more confidence or there are still room for discussions.

Thanks,
Rodrigo.

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