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

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

On 18.07.2025 16:27, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Wajdeczko, Michal <Michal.Wajdeczko@intel.com> 
> Sent: Friday, July 18, 2025 2:30 AM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-xe@lists.freedesktop.org
> Cc: De Marchi, Lucas <lucas.demarchi@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@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
> 
> 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.

just to confirm (as this is a different reason than I mentioned above):
after pci_dev_put() we shouldn't access pdev any more

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