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

[Rodrigo Vivi]

On Wed, Jul 30, 2025 at 10:12:06AM -0700, John Harrison wrote:
> On 7/30/2025 9:30 AM, Rodrigo Vivi wrote:
> > 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 at intel.com>
> > > > > > Cc: Lucas De Marchi <lucas.demarchi at 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.
> I'm confused by what is wrong with EINVAL. Are there many other paths that
> can return EINVAL for this operation? If not then how is any other return
> code any different from a user confusion point of view? But yes, I think at
> least a debug message to say "requested device is not supported by this
> driver" would be good.

to me it looks more like a 'no device' than 'invalid device', hence my
thought on the enodev, but no strong/hard preference to be honest.
and we are definitely on the same page that there
shouldn't be nothing wrong with reusing the error code and printing the
debug message like 'requested device is not supported by this driver'...

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