Re: [PATCH v5 10/11] drm/xe/configfs: Only allow configurations for supported devices

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

On 7/30/2025 12:20 AM, John Harrison wrote:
> On 7/29/2025 2:40 PM, Rodrigo Vivi wrote:
>> On Tue, Jul 29, 2025 at 01:42:14PM +0200, Michal Wajdeczko wrote:
>>> Since we already lookup for the real PCI device before we allow
>>> to create its directory config, we might also check if the found
>>> device matches our driver PCI ID list. This will prevent creation
>>> of the directory configs for the unsupported devices.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> ---
>>> v2: rebased
>>> ---
>>>   drivers/gpu/drm/xe/xe_configfs.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
>>> index 5f145ccdf535..766775772eef 100644
>>> --- a/drivers/gpu/drm/xe/xe_configfs.c
>>> +++ b/drivers/gpu/drm/xe/xe_configfs.c
>>> @@ -275,11 +275,22 @@ static const struct config_item_type xe_config_device_type = {
>>>       .ct_owner    = THIS_MODULE,
>>>   };
>>>   +static const struct xe_device_desc *xe_match_desc(struct pci_dev *pdev)
>>> +{
>>> +    struct device_driver *driver = driver_find("xe", &pci_bus_type);
>>> +    struct pci_driver *drv = to_pci_driver(driver);
>>> +    const struct pci_device_id *ids = drv ? drv->id_table : NULL;
>>> +    const struct pci_device_id *found = pci_match_id(ids, pdev);
>>> +
>>> +    return found ? (const void *)found->driver_data : NULL;
>>> +}
>>> +
>>>   static struct config_group *xe_config_make_device_group(struct config_group *group,
>>>                               const char *name)
>>>   {
>>>       unsigned int domain, bus, slot, function;
>>>       struct xe_config_group_device *dev;
>>> +    const struct xe_device_desc *match;
>>>       struct pci_dev *pdev;
>>>       char canonical[16];
>>>       int ret;
>>> @@ -297,8 +308,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 = xe_match_desc(pdev);
>>> +
>>>       pci_dev_put(pdev);
>>>   +    if (!match)
>>> +        return ERR_PTR(-ECANCELED);
>> why ECANCELED instead of ENODEV?
> Or EINVAL? This is for when a user has created a directory for a PCI device that exists but it is not a Xe device? Seems like ENODEV is not the correct option given that the device does exist. But ECANCELED also seems odd.

we can always return -EINVAL but user will have no clue what went wrong

so the idea was:

-EINVAL     when config directory name is malformed
-ENODEV     config directory name is correct but
            there is no such device under such BDF
-ECANCELED  when device exists but it is not covered by Xe driver

since you both don't like -ECANCELED for this case, what about:

-ECONNREFUSED /* Connection refused */
-EOPNOTSUPP   /* Operation not supported on transport endpoint */
-ENXIO        /* No such device or address */

> 
> John.
> 
>>
>>> +
>>>       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>>       if (!dev)
>>>           return ERR_PTR(-ENOMEM);
>>> -- 
>>> 2.47.1
>>>
>