[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 7:34 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 18.07.2025 16:27, Cavitt, Jonathan wrote:
> > -----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.
> 
> just to confirm (as this is a different reason than I mentioned above):
> after pci_dev_put() we shouldn't access pdev any more

Okay.  That more directly answers why we'd need to store the value earlier: we very
literally cannot access the value later, so we need to do the comparison now and save
the return value for when it's needed.

My later comment on separating the check into "op_supported" and "match" still
stands, but as it's just a suggestion, I'll continue to not block on it.

My Reviewed-by still stands as well.
-Jonathan Cavitt

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