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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10.06.2025 00:11, John Harrison wrote:
> 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!

there is some ongoing refactoring of IGT debugfs tests [1]
likely we must wait until it's finished to avoid clashes

[1] https://patchwork.freedesktop.org/series/150310/

> 
> 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.

your call, for me it's fine to drop this attribute

> 
>> 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?

AFAIK there is no DEFINE_STORE_ATTRIBUTE() helper macro (yet)

> 
> 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);
>>       }
>>   }
>