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

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

On Fri, Mar 07, 2025 at 07:54:44PM +0530, Riana Tauro wrote:
> Registers a configfs subsystem called 'xe' to userspace. The user can
> use this to modify exposed attributes.
> 
> Add survivability mode attribute (config/xe/survivability_mode) to the
> subsystem to allow the user to specify the card that should enter
> survivability mode.
> 
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/Makefile      |  1 +
>  drivers/gpu/drm/xe/xe_configfs.c | 95 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_configfs.h | 12 ++++
>  drivers/gpu/drm/xe/xe_module.c   |  5 ++
>  drivers/gpu/drm/xe/xe_module.h   |  1 +
>  5 files changed, 114 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 9699b08585f7..3f8c87292cee 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..8c5f248e466d
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#include <linux/configfs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +
> +#include "xe_configfs.h"
> +#include "xe_module.h"
> +
> +/**
> + * DOC: Xe Configfs
> + *
> + * XE KMD registers a configfs subsystem called 'xe'to userspace that allows users to modify

----->                                                 ^ missing space

> + * the exposed attributes.
> + *
> + * Attributes:
> + *
> + * config/xe/survivability_mode : Write only attribute that allows user to specify the PCI address
> + *				  of the card that has to enter survivability mode
> + */
> +
> +void xe_configfs_clear_survivability_mode(void)
> +{
> +	kfree(xe_modparam.survivability_mode);
> +	xe_modparam.survivability_mode = NULL;

let's avoid xe_modparam, but more about this below...

> +}
> +
> +static ssize_t survivability_mode_store(struct config_item *item, const char *page, size_t len)
> +{
> +	char *survivability_mode;
> +	int ret;
> +	unsigned int domain, bus, slot, function;
> +
> +	ret = sscanf(page, "%04x:%02x:%02x.%x", &domain, &bus, &slot, &function);
> +	if (ret != 4)
> +		return -EINVAL;
> +
> +	survivability_mode = kstrdup(page, GFP_KERNEL);
> +	if (!survivability_mode)
> +		return -ENOMEM;
> +
> +	xe_configfs_clear_survivability_mode();
> +	xe_modparam.survivability_mode = survivability_mode;
> +
> +	return len;
> +}
> +
> +CONFIGFS_ATTR_WO(, survivability_mode);
> +
> +static struct configfs_attribute *xe_configfs_attrs[] = {
> +	&attr_survivability_mode,
> +	NULL,
> +};
> +
> +static const struct config_item_type xe_config_type = {
> +	.ct_attrs	= xe_configfs_attrs,
> +	.ct_owner	= THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem xe_config_subsys = {
> +	.su_group = {
> +		.cg_item = {
> +			.ci_namebuf = "xe",
> +			.ci_type = &xe_config_type,
> +		},
> +	},
> +};
> +
> +int __init xe_configfs_init(void)
> +{
> +	int ret;
> +
> +	config_group_init(&xe_config_subsys.su_group);
> +	mutex_init(&xe_config_subsys.su_mutex);

That's strange... if we are not using the mutex here,
why do we need to initialize it ourselves?
Why isn't it done inside the config_group_init then?!

> +	ret = configfs_register_subsystem(&xe_config_subsys);
> +	if (ret) {
> +		pr_err("Error %d while registering subsystem %s\n",
> +		       ret, xe_config_subsys.su_group.cg_item.ci_namebuf);
> +		mutex_destroy(&xe_config_subsys.su_mutex);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void __exit xe_configfs_exit(void)
> +{
> +	xe_configfs_clear_survivability_mode();
> +	configfs_unregister_subsystem(&xe_config_subsys);
> +	mutex_destroy(&xe_config_subsys.su_mutex);
> +}
> +
> diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
> new file mode 100644
> index 000000000000..491629a2ca53
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_configfs.h
> @@ -0,0 +1,12 @@
> +/* 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);
> +void xe_configfs_clear_survivability_mode(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,
> diff --git a/drivers/gpu/drm/xe/xe_module.h b/drivers/gpu/drm/xe/xe_module.h
> index 84339e509c80..c238dbee6bc7 100644
> --- a/drivers/gpu/drm/xe/xe_module.h
> +++ b/drivers/gpu/drm/xe/xe_module.h
> @@ -24,6 +24,7 @@ struct xe_modparam {
>  #endif
>  	int wedged_mode;
>  	u32 svm_notifier_size;
> +	char *survivability_mode;

We should probably create another struct
struct xe_configfs {
       char *survivability_mode;
}

it could be here or perhaps inside a xe_configfs_types.h
that is imported wherever it is needed.

>  };
>  
>  extern struct xe_modparam xe_modparam;
> -- 
> 2.47.1
>