[PATCH] driver core: add a debugfs entry to show deferred devices

[PATCH] driver core: add a debugfs entry to show deferred devices

[Javier Martinez Canillas]

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);
+
 	driver_deferred_probe_enable = true;
 	driver_deferred_probe_trigger();
 	/* Sort as many dependencies as possible before exiting initcalls */
-- 
2.17.1

Re: [PATCH] driver core: add a debugfs entry to show deferred devices

[Andy Shevchenko]

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
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] driver core: add a debugfs entry to show deferred devices

[Greg Kroah-Hartman]

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

Re: [PATCH] driver core: add a debugfs entry to show deferred devices

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] driver core: add a debugfs entry to show deferred devices

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] driver core: add a debugfs entry to show deferred devices

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] driver core: add a debugfs entry to show deferred devices

[Javier Martinez Canillas]

[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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] driver core: add a debugfs entry to show deferred devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]