Re: [PATCH 1/2] drm/xe: Add configfs to enable survivability mode

[Riana Tauro <riana.tauro@intel.com>]

Hi Lucas

Thank you for the review comments
On 4/1/2025 6:33 AM, Lucas De Marchi wrote:
> On Thu, Mar 27, 2025 at 12:12:01PM +0530, Riana Tauro wrote:
>> Registers a configfs subsystem called 'xe' to userspace. The user can
>> use this to modify exposed attributes for a device.
>>
>> Attribute exposed:
>>
>> /config/xe/<device>/survivability_mode : Enables survivability mode
>>
>> The attributes can be modified by creating the device directory under the
>> configfs directory
>>
>> mount -t configfs none /config
>> mkdir /config/xe/0000:03:00.0
>>
>> echo 0000:03:00.0 > /sys/bus/pci/drivers/xe/unbind
>> echo 1 > /config/xe/0000:03:00.0/survivability_mode
>> echo 0000:03:00.0 > /sys/bus/pci/drivers/xe/bind
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile      |   1 +
>> drivers/gpu/drm/xe/xe_configfs.c | 174 +++++++++++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_configfs.h |  11 ++
>> drivers/gpu/drm/xe/xe_module.c   |   5 +
>> 4 files changed, 191 insertions(+)
>> create mode 100644 drivers/gpu/drm/xe/xe_configfs.c
>> create mode 100644 drivers/gpu/drm/xe/xe_configfs.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index cd464fe26eb8..190176c2e10d 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -28,6 +28,7 @@ $(obj)/generated/%_wa_oob.c $(obj)/generated/ 
>> %_wa_oob.h: $(obj)/xe_gen_wa_oob \
>> xe-y += xe_bb.o \
>>     xe_bo.o \
>>     xe_bo_evict.o \
>> +    xe_configfs.o \
>>     xe_devcoredump.o \
>>     xe_device.o \
>>     xe_device_sysfs.o \
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/ 
>> xe_configfs.c
>> new file mode 100644
>> index 000000000000..59e1bc4c5f76
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_configfs.c
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#include <linux/configfs.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +
>> +#include "xe_configfs.h"
>> +#include "xe_module.h"
>> +
>> +/**
>> + * DOC: XE Configfs Support
>> + *
>> + * Overview
>> + * =========
>> + *
>> + * Configfs is a filesystem-based manager of kernel objects. XE KMD 
>> registers a
>> + * configfs subsystem called 'xe' that allows user to configure 
>> various attributes
>> + * See Documentation/filesystems/configfs.rst for more information 
>> about how configfs works.
>> + *
>> + * Create devices
>> + * ===============
>> + *
>> + * To use configfs exposed by Xe, mount the configfs directory and 
>> create the device directory
>> + *
>> + * $ mount -t configfs none /config
> 
> nit: once upon a time I think the "standard" was to have a /config. Same
> thing as for debugfs. Nowadays I think pretty much every distro settled
> on /sys/kernel/{debug,config} for debugfs and configfs. Mine even gets
> mounted automatically:
> 
Oh okay. All docs had this so added the same.Yeah i saw the 
/sys/kernel/config being mounted. Ok will change this to generic

> # grep Where /usr/lib/systemd/system/sys-kernel-config.mount
> Where=/sys/kernel/config
> 
>> + * $ mkdir /config/xe/0000:03:00.0
>> + *
>> + * Configure Attributes
>> + * ====================
>> + *
>> + * /config/xe/<device>/survivability mode : Enables survivability 
>> mode if supported by device
> 
>     * <configfs>/xe/<device>/survivability_mode: Enable survivability 
> mode if supported by device
> 
> 
>> + *
>> + * Unbind            : echo 0000:03:00.0 > /sys/bus/pci/drivers/xe/ 
>> unbind
> 
> please, no unbind. The unbind part is part of "the previous time the
> driver was attached/bound to the device".
> 
>> + * Enable survivability mode    : echo 1 > /config/xe/0000:03:00.0/ 
>> survivability_mode
>> + * Bind                : echo 0000:03:00.0 > /sys/bus/pci/drivers/xe/ 
>> bind
> 
> are these extra spaces to try to align anything?

aligns in the code. will generate kernel-doc and check>
>> + *
>> + * The device enters survivability mode if supported
>> + *
> 
> trailing line.
will remove this>
>> + */
>> +
>> +struct xe_config_device {
>> +    struct config_group group;
>> +
>> +    /** @survivability_mode: enables survivability mode when set */
> 
> I don't think we should kernel-doc this. But if we are, then it should
> be proper kernel-doc, including the doc about the struct and not leaving
> members undocumented.
will remove this>
>> +    bool survivability_mode;
>> +
>> +    /** @lock: protects attributes */
>> +    struct mutex lock;
>> +};
>> +
>> +static struct xe_config_device *to_xe_config_device(struct 
>> config_item *item)
>> +{
>> +    return container_of(to_config_group(item), struct 
>> xe_config_device, group);
>> +}
>> +
>> +static ssize_t survivability_mode_show(struct config_item *item, char 
>> *page)
>> +{
>> +    struct xe_config_device *dev = to_xe_config_device(item);
>> +
>> +    return sprintf(page, "%d\n", dev->survivability_mode);
>> +}
>> +
>> +static ssize_t survivability_mode_store(struct config_item *item, 
>> const char *page, size_t len)
>> +{
>> +    struct xe_config_device *dev = to_xe_config_device(item);
>> +    bool survivability_mode;
>> +    int ret;
>> +
>> +    ret = kstrtobool(page, &survivability_mode);
>> +    if (ret)
>> +        return ret;
>> +
>> +    mutex_lock(&dev->lock);
>> +    dev->survivability_mode = survivability_mode;
>> +    mutex_unlock(&dev->lock);
>> +
>> +    return len;
>> +}
>> +
>> +CONFIGFS_ATTR(, survivability_mode);
>> +
>> +static struct configfs_attribute *xe_config_device_attrs[] = {
>> +    &attr_survivability_mode,
>> +    NULL,
>> +};
>> +
>> +static void xe_config_device_release(struct config_item *item)
>> +{
>> +    struct xe_config_device *dev = to_xe_config_device(item);
>> +
>> +    mutex_destroy(&dev->lock);
>> +    kfree(dev);
>> +}
>> +
>> +static struct configfs_item_operations xe_config_device_ops = {
>> +    .release    = xe_config_device_release,
>> +};
>> +
>> +static const struct config_item_type xe_config_device_type = {
>> +    .ct_item_ops    = &xe_config_device_ops,
>> +    .ct_attrs    = xe_config_device_attrs,
>> +    .ct_owner    = THIS_MODULE,
>> +};
>> +
>> +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_device *dev;
>> +    struct pci_dev *pdev;
>> +    int ret;
>> +
>> +    ret = sscanf(name, "%04x:%02x:%02x.%x", &domain, &bus, &slot, 
>> &function);
>> +    if (ret != 4)
>> +        return ERR_PTR(-EINVAL);
>> +
>> +    pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(slot, 
>> function));
>> +    if (!pdev)
>> +        return ERR_PTR(-EINVAL);
>> +
>> +    dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> +    if (!dev)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    config_group_init_type_name(&dev->group, name, 
>> &xe_config_device_type);
>> +
>> +    mutex_init(&dev->lock);
>> +
>> +    return &dev->group;
>> +}
>> +
>> +static struct configfs_group_operations xe_config_device_group_ops = {
>> +    .make_group    = xe_config_make_device_group,
>> +};
>> +
>> +static const struct config_item_type xe_configfs_type = {
>> +    .ct_group_ops    = &xe_config_device_group_ops,
>> +    .ct_owner    = THIS_MODULE,
> 
> Rodrigo,            ^
> 
> this is what makes the config side take a ref on the module, as it
> should.
> 
>> +};
>> +
>> +static struct configfs_subsystem xe_configfs = {
>> +    .su_group = {
>> +        .cg_item = {
>> +            .ci_namebuf = "xe",
>> +            .ci_type = &xe_configfs_type,
>> +        },
>> +    },
>> +};
>> +
>> +int __init xe_configfs_init(void)
>> +{
>> +    struct config_group *root = &xe_configfs.su_group;
>> +    int ret = 0;
> 
> pointless init as it's overwritten below.
> 
>> +
>> +    config_group_init(root);
>> +    mutex_init(&xe_configfs.su_mutex);
>> +    ret = configfs_register_subsystem(&xe_configfs);
>> +    if (ret) {
>> +        pr_err("Error %d while registering %s subsystem\n",
>> +               ret, root->cg_item.ci_namebuf);
> 
> return ret;
> 
>> +    }
>> +
>> +    return ret;
> 
> return 0;
> 
> 
> A bunch of nits above, but looking pretty good.

Will fix the above review comments.

Thank you
Riana>
> thanks
> Lucas De Marchi
> 
>> +}
>> +
>> +void __exit xe_configfs_exit(void)
>> +{
>> +    configfs_unregister_subsystem(&xe_configfs);
>> +}
>> +
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/ 
>> xe_configfs.h
>> new file mode 100644
>> index 000000000000..2c30be9a2c7e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_configfs.h
>> @@ -0,0 +1,11 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +#ifndef _XE_CONFIGFS_H_
>> +#define _XE_CONFIGFS_H_
>> +
>> +int xe_configfs_init(void);
>> +void xe_configfs_exit(void);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/ 
>> xe_module.c
>> index 475acdba2b55..15b3cf22193c 100644
>> --- a/drivers/gpu/drm/xe/xe_module.c
>> +++ b/drivers/gpu/drm/xe/xe_module.c
>> @@ -11,6 +11,7 @@
>> #include <drm/drm_module.h>
>>
>> #include "xe_drv.h"
>> +#include "xe_configfs.h"
>> #include "xe_hw_fence.h"
>> #include "xe_pci.h"
>> #include "xe_pm.h"
>> @@ -91,6 +92,10 @@ static const struct init_funcs init_funcs[] = {
>>     {
>>         .init = xe_check_nomodeset,
>>     },
>> +    {
>> +        .init = xe_configfs_init,
>> +        .exit = xe_configfs_exit,
>> +    },
>>     {
>>         .init = xe_hw_fence_module_init,
>>         .exit = xe_hw_fence_module_exit,
>> -- 
>> 2.47.1
>>