Re: [PATCH v5 11/11] drm/xe/configfs: Allow adding configurations for future VFs

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

On Tue, Jul 29, 2025 at 01:42:15PM +0200, Michal Wajdeczko wrote:
> Since we are expecting that all configuration directory names
> will match some of the existing devices, we can't provide any
> configuration for the VFs until they are actually enabled.
> 
> But we can relax that restriction by just checking if there
> is a PF device that could create given VF. This is easy since
> all our PF devices are always present at function 0 and we can
> query PF device for number of VFs it could support.
> 
> Then for some system with PF device at 0000:00:02.0 we can add
> configs for all VFs:
> 
>   /sys/kernel/config/xe/
>   ├── 0000:00:02.0
>   │   └── ...
>   ├── 0000:00:02.1
>   │   └── ...
>   ├── 0000:00:02.2
>   │   └── ...
>   :
>   └── 0000:00:02.7
>       └── ...
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> v2: rebase and improve checks, fix include order
> v3: put correct device (Lucas)
> ---
>  drivers/gpu/drm/xe/xe_configfs.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 766775772eef..da9a8e64621f 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
> @@ -12,9 +12,9 @@
>  #include <linux/string.h>
>  
>  #include "xe_configfs.h"
> -#include "xe_module.h"
> -
>  #include "xe_hw_engine_types.h"
> +#include "xe_module.h"
> +#include "xe_pci_types.h"
>  
>  /**
>   * DOC: Xe Configfs
> @@ -285,6 +285,15 @@ static const struct xe_device_desc *xe_match_desc(struct pci_dev *pdev)
>  	return found ? (const void *)found->driver_data : NULL;
>  }
>  
> +static struct pci_dev *get_physfn_instead(struct pci_dev *virtfn)
> +{
> +	struct pci_dev *physfn = pci_physfn(virtfn);
> +
> +	pci_dev_get(physfn);
> +	pci_dev_put(virtfn);
> +	return physfn;
> +}
> +
>  static struct config_group *xe_config_make_device_group(struct config_group *group,
>  							const char *name)
>  {
> @@ -306,11 +315,22 @@ static struct config_group *xe_config_make_device_group(struct config_group *gro
>  		return ERR_PTR(-EINVAL);
>  
>  	pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(slot, function));
> +	if (!pdev && function)
> +		pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(slot, 0));
> +	if (!pdev && slot)
> +		pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(0, 0));

I confess I got a bit lost here... specially this last case with 00.00.0, why's this
needed?

Perhaps it would be good to add some comments here explaining a bit of the
flow and thought... from the commit message I understand the PCI_DEVFN(slot, 0)....

Oh wait... okay the discrete has another level 03.00.0 hence you check this
case here as well...

Perhaps a comment to explain the different cases here would be really helpful
for the future... but the code is right:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

 
>  	if (!pdev)
>  		return ERR_PTR(-ENODEV);
> +	pdev = get_physfn_instead(pdev);
>  
>  	match = xe_match_desc(pdev);
> +	if (match) {
> +		int vfnumber = PCI_DEVFN(slot, function) - pdev->devfn;
>  
> +		if (vfnumber && (!dev_is_pf(&pdev->dev) || !match->has_sriov ||
> +				 vfnumber > pci_sriov_get_totalvfs(pdev)))
> +			match = NULL;
> +	}
>  	pci_dev_put(pdev);
>  
>  	if (!match)
> -- 
> 2.47.1
>