| Message ID | 20180619205914.21375-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 19, 2018 at 11:59 PM, Javier Martinez Canillas <javierm@redhat.com> wrote: > For debugging purposes it may be useful to know what are the devices whose > probe function was deferred. Add a debugfs entry showing that information. > > $ cat /sys/kernel/debug/deferred_devices > 48070000.i2c:twl@48:bci > musb-hdrc.0.auto > omapdrm.0 > FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > --- > > Changes since RFC v1: > - Remove unneeded ret variable from deferred_devs_show() > > Changes since RFC v2: > - Use DEFINE_SHOW_ATTRIBUTE() macro. > - Don't propagate debugfs_create_file() error. > - Remove IS_ENABLED(CONFIG_DEBUG_FS) guards. > - Drop RFC prefix. > > drivers/base/dd.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 1435d7281c6..8ec9e3cfbe4 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -16,6 +16,7 @@ > * Copyright (c) 2007-2009 Novell Inc. > */ > > +#include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/delay.h> > #include <linux/dma-mapping.h> > @@ -224,6 +225,24 @@ void device_unblock_probing(void) > driver_deferred_probe_trigger(); > } > > +/* > + * deferred_devs_show() - Show the devices in the deferred probe pending list. > + */ > +static int deferred_devs_show(struct seq_file *s, void *data) > +{ > + struct device_private *curr; > + > + mutex_lock(&deferred_probe_mutex); > + > + list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) > + seq_printf(s, "%s\n", dev_name(curr->device)); > + > + mutex_unlock(&deferred_probe_mutex); > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(deferred_devs); > + > /** > * deferred_probe_initcall() - Enable probing of deferred devices > * > @@ -233,6 +252,9 @@ void device_unblock_probing(void) > */ > static int deferred_probe_initcall(void) > { > + debugfs_create_file("deferred_devices", 0444, NULL, NULL, > + &deferred_devs_fops); > + > driver_deferred_probe_enable = true; > driver_deferred_probe_trigger(); > /* Sort as many dependencies as possible before exiting initcalls */ > -- > 2.17.1 >
On Tue, Jun 19, 2018 at 10:59:14PM +0200, Javier Martinez Canillas wrote: > For debugging purposes it may be useful to know what are the devices whose > probe function was deferred. Add a debugfs entry showing that information. > > $ cat /sys/kernel/debug/deferred_devices > 48070000.i2c:twl@48:bci > musb-hdrc.0.auto > omapdrm.0 > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > --- > > Changes since RFC v1: > - Remove unneeded ret variable from deferred_devs_show() > > Changes since RFC v2: > - Use DEFINE_SHOW_ATTRIBUTE() macro. > - Don't propagate debugfs_create_file() error. > - Remove IS_ENABLED(CONFIG_DEBUG_FS) guards. > - Drop RFC prefix. > > drivers/base/dd.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 1435d7281c6..8ec9e3cfbe4 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -16,6 +16,7 @@ > * Copyright (c) 2007-2009 Novell Inc. > */ > > +#include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/delay.h> > #include <linux/dma-mapping.h> > @@ -224,6 +225,24 @@ void device_unblock_probing(void) > driver_deferred_probe_trigger(); > } > > +/* > + * deferred_devs_show() - Show the devices in the deferred probe pending list. > + */ > +static int deferred_devs_show(struct seq_file *s, void *data) > +{ > + struct device_private *curr; > + > + mutex_lock(&deferred_probe_mutex); > + > + list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) > + seq_printf(s, "%s\n", dev_name(curr->device)); > + > + mutex_unlock(&deferred_probe_mutex); > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(deferred_devs); > + > /** > * deferred_probe_initcall() - Enable probing of deferred devices > * > @@ -233,6 +252,9 @@ void device_unblock_probing(void) > */ > static int deferred_probe_initcall(void) > { > + debugfs_create_file("deferred_devices", 0444, NULL, NULL, > + &deferred_devs_fops); In the root of debugfs? Anyway, what about "devices_deferred", to help keep things semi-sane if we have other driver core debugfs entries? And you don't remove the file ever? And what is the use of this file? What can you do with this information? Who is going to use it? Don't we have other deferred probe debugging somewhere else? thanks, greg k-h
On Wed, Jun 20, 2018 at 1:51 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Tue, Jun 19, 2018 at 10:59:14PM +0200, Javier Martinez Canillas wrote: >> For debugging purposes it may be useful to know what are the devices whose >> probe function was deferred. Add a debugfs entry showing that information. >> >> $ cat /sys/kernel/debug/deferred_devices >> 48070000.i2c:twl@48:bci >> musb-hdrc.0.auto >> omapdrm.0 > And what is the use of this file? What can you do with this > information? Who is going to use it? Don't we have other deferred > probe debugging somewhere else? Indeed. Javier, have you tried to add 'initcall_debug' to a kernel command line followed by 'dyndbg="file drivers/base/dd.c +p"'?
On 06/20/2018 12:51 AM, Greg Kroah-Hartman wrote: [snip] >> @@ -233,6 +252,9 @@ void device_unblock_probing(void) >> */ >> static int deferred_probe_initcall(void) >> { >> + debugfs_create_file("deferred_devices", 0444, NULL, NULL, >> + &deferred_devs_fops); > > In the root of debugfs? > I added in the root for lack of a better place. Any suggestion is welcomed. > Anyway, what about "devices_deferred", to help keep things semi-sane if > we have other driver core debugfs entries? > I don't have a strong opinion on the name really, so I'll change it. > And you don't remove the file ever? > Yeah, I saw that it wasn't removed in other places for debugfs entries created by the core since unlike drivers they can't be built as a module or re-loaded. But you are right, I'll add an __exitcall to remove there. > And what is the use of this file? What can you do with this > information? Who is going to use it? Don't we have other deferred This patch is the result of a discussion with Tomeu and Mark (cc'ed) to allow https://kernelci.org to test if there was a regression that makes drivers to defer their probe. The problem with the probe deferral mechanism is that you don't have a way to distinguish between a valid deferral due a dependency not being available yet and a bug (i.e: wrong DTB, config symbol not enabled, etc) that prevents the device to eventually being probed. > probe debugging somewhere else? > There is some debug yes, but it isn't suitable for the use case I explained. For start, it only tells you if a given driver for a device was deferred or probed correctly while this patch attempts to tell what was left (if any) in the queue after the last driver was registered. Second, is only enabled until late_initcall so it will only print the probe deferral for built-in drivers and not for modules. This patch registers the debugfs entry after the probe debugging has been disabled. > thanks, > > greg k-h > Best regards,
Hi Andy, On 06/20/2018 01:43 AM, Andy Shevchenko wrote: > On Wed, Jun 20, 2018 at 1:51 AM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> On Tue, Jun 19, 2018 at 10:59:14PM +0200, Javier Martinez Canillas wrote: >>> For debugging purposes it may be useful to know what are the devices whose >>> probe function was deferred. Add a debugfs entry showing that information. >>> >>> $ cat /sys/kernel/debug/deferred_devices >>> 48070000.i2c:twl@48:bci >>> musb-hdrc.0.auto >>> omapdrm.0 > > >> And what is the use of this file? What can you do with this >> information? Who is going to use it? Don't we have other deferred >> probe debugging somewhere else? > > Indeed. > > Javier, have you tried to add 'initcall_debug' to a kernel command > line followed by 'dyndbg="file drivers/base/dd.c +p"'? > I already mentioned this to Greg, but I'll elaborate a little bit. Using these kernel cmdline options will only tell us when a driver for a device was probed or deferred but it doesn't tell us what's left in the queue after all drivers have been registered. Yes, we could parse the kernel log and do some computation to figure out if a deferred driver finally got probed, but I don't understand why we can't just expose the deferred queue if the kernel already has that info and is useful? But even if we do that, the current debug printouts are only enabled until late_initcall time. So it won't print deferred probes for drivers registered by modules: static void deferred_probe_work_func(struct work_struct *work) { ... if (initcall_debug && !initcalls_done) deferred_probe_debug(dev); else bus_probe_device(dev); ... } static int deferred_probe_initcall(void) { ... initcalls_done = true; ... } late_initcall(deferred_probe_initcall); Again, we could change that but in my opinion we should try to make debug more easier and this patch is quite trivial. The kernelci folks said that this will be useful for them and allows to detect regressions on drivers' probe as early as possible, which I think is very important. Best regards,
[adding Peter Robinson - Fedora IoT Architect to cc list] On 06/20/2018 10:46 AM, Javier Martinez Canillas wrote: > On 06/20/2018 12:51 AM, Greg Kroah-Hartman wrote: > > [snip] > >>> @@ -233,6 +252,9 @@ void device_unblock_probing(void) >>> */ >>> static int deferred_probe_initcall(void) >>> { >>> + debugfs_create_file("deferred_devices", 0444, NULL, NULL, >>> + &deferred_devs_fops); >> >> In the root of debugfs? >> > > I added in the root for lack of a better place. Any suggestion is welcomed. > >> Anyway, what about "devices_deferred", to help keep things semi-sane if >> we have other driver core debugfs entries? >> > > I don't have a strong opinion on the name really, so I'll change it. > >> And you don't remove the file ever? >> > > Yeah, I saw that it wasn't removed in other places for debugfs entries > created by the core since unlike drivers they can't be built as a module > or re-loaded. But you are right, I'll add an __exitcall to remove there. > >> And what is the use of this file? What can you do with this >> information? Who is going to use it? Don't we have other deferred > > This patch is the result of a discussion with Tomeu and Mark (cc'ed) to > allow https://kernelci.org to test if there was a regression that makes > drivers to defer their probe. > > The problem with the probe deferral mechanism is that you don't have a > way to distinguish between a valid deferral due a dependency not being > available yet and a bug (i.e: wrong DTB, config symbol not enabled, etc) > that prevents the device to eventually being probed. > This is not only useful for catching regressions though, Peter also told me that having this information would save him a lot of time when doing hardware bringup for ARM devices / IoT platforms. As mentioned, debugging probe deferral issues caused by drivers not available or wrong Device Trees is really a PITA. Not all architectures have the luxury of ACPI / PnP / auto enumerable buses / etc, that hide all this complexity. So the most information to troubleshoot we have, the better in my opinion. >> probe debugging somewhere else? >> > > There is some debug yes, but it isn't suitable for the use case I explained. > > For start, it only tells you if a given driver for a device was deferred or > probed correctly while this patch attempts to tell what was left (if any) > in the queue after the last driver was registered. > > Second, is only enabled until late_initcall so it will only print the probe > deferral for built-in drivers and not for modules. This patch registers the > debugfs entry after the probe debugging has been disabled. > >> thanks, >> >> greg k-h >> Best regards,
On Wed, Jun 20, 2018 at 07:51:45AM +0900, Greg Kroah-Hartman wrote: > And what is the use of this file? What can you do with this > information? Who is going to use it? Don't we have other deferred > probe debugging somewhere else? Pretty much all we have right now is kernel log messages, and people keep trying to suppress those as they're quite noisy sometimes. Ideally the device dependency stuff will reduce that problem but I'm not sure how that's getting on.
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 1435d7281c6..8ec9e3cfbe4 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -16,6 +16,7 @@ * Copyright (c) 2007-2009 Novell Inc. */ +#include <linux/debugfs.h> #include <linux/device.h> #include <linux/delay.h> #include <linux/dma-mapping.h> @@ -224,6 +225,24 @@ void device_unblock_probing(void) driver_deferred_probe_trigger(); } +/* + * deferred_devs_show() - Show the devices in the deferred probe pending list. + */ +static int deferred_devs_show(struct seq_file *s, void *data) +{ + struct device_private *curr; + + mutex_lock(&deferred_probe_mutex); + + list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) + seq_printf(s, "%s\n", dev_name(curr->device)); + + mutex_unlock(&deferred_probe_mutex); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(deferred_devs); + /** * deferred_probe_initcall() - Enable probing of deferred devices * @@ -233,6 +252,9 @@ void device_unblock_probing(void) */ static int deferred_probe_initcall(void) { + debugfs_create_file("deferred_devices", 0444, NULL, NULL, + &deferred_devs_fops); + driver_deferred_probe_enable = true; driver_deferred_probe_trigger(); /* Sort as many dependencies as possible before exiting initcalls */
For debugging purposes it may be useful to know what are the devices whose probe function was deferred. Add a debugfs entry showing that information. $ cat /sys/kernel/debug/deferred_devices 48070000.i2c:twl@48:bci musb-hdrc.0.auto omapdrm.0 Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- Changes since RFC v1: - Remove unneeded ret variable from deferred_devs_show() Changes since RFC v2: - Use DEFINE_SHOW_ATTRIBUTE() macro. - Don't propagate debugfs_create_file() error. - Remove IS_ENABLED(CONFIG_DEBUG_FS) guards. - Drop RFC prefix. drivers/base/dd.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)