[PATCH 1/2] drm/xe: Add configfs to enable survivability mode
Riana Tauro
riana.tauro at intel.com
Tue Apr 1 08:13:48 UTC 2025
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 at 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
>>
More information about the Intel-xe
mailing list