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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jul 30, 2025 at 10:23:22AM +0200, Michal Wajdeczko wrote:
> 
> 
> 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 */

What about -ENODEV, but with debug messages to differentiate the cases?

Otherwise -ENXIO seems the best of the alternatives here.

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