Re: [PATCH] drm/xe/guc: Make debugfs guc_log_dmesg attribute write-only

[John Harrison <john.c.harrison@intel.com>]

On 6/3/2025 10:53 AM, Michal Wajdeczko wrote:
> Ocassionally we want to dump a GuC log into the dmesg, but current
I would describe this interface as purely a test for the dump-via-dmesg 
code itself. It is not really useful for triggering a dump for actual 
bug investigation purposes. If a system is alive enough to use this 
debugfs interface then it is alive enough to use the official 
devcoredump or GuC-log-via-debugfs interfaces.

Having said that, this change probably needs an IGT test to go with it. 
One that explicitly does the write and triggers the dump to make sure it 
works. Potentially it could actually check the dmesg output for a valid 
dump but as a first pass, just calling the interface and ensuring the 
kernel doesn't die is a good step!

The problem at the moment is that there are multiple IGTs which do a 
blanket read of everything in debugfs. Thus, they get the dump-via-dmesg 
even if they aren't interested in it. And those multiple dumps fill up 
dmesg to the point where CI complains about too much log output. So 
making this change will make those tests more reliable. However, it 
means that we would get no testing of the dump-via-dmesg mechanism 
itself. So unless a new IGT is added to explicitly test the interface, 
we might as well just remove it completely.

> implementation is based on the drm debugfs API and was triggering
> dump on the read operation.  Expose guc_log_dmesg attribute as
> write-only using pure debugfs API to make dump requests explicit.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc_debugfs.c | 43 ++++++++++++++++++++++++-----
>   1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_debugfs.c b/drivers/gpu/drm/xe/xe_guc_debugfs.c
> index 0b102ab46c4d..eb6124d02d53 100644
> --- a/drivers/gpu/drm/xe/xe_guc_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_guc_debugfs.c
> @@ -5,6 +5,8 @@
>   
>   #include "xe_guc_debugfs.h"
>   
> +#include <linux/debugfs.h>
> +
>   #include <drm/drm_debugfs.h>
>   #include <drm/drm_managed.h>
>   
> @@ -85,12 +87,6 @@ static int guc_log(struct xe_guc *guc, struct drm_printer *p)
>   	return 0;
>   }
>   
> -static int guc_log_dmesg(struct xe_guc *guc, struct drm_printer *p)
> -{
> -	xe_guc_log_print_dmesg(&guc->log);
> -	return 0;
> -}
> -
>   static int guc_ctb(struct xe_guc *guc, struct drm_printer *p)
>   {
>   	xe_guc_ct_print(&guc->ct, p, true);
> @@ -121,9 +117,39 @@ static const struct drm_info_list slpc_debugfs_list[] = {
>   /* everything else should be added here */
>   static const struct drm_info_list pf_only_debugfs_list[] = {
>   	{ "guc_log", .show = guc_debugfs_show, .data = guc_log },
> -	{ "guc_log_dmesg", .show = guc_debugfs_show, .data = guc_log_dmesg },
>   };
>   
> +/*
> + *      /sys/kernel/debug/dri/0/
> + *      ├── gt0
> + *      │   ├── uc
> + *      │   │   ├── guc_log_dmesg
> + */
> +static ssize_t guc_log_dmesg_write(struct file *file,
> +				   const char __user *userbuf,
> +				   size_t count, loff_t *ppos)
> +{
> +	struct seq_file *s = file->private_data;
> +	struct xe_guc *guc = s->private;
> +	bool yes;
> +	int ret;
> +
> +	if (*ppos)
> +		return -EINVAL;
> +	ret = kstrtobool_from_user(userbuf, count, &yes);
> +	if (ret < 0)
> +		return ret;
> +	if (yes)
> +		xe_guc_log_print_dmesg(&guc->log);
> +	return count;
> +}
> +
> +static int guc_log_dmesg_show(struct seq_file *s, void *unused)
> +{
> +	return 0;
> +}
> +DEFINE_SHOW_STORE_ATTRIBUTE(guc_log_dmesg);
Is a 'show' entry needed if the file is write only?

John.

> +
>   void xe_guc_debugfs_register(struct xe_guc *guc, struct dentry *parent)
>   {
>   	struct xe_device *xe =  guc_to_xe(guc);
> @@ -142,5 +168,8 @@ void xe_guc_debugfs_register(struct xe_guc *guc, struct dentry *parent)
>   			drm_debugfs_create_files(slpc_debugfs_list,
>   						 ARRAY_SIZE(slpc_debugfs_list),
>   						 parent, minor);
> +
> +		debugfs_create_file("guc_log_dmesg", 0200, parent,
> +				    guc, &guc_log_dmesg_fops);
>   	}
>   }