[PATCH 4/5] drm/xe/configfs: Allow configurations only for Intel VGA devices

[Cavitt, Jonathan]

-----Original Message-----
From: Wajdeczko, Michal <Michal.Wajdeczko at intel.com> 
Sent: Friday, July 18, 2025 2:30 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-xe at lists.freedesktop.org
Cc: De Marchi, Lucas <lucas.demarchi at intel.com>
Subject: Re: [PATCH 4/5] drm/xe/configfs: Allow configurations only for Intel VGA devices
> 
> On 17.07.2025 23:19, Cavitt, Jonathan wrote:
> > -----Original Message-----
> >> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Michal Wajdeczko
> >> Sent: Thursday, July 17, 2025 11:48 AM
> >> To: intel-xe at lists.freedesktop.org
> >> Cc: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>
> >> Subject: [PATCH 4/5] drm/xe/configfs: Allow configurations only for Intel VGA devices
> >>
> >> The Xe driver supports only Intel GPUs devices that all are PCI
> >> VGA class devices. Reject creation of configuration directories
> >> for PCI device addresses that are not Intel or VGA.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_configfs.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> >> index 00bb4e412c12..1df8cce78f13 100644
> >> --- a/drivers/gpu/drm/xe/xe_configfs.c
> >> +++ b/drivers/gpu/drm/xe/xe_configfs.c
> >> @@ -260,6 +260,7 @@ static struct config_group *xe_config_make_device_group(struct config_group *gro
> >>  	struct xe_config_device *dev;
> >>  	struct pci_dev *pdev;
> >>  	char canonical[16];
> >> +	bool match;
> >>  	int ret;
> >>  
> >>  	ret = sscanf(name, "%x:%x:%x.%d", &domain, &bus, &slot, &function);
> >> @@ -275,8 +276,14 @@ static struct config_group *xe_config_make_device_group(struct config_group *gro
> >>  	pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(slot, function));
> >>  	if (!pdev)
> >>  		return ERR_PTR(-ENODEV);
> >> +
> >> +	match = pci_is_vga(pdev) && pdev->vendor == PCI_VENDOR_ID_INTEL;
> > 
> > This match check is relatively short.  Debatably, we could just do the comparison
> > directly in the if-statement instead of assigning the result to a new variable.
> 
> what do you mean by the "if-statement" ?
> 
> we can't return early here as then we would leak (again) a device
> reference that we just fixed in patch 1/5

By 'if-statement', I meant this one lower down, after the pci_dev_put:

"""
	if (!match)
"""

I was suggesting that we could perform the check there instead.  Something like:

"""
	if (!pci_is_vga(pdev) || pdev->vendor != PCI_VENDOR_ID_INTEL)
"""

Doing so would eliminate the need for the new variable 'match' defined in this patch.

However, I understand why we're not doing it that way.
-Jonathan Cavitt

> 
> > 
> > I won't block on it, though, since clearly labeling these comparisons is important.  Though,
> > perhaps we should call it "op_supported" instead of "match"?  I understand why it's called
> > "match" here (it's more in line with how it's used in patch 5), but maybe this check and the
> > check in patch 5 should be separated out?
> 
> I'm not sure that adding more flags variables will make code any
> cleaner, what's important are conditions inside, and yes, this is
> aligned with next patch, which adds more conditions to be checked before
> we can claim that there is a "match" between device address and device
> class that we may support
> 
> > 
> > Just a suggestion.
> > 
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > -Jonathan Cavitt
> > 
> >> +
> >>  	pci_dev_put(pdev);
> >>  
> >> +	if (!match)
> >> +		return ERR_PTR(-ENODEV);
> >> +
> >>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >>  	if (!dev)
> >>  		return ERR_PTR(-ENOMEM);
> >> -- 
> >> 2.47.1
> >>
> >>
> 
>