Re: [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Thu, 03 Jun 2021, Greg Kroah-Hartman 
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 03, 2021 at 03:55:34PM +0300, Adrian Ratiu wrote: 
>> This adds a new devices_failed debugfs attribute to list driver 
>> probe failures excepting -EPROBE_DEFER which are still handled 
>> as before via their own devices_deferred attribute. 
> 
> Who is going to use this? 
> 

It's for KernelCI testing, I explained the background in my other 
reply.

>> This is useful on automated test systems like KernelCI to avoid 
>> filtering dmesg dev_err() messages to extract potential probe 
>> failures. 
> 
> I thought we listed these already some other way today? 
>

The only other place is the printk buffer via dev_err() and only 
the result subset of -EPROBE_DEFER info is exported via debugfs.

An additional problem with this new interface implementation is 
that it is based on the new-ish driver core "dev_err_probe" helper 
to which not all drivers have been converted (yet...), so there 
will be a mismatch between printk and this new interface.
 
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael 
>> J. Wysocki" <rafael@kernel.org> Cc: Guillaume Tucker 
>> <gtucker.collabora@gmail.com> Suggested-by: Enric Balletbò 
>> <enric.balletbo@collabora.com> Signed-off-by: Adrian Ratiu 
>> <adrian.ratiu@collabora.com> --- 
>>  drivers/base/core.c | 76 
>>  +++++++++++++++++++++++++++++++++++++++++++-- 
>>  lib/Kconfig.debug   | 23 ++++++++++++++ 2 files changed, 96 
>>  insertions(+), 3 deletions(-) 
>>  diff --git a/drivers/base/core.c b/drivers/base/core.c index 
>> b8a8c96dca58..74bf057234b8 100644 --- a/drivers/base/core.c +++ 
>> b/drivers/base/core.c @@ -9,7 +9,9 @@ 
>>   */ 
>>   #include <linux/acpi.h> 
>> +#include <linux/circ_buf.h> 
>>  #include <linux/cpufreq.h> 
>> +#include <linux/debugfs.h> 
>>  #include <linux/device.h> #include <linux/err.h> #include 
>>  <linux/fwnode.h> 
>> @@ -53,6 +55,15 @@ static DEFINE_MUTEX(fwnode_link_lock); 
>>  static bool fw_devlink_is_permissive(void); static bool 
>>  fw_devlink_drv_reg_done;  
>> +#ifdef CONFIG_DEBUG_FS_PROBE_ERR +#define 
>> PROBE_ERR_BUF_ELEM_SIZE	64 +#define PROBE_ERR_BUF_SIZE	(1 
>> << CONFIG_DEBUG_FS_PROBE_ERR_BUF_SHIFT) +static struct circ_buf 
>> probe_err_crbuf; +static char 
>> failed_probe_buffer[PROBE_ERR_BUF_SIZE]; +static 
>> DEFINE_MUTEX(failed_probe_mutex); +static struct dentry 
>> *devices_failed_probe; +#endif 
> 
> All of this just for a log buffer?  The kernel provides a great 
> one, printk, let's not create yet-another-log-buffer if at all 
> possible please.

Yes, that is correct, this is esentially duplicating information 
already exposed via the printk buffer.
 
> 
> If the existing messages are "hard to parse", what can we do to 
> make them "easier" that would allow systems to do something with 
> them? 
> 
> What _do_ systems want to do with this information anyway?  What 
> does it help with exactly? 
>

I know driver core probe error message formats are unlikely to 
change over time and debugfs in theory is as "stable" as printk, 
but I think the main concern is to find a a more reliable way than 
parsing printk to extract probe erros, like for the existing 
devices_deferred in debugfs.

The idea in my specific case is to be able to reliably run driver 
tests in KernelCI for expected or unexpected probe errors like 
-EINVAL.

>
>
>> +
>>  /**
>>   * fwnode_link_add - Create a link between two fwnode_handles.
>>   * @con: Consumer end of the link.
>> @@ -3769,6 +3780,29 @@ struct device *device_find_child_by_name(struct device *parent,
>>  }
>>  EXPORT_SYMBOL_GPL(device_find_child_by_name);
>>  
>> +/*
>> + * failed_devs_show() - Show devices & drivers which failed to probe.
>> + */
>> +#ifdef CONFIG_DEBUG_FS_PROBE_ERR
>
> .c files shouldn't have #ifdefs if at all possible, so this patch isn't
> good for that reason alone :(
>
>
>> +static int failed_devs_show(struct seq_file *s, void *data)
>> +{
>> +	size_t offset;
>> +
>> +	mutex_lock(&failed_probe_mutex);
>> +
>> +	for (offset = 0;
>> +	     offset < PROBE_ERR_BUF_SIZE;
>> +	     offset += PROBE_ERR_BUF_ELEM_SIZE)
>> +		if (probe_err_crbuf.buf[offset])
>> +			seq_printf(s, "%s\n", probe_err_crbuf.buf + offset);
>> +
>> +	mutex_unlock(&failed_probe_mutex);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(failed_devs);
>> +#endif
>> +
>>  int __init devices_init(void)
>>  {
>>  	devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL);
>> @@ -3784,6 +3818,12 @@ int __init devices_init(void)
>>  	if (!sysfs_dev_char_kobj)
>>  		goto char_kobj_err;
>>  
>> +#ifdef CONFIG_DEBUG_FS_PROBE_ERR
>> +	devices_failed_probe = debugfs_create_file("devices_failed", 0444, NULL,
>> +						   NULL, &failed_devs_fops);
>> +	probe_err_crbuf.buf = failed_probe_buffer;
>
> Nit, no need to save the dentry here, you can look it up if you really
> need it later on, but most importantly, you NEVER do anything with this
> dentry so why save it at all?
>
> And again, #ifdef is not ok, that makes the code much more
> unmaintainable over time.
>
> thanks,
>
> greg k-h