[PATCH v3 1/2] drm/xe/pf: Create a link between PF and VF devices

[Michal Wajdeczko]

On 21.02.2025 12:07, Satyanarayana K V P wrote:
> When both PF and VF devices are enabled on the host, they
> resume simultaneously during system resume.
> 
> However, the PF must finish provisioning the VF before any
> VFs can successfully resume.
> 
> Establish a parent-child device link between the PF and VF
> devices to ensure the correct order of resumption.
> 
> V2 -> V3:
> - Added function documentation for xe_pci_pf_get_vf_dev().
> - Added assertion if not called from PF.
> V1 -> V2:
> - Added a helper function to get VF pci_dev.
> - Updated xe_sriov_notice() to xe_sriov_warn() if vf pci_dev
>   is not found.
> 
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p at intel.com>
> Cc: Michał Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Piotr Piórkowski <piotr.piorkowski at intel.com>
> Reviewed-by: Piotr Piorkowski <piotr.piorkowski at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pci_sriov.c | 60 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_pci_sriov.h |  6 ++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pci_sriov.c b/drivers/gpu/drm/xe/xe_pci_sriov.c
> index aaceee748287..b9ac3b4e0497 100644
> --- a/drivers/gpu/drm/xe/xe_pci_sriov.c
> +++ b/drivers/gpu/drm/xe/xe_pci_sriov.c
> @@ -62,6 +62,64 @@ static void pf_reset_vfs(struct xe_device *xe, unsigned int num_vfs)
>  			xe_gt_sriov_pf_control_trigger_flr(gt, n);
>  }
>  
> +/**
> + * xe_pci_pf_get_vf_dev - Get VF pci_dev.
> + * @pdev: the &pci_dev
> + * @vf_id: the VF identifier
> + *
> + * This function can only be called on PF.
> + *
> + * This function can be called to get VF (Virtual Function) pci_dev
> + * using PF (Physical Function) pci_dev for a given VF id.
> + *
> + * Return: pci_dev structure of VF on success or NULL on error.
> + */
> +struct pci_dev *xe_pci_pf_get_vf_dev(struct pci_dev *pdev, unsigned int vf_id)

it doesn't look that function is used elsewhere, shouldn't it be static?

and as static function it could take "xe" as param

struct pci_dev *pf_lookup_vf_pdev(struct xe_device *xe,
				  unsigned int vf_id)

> +{
> +	struct xe_device *xe = pdev_to_xe_device(pdev);
> +
> +	xe_assert(xe, IS_SRIOV_PF(xe));
> +
> +	/* caller must use pci_dev_put() */
> +	return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> +			pdev->bus->number,
> +			pci_iov_virtfn_devfn(pdev, vf_id));
> +}
> +
> +static void pf_link_vfs(struct xe_device *xe, int num_vfs)
> +{
> +	struct pci_dev *pdev_pf = to_pci_dev(xe->drm.dev);
> +	struct device_link *link;
> +	struct pci_dev *pdev_vf;
> +	unsigned int n;
> +
> +	for (n = 1; n <= num_vfs; n++) {
> +		pdev_vf = xe_pci_pf_get_vf_dev(pdev_pf, n - 1);
> +
> +		if (!pdev_vf) {
> +			xe_sriov_warn(xe, "Can't link PF and VF%d due to missing pci dev..\n", n);

xe_sriov_warn() will add a "PF: " prefix to the output so likely "PF"
inside the message is redundant

n is unsigned so the VF format should be VF%u

what does "missing pci dev" really mean to the user?
and this looks like a serious problem that would affect all VFs so maybe
we should just abort and say:
	"Can't find VF%u device, aborting link%s creation!"
	n, str_plural(num_vfs)

two dots at the end (likely none is needed)

> +			continue;
> +		}
> +
> +		/*
> +		 * When both PF and VF devices are enabled on the host, during system
> +		 * resume they are resuming in parallel.
> +		 *
> +		 * But PF has to complete the provision of VF first to allow any VFs to
> +		 * successfully resume.
> +		 *
> +		 * Create a parent-child device link between PF and VF devices that will
> +		 * enforce correct resume order.
> +		 */

I would move that comment just before the "for" loop

> +		link = device_link_add(&pdev_vf->dev, &pdev_pf->dev,
> +				       DL_FLAG_AUTOREMOVE_CONSUMER);
> +		if (!link)
> +			xe_sriov_notice(xe, "Failed linking PF and VF%u\n", n);
> +
> +		pci_dev_put(pdev_vf);
> +	}
> +}
> +
>  static int pf_enable_vfs(struct xe_device *xe, int num_vfs)
>  {
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> @@ -92,6 +150,8 @@ static int pf_enable_vfs(struct xe_device *xe, int num_vfs)
>  	if (err < 0)
>  		goto failed;
>  
> +	pf_link_vfs(xe, num_vfs);
> +
>  	xe_sriov_info(xe, "Enabled %u of %u VF%s\n",
>  		      num_vfs, total_vfs, str_plural(total_vfs));
>  	return num_vfs;
> diff --git a/drivers/gpu/drm/xe/xe_pci_sriov.h b/drivers/gpu/drm/xe/xe_pci_sriov.h
> index c76dd0d90495..f66b68a25b20 100644
> --- a/drivers/gpu/drm/xe/xe_pci_sriov.h
> +++ b/drivers/gpu/drm/xe/xe_pci_sriov.h
> @@ -10,11 +10,17 @@ struct pci_dev;
>  
>  #ifdef CONFIG_PCI_IOV
>  int xe_pci_sriov_configure(struct pci_dev *pdev, int num_vfs);
> +struct pci_dev *xe_pci_pf_get_vf_dev(struct pci_dev *pdev, unsigned int vf_id);
>  #else
>  static inline int xe_pci_sriov_configure(struct pci_dev *pdev, int num_vfs)
>  {
>  	return 0;
>  }
> +
> +static inline struct pci_dev *xe_pci_pf_get_vf_dev(struct pci_dev *pdev, unsigned int vf_id)
> +{
> +	return NULL;
> +}
>  #endif

both chunks likely unneeded

>  
>  #endif