* [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? @ 2021-06-03 12:55 Adrian Ratiu 2021-06-03 12:55 ` [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs Adrian Ratiu 2021-06-03 13:13 ` [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? Greg Kroah-Hartman 0 siblings, 2 replies; 9+ messages in thread From: Adrian Ratiu @ 2021-06-03 12:55 UTC (permalink / raw) To: Greg Kroah-Hartman, Rafael J. Wysocki; +Cc: Andrew Morton, kernel, linux-kernel Hi Greg & all, I would like to add a new debugfs file like in the next patch but I'm having a problem with commit 56348560d495 ("debugfs: do not attempt to create a new file before the filesystem is initalized"). The problem is debugfs is initialized after the driver core, during the core initcall, so I get an -ENOENT failure due to the above commit. My first reaction is to move the ksysfs_init() and debugfs_init() calls before the driver core init which works. Would that be ok? An alternative would be to create the new debugfs file somewhere else than the driver core, but I'm not sure where would be a good location, maybe in debugfs_init()? Doesn't seem right. Any guidance is appreciated, thank you, Adrian Adrian Ratiu (1): drivers: base: Expose probe failures via debugfs drivers/base/core.c | 76 +++++++++++++++++++++++++++++++++++++++++++-- lib/Kconfig.debug | 23 ++++++++++++++ 2 files changed, 96 insertions(+), 3 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs 2021-06-03 12:55 [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? Adrian Ratiu @ 2021-06-03 12:55 ` Adrian Ratiu 2021-06-03 13:16 ` Greg Kroah-Hartman 2021-06-03 13:13 ` [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? Greg Kroah-Hartman 1 sibling, 1 reply; 9+ messages in thread From: Adrian Ratiu @ 2021-06-03 12:55 UTC (permalink / raw) To: Greg Kroah-Hartman, Rafael J. Wysocki Cc: Andrew Morton, kernel, linux-kernel, Guillaume Tucker, Enric Balletbò 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. This is useful on automated test systems like KernelCI to avoid filtering dmesg dev_err() messages to extract potential probe failures. 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 + /** * 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 +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; +#endif + return 0; char_kobj_err: @@ -4623,6 +4663,34 @@ define_dev_printk_level(_dev_info, KERN_INFO); #endif +/** + * device_log_probe_failure: Print err to kernel log then add debugfs entry + * @dev: the pointer to the struct device + * @err: error value to print + * @vaf: struct containing printf-style error format string and arguments + */ +void device_log_probe_failure(const struct device *dev, int err, + struct va_format *vaf) +{ + dev_err(dev, "error %pe: %pV", ERR_PTR(err), vaf); + +#ifdef CONFIG_DEBUG_FS_PROBE_ERR + mutex_lock(&failed_probe_mutex); + + snprintf(probe_err_crbuf.buf + probe_err_crbuf.head, + PROBE_ERR_BUF_ELEM_SIZE, + "%s %s %d %pV", + dev_driver_string(dev), dev_name(dev), err, vaf); + + if (probe_err_crbuf.head < PROBE_ERR_BUF_SIZE - PROBE_ERR_BUF_ELEM_SIZE) + probe_err_crbuf.head += PROBE_ERR_BUF_ELEM_SIZE; + else + probe_err_crbuf.head = 0; + + mutex_unlock(&failed_probe_mutex); +#endif +} + /** * dev_err_probe - probe error check and log helper * @dev: the pointer to the struct device @@ -4631,10 +4699,12 @@ define_dev_printk_level(_dev_info, KERN_INFO); * @...: arguments as specified in the format string * * This helper implements common pattern present in probe functions for error - * checking: print debug or error message depending if the error value is - * -EPROBE_DEFER and propagate error upwards. + * checking: print debug or error message depending on the error value and then + * propagate the error upwards. * In case of -EPROBE_DEFER it sets also defer probe reason, which can be * checked later by reading devices_deferred debugfs attribute. + * Errors other than -EPROBE_DEFER can be checked by reading the devices_failed + * debugfs attribute if CONFIG_DEBUG_FS_PROBE_ERR is enabled. * It replaces code sequence:: * * if (err != -EPROBE_DEFER) @@ -4660,7 +4730,7 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) vaf.va = &args; if (err != -EPROBE_DEFER) { - dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf); + device_log_probe_failure(dev, err, &vaf); } else { device_set_deferred_probe_reason(dev, &vaf); dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf); diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 150f13baa6cc..10044ba47c26 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -578,6 +578,29 @@ config DEBUG_FS_ALLOW_NONE endchoice +config DEBUG_FS_PROBE_ERR + bool "Show device driver probe failures" + depends on DEBUG_FS + help + This feature enables a devices_failed debugfs attribute exposing + driver probe failures different from -EPROBE_DEFER. + +config DEBUG_FS_PROBE_ERR_BUF_SHIFT + int "Probe failures log buffer size" + depends on DEBUG_FS && DEBUG_FS_PROBE_ERR + range 12 16 + default 12 + help + This options allows to increase the default ringbuffer size for + driver probe failures as a power of 2. + + Examples: + 16 => 64 KB + 15 => 32 KB + 14 => 16 KB + 13 => 8 KB + 12 => 4 KB + source "lib/Kconfig.kgdb" source "lib/Kconfig.ubsan" source "lib/Kconfig.kcsan" -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs 2021-06-03 12:55 ` [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs Adrian Ratiu @ 2021-06-03 13:16 ` Greg Kroah-Hartman 2021-06-03 20:00 ` Adrian Ratiu 0 siblings, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2021-06-03 13:16 UTC (permalink / raw) To: Adrian Ratiu Cc: Rafael J. Wysocki, Andrew Morton, kernel, linux-kernel, Guillaume Tucker, Enric Balletbò 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? > 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? > 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. 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? > + > /** > * 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs 2021-06-03 13:16 ` Greg Kroah-Hartman @ 2021-06-03 20:00 ` Adrian Ratiu 2021-06-04 12:58 ` Greg Kroah-Hartman 0 siblings, 1 reply; 9+ messages in thread From: Adrian Ratiu @ 2021-06-03 20:00 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rafael J. Wysocki, Andrew Morton, kernel, linux-kernel, Guillaume Tucker, Enric Balletbò 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs 2021-06-03 20:00 ` Adrian Ratiu @ 2021-06-04 12:58 ` Greg Kroah-Hartman 0 siblings, 0 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2021-06-04 12:58 UTC (permalink / raw) To: Adrian Ratiu Cc: Rafael J. Wysocki, Andrew Morton, kernel, linux-kernel, Guillaume Tucker, Enric Balletbò On Thu, Jun 03, 2021 at 11:00:14PM +0300, Adrian Ratiu wrote: > 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. Then why not move to use the 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. Not good, I will not take this for that reason alone. Also I don't want to maintain something like this for the next 10+ years for no good reason. > > 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. But what exactly are you trying to detect? And what are you going to do if you detect it? > 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. How about making a "real" test for this type of thing and we add that to the kernel itself? Wouldn't that be a better thing to have? thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? 2021-06-03 12:55 [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? Adrian Ratiu 2021-06-03 12:55 ` [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs Adrian Ratiu @ 2021-06-03 13:13 ` Greg Kroah-Hartman 2021-06-03 20:00 ` Adrian Ratiu 1 sibling, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2021-06-03 13:13 UTC (permalink / raw) To: Adrian Ratiu; +Cc: Rafael J. Wysocki, Andrew Morton, kernel, linux-kernel On Thu, Jun 03, 2021 at 03:55:33PM +0300, Adrian Ratiu wrote: > Hi Greg & all, > > I would like to add a new debugfs file like in the next patch but > I'm having a problem with commit 56348560d495 ("debugfs: do not > attempt to create a new file before the filesystem is initalized"). You should have had a problem before that commit happened as well, right? > > The problem is debugfs is initialized after the driver core, during > the core initcall, so I get an -ENOENT failure due to the above commit. > > My first reaction is to move the ksysfs_init() and debugfs_init() calls > before the driver core init which works. Would that be ok? > > An alternative would be to create the new debugfs file somewhere else > than the driver core, but I'm not sure where would be a good location, > maybe in debugfs_init()? Doesn't seem right. I don't really want the driver core to be messing with debugfs at all, why is that needed? I'll respond on your patch... thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? 2021-06-03 13:13 ` [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? Greg Kroah-Hartman @ 2021-06-03 20:00 ` Adrian Ratiu 2021-06-04 12:56 ` Greg Kroah-Hartman 0 siblings, 1 reply; 9+ messages in thread From: Adrian Ratiu @ 2021-06-03 20:00 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Rafael J. Wysocki, Andrew Morton, kernel, linux-kernel Hi and thank you for the quick response! On Thu, 03 Jun 2021, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Jun 03, 2021 at 03:55:33PM +0300, Adrian Ratiu wrote: >> Hi Greg & all, I would like to add a new debugfs file like in >> the next patch but I'm having a problem with commit >> 56348560d495 ("debugfs: do not attempt to create a new file >> before the filesystem is initalized"). > > You should have had a problem before that commit happened as > well, right? > Actually no, it works without problems before commit 56348560d495 and also works if I revert that commit or move the debugfs_init() and its dependency ksysfs_init() before the driver core init. All these 3 cases work without issues while testing, but none of them seem to be viable ideas and especially moving debugfs init earlier just to add this new attr file to the driver core is some thin reasoning, so that's why I asked via this RFC. :) >> The problem is debugfs is initialized after the driver core, >> during the core initcall, so I get an -ENOENT failure due to >> the above commit. My first reaction is to move the >> ksysfs_init() and debugfs_init() calls before the driver core >> init which works. Would that be ok? An alternative would be >> to create the new debugfs file somewhere else than the driver >> core, but I'm not sure where would be a good location, maybe in >> debugfs_init()? Doesn't seem right. > > I don't really want the driver core to be messing with debugfs > at all, why is that needed? I'll respond on your patch... KernelCI maintainers asked me to add some tests for driver probe() results similar to how the -EPROBE_DEFER is tested via the existing debugfs devices_deferred and at the same time to add a new debugfs interface similar to devices_deferred to avoid parsing the printk buffer for non-EPROBE_DEFER results. If you think adding such an interface is a bad idea, please just tell me so and I will use it as amunition to push back and get the info from printk. :) > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? 2021-06-03 20:00 ` Adrian Ratiu @ 2021-06-04 12:56 ` Greg Kroah-Hartman 2021-06-04 16:06 ` Adrian Ratiu 0 siblings, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2021-06-04 12:56 UTC (permalink / raw) To: Adrian Ratiu; +Cc: Rafael J. Wysocki, Andrew Morton, kernel, linux-kernel On Thu, Jun 03, 2021 at 11:00:00PM +0300, Adrian Ratiu wrote: > Hi and thank you for the quick response! > > On Thu, 03 Jun 2021, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 03, 2021 at 03:55:33PM +0300, Adrian Ratiu wrote: > > > Hi Greg & all, I would like to add a new debugfs file like in the > > > next patch but I'm having a problem with commit 56348560d495 > > > ("debugfs: do not attempt to create a new file before the filesystem > > > is initalized"). > > > > You should have had a problem before that commit happened as well, > > right? > > > > Actually no, it works without problems before commit 56348560d495 and also > works if I revert that commit or move the debugfs_init() and its dependency > ksysfs_init() before the driver core init. And the file shows up properly? How can that be? Is it ok to create a file before the superblock is created in the system? For some reason I thought these were just silently failing and no one noticed. Maybe we should revisit that commit... thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? 2021-06-04 12:56 ` Greg Kroah-Hartman @ 2021-06-04 16:06 ` Adrian Ratiu 0 siblings, 0 replies; 9+ messages in thread From: Adrian Ratiu @ 2021-06-04 16:06 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Rafael J. Wysocki, Andrew Morton, kernel, linux-kernel On Fri, 04 Jun 2021, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Jun 03, 2021 at 11:00:00PM +0300, Adrian Ratiu wrote: >> Hi and thank you for the quick response! On Thu, 03 Jun 2021, >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: >> > On Thu, Jun 03, 2021 at 03:55:33PM +0300, Adrian Ratiu wrote: >> > > Hi Greg & all, I would like to add a new debugfs file like >> > > in the next patch but I'm having a problem with commit >> > > 56348560d495 ("debugfs: do not attempt to create a new file >> > > before the filesystem is initalized"). >> > You should have had a problem before that commit happened as >> > well, right? >> Actually no, it works without problems before commit >> 56348560d495 and also works if I revert that commit or move the >> debugfs_init() and its dependency ksysfs_init() before the >> driver core init. > > And the file shows up properly? How can that be? Is it ok to > create a file before the superblock is created in the system? > For some reason I thought these were just silently failing and > no one noticed. Maybe we should revisit that commit... Yes the file shows up and works properly, but I do not know why. My tests run after the system boots to an interactive shell and by that time everything is initialized and working. I thought maybe there were some specific corner-cases you wanted to guard against by adding that initialization -ENOENT check, but honestly since we're not adding a file earlier in the driver core anymore, I'd just leave it as is because the check makes sense in general. Thanks again, Adrian > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-06-04 16:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-03 12:55 [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? Adrian Ratiu 2021-06-03 12:55 ` [RFC PATCH 1/1] drivers: base: Expose probe failures via debugfs Adrian Ratiu 2021-06-03 13:16 ` Greg Kroah-Hartman 2021-06-03 20:00 ` Adrian Ratiu 2021-06-04 12:58 ` Greg Kroah-Hartman 2021-06-03 13:13 ` [RFC PATCH 0/1] Initialize debugfs/ksysfs earlier? Greg Kroah-Hartman 2021-06-03 20:00 ` Adrian Ratiu 2021-06-04 12:56 ` Greg Kroah-Hartman 2021-06-04 16:06 ` Adrian Ratiu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.