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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 17.07.2025 23:19, Cavitt, Jonathan wrote:
> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Michal Wajdeczko
>> Sent: Thursday, July 17, 2025 11:48 AM
>> To: intel-xe@lists.freedesktop.org
>> Cc: Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; De Marchi, Lucas <lucas.demarchi@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@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@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

> 
> 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@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
>>
>>