Date:	Thu, 24 Jul 2014 08:59:12 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Rob Jones <rob.jones@...ethink.co.uk>
Cc:	rdunlap@...radead.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kernel@...ts.codethink.co.uk,
	ian.molton@...ethink.co.uk, ben.dooks@...ethink.co.uk
Subject: Re: [PATCH 1/1] Managed Devices devres_debugfs file system

On Thu, Jul 24, 2014 at 04:18:01PM +0100, Rob Jones wrote:
> Reviewed-by: Ian Molton <ian.molton@...ethink.co.uk>
> Suggested-by: Ben Dooks <ben.dooks@...ethink.co.uk>
> Signed-off-by: Rob Jones <rob.jones@...ethink.co.uk>
> ---
>  Documentation/driver-model/devres-debugfs.txt |  140 +++++++++
>  drivers/base/Kconfig                          |   18 ++
>  drivers/base/devres.c                         |  387 +++++++++++++++++++++++++
>  3 files changed, 545 insertions(+)
>  create mode 100644 Documentation/driver-model/devres-debugfs.txt
> 
> diff --git a/Documentation/driver-model/devres-debugfs.txt b/Documentation/driver-model/devres-debugfs.txt
> new file mode 100644
> index 0000000..7004ebd
> --- /dev/null
> +++ b/Documentation/driver-model/devres-debugfs.txt
> @@ -0,0 +1,140 @@
> +
> +Introduction
> +============
> +
> +This document describes a file system that can be enabled to assist
> +with the debugging of devices that use managed reources
> +
> +Outline of Operation
> +====================
> +
> +Setting the flag DEVRES_DEBUGFS (depends on DEBUG_DEVRES) enables a
> +debug facility for device managed resources. This takes the form of
> +a directory in the debugfs file system which is a pseudo file system
> +(typically mounted at /sys/kernel/debug) similar in concept to sysfs
> +but with no restrictions on the format of the data read from a file.
> +
> +The directory (devres/) is created the first time a managed resource
> +is allocated for any device.
> +
> +The first time a managed resource is allocated for a device, a file
> +with the same name as the device is created in devres/.
> +
> +The file is read only and can be read by normal userspace utilities
> +(such as cat).
> +
> +The data returned is in ASCII text format and has one header line for
> +the device followed by one line per allocated resource.
> +
> +It is possible to seek to a given line within the file: position 0
> +(zero) goes directly to the device line, position 1 goes to the first
> +resource line and so on. Attempting to seek to a line that does not
> +exist returns EOF.
> +
> +If opened and read sequentially, a file will initially give Line 0 (see
> +below) and then each subsequent read will give a Resource Line until
> +EOF. Note that the data may change on the fly (see Notes below).
> +
> +Data Format
> +===========
> +
> +Device Line (Line 0)
> +--------------------
> +
> +dev: <address> <name>
> +
> +There is a single space between each field.
> +
> +dev: = literal text identifying this as a device line.
> +
> +<address> = the address in kernel memory of the device data structure.
> +The layout of "struct device" can be found in include/linux/device.h.
> +This value is displayed in hexadecimal format using the "%p" format
> +specifier and thus will vary in length depending on the word size of
> +the kernel, e.g. 8 characters for a 32 bit kernel.
> +
> +<name> = the name of the device. This should be the same as the file
> +name, it is included to facilitate identification when multiple
> +devices are being listed. The length of this field can vary.
> +
> +Resource Line (Line 1 et seq.)
> +------------------------------
> +
> +res: <address> <length> <data> <name>
> +
> +There is a single space between each field.
> +
> +res: = literal text identifying this as a resource line.
> +
> +<address> = the address in kernel memory of the resource data. The
> +structure pointed to can vary depending on the type of resource.
> +This field is encoded in hexadecimal format using the "%p" format
> +specifier and thus will vary in length depending on the word size of
> +the kernel, e.g. 8 characters for a 32 bit kernel.
> +
> +<length> = the length of the resource data. This field is encoded
> +in decimal format, right justified, space filled and is always 9
> +characters long.
> +
> +<data> = a hexadecimal display of the first 16 (or <length> if less)
> +bytes of data starting for location <address>. If <length> is less
> +than 16, this field is padded with spaces on the right to the full
> +32 characters.
> +
> +<name> = a string indentifying the resource type. This is often a
> +string containing the name of the function to be used to free the
> +resource. Typical examples are "devm_kzalloc_release" which identifies
> +a memory resource and "devm_gpio_release" which identifies a gpio
> +resource. The length of this field can vary.
> +
> +Notes
> +=====
> +
> +1. Once created, a file persists until the device is removed even
> +   if all allocated resources are freed. This means that the existence
> +   of the file is confirmation that at least one resource has been
> +   allocated at some point, even if the device now has none allocated.
> +
> +2. Opening a file and holding it open will prevent the freeing up of
> +   of the device - the final removal is held off until the file is
> +   closed. Managed resources can still be freed if, say, a driver is
> +   removed.
> +
> +3. Resources can, in principle, be allocated and freed on the fly so
> +   it should not be assumed in general that seeking to a particular
> +   resource line will always return the same resource. However, that is
> +   a function of the device driver concerned and it may be a valid
> +   assumption for some drivers, e.g. one that only allocates resources
> +   during its .probe function.
> +
> +4. The device's list of resources is traversed from its base to the
> +   requested item each time the file is read. During this scan resources
> +   are spinlocked, which may have implications if resources can be
> +   allocated during time critical code at the same time as a read is
> +   taking place.
> +
> +5. A copy is taken of (up to) 16 bytes of resource data while the
> +   spinlock (see note 4, above) is still in place and so can be relied
> +   upon as an accurate snapshot at the time of the read but it must be
> +   remembered that resources may change dynamically (see note 3, above)
> +   so the memory may have changed subsequently.
> +
> +Sample Output
> +=============
> +
> +root@arm:~# cat /sys/kernel/debug/devres/mmc0
> +dev: ed980008 mmc0
> +res: ed95a618        50 000000004884e8800001000039a695ed devm_kzalloc_release
> +res: ed95cc58         4 02000000                         devm_gpio_release
> +res: ed95cc98         8 a2000000000098ed                 devm_irq_release
> +root@arm:~#
> +
> +This shows three managed resources allocated to device "mmc0".
> +
> +1. A block of memory 50 bytes long (the first 16 byte are displayed).
> +2. A GPIO.
> +3. An IRQ.
> +
> +If 16 bytes of data is insufficient, you can try using other debugging
> +tools to examine the data.

Why did you pick 16 bytes?  What can I do with that information?


> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 8fa8dea..6a2f125 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -177,6 +177,24 @@ config DEBUG_DEVRES
>  
>  	  If you are unsure about this, Say N here.
>  
> +config DEVRES_DEBUGFS
> +	bool "Managed device resources debugfs file system"
> +	depends on DEBUG_DEVRES
> +	help
> +	  This option enables a debugfs file system related to Managed
> +	  Resources. When a device allocates a managed resource, with
> +	  this option enabled, a read-only file with the same name as
> +	  the device is created in the file system. Reading this file
> +	  provides some basic debug information about the managed
> +	  resources allocated to the device.
> +
> +	  The overhead caused by enabling this option is quite small.
> +	  Specifically, a small memory overhead for each device and a
> +	  small time overhead at each resource allocation and
> +	  de-allocation.
> +
> +	  If you are unsure about this, Say N here.
> +
>  config SYS_HYPERVISOR
>  	bool
>  	default n
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 5c88a27..41b6465 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -7,9 +7,13 @@
>   * This file is released under the GPLv2.
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/seq_file.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/uaccess.h>

why not just make this a separate file, that would remove the #ifdef
from this file, right?


> +/**
> + * devres_dbgfs_seq_show - seq file output routine for a devres debugfs file
> + * @s: pointer to seq file structure
> + * @v: pointer to private data set up by devres_dbgfs_seq_next().
> + *
> + * Static debug function called when the user reads from a device managed
> + * resources debugfs file. It outputs to the user buffer using seq_file
> + * function seq_printf();
> + *
> + * This function locks devres_lock in the device structure.
> + */
> +static int devres_dbgfs_seq_show(struct seq_file *s, void *v)
> +{
> +	struct devres_dbgfs_private *priv = v;
> +	struct device *dev = priv->dev;
> +	int size, i, pos = priv->pos;
> +	struct devres *dr;
> +	struct list_head *head;
> +	struct list_head *item;
> +	unsigned long flags;
> +	char data[16];
> +	char *dataptr;
> +
> +	if (pos == 0) {
> +		seq_printf(s, "dev: %p %s\n", dev, dev_name(dev));

%pK please for all kernel pointers.



> +/**
> + * devres_dbgfs_create_file - create debugfs file for a device
> + * @dev: device
> + *
> + * Static debug function that will automatically create a debugfs file
> + * with the same name as the supplied device IFF the said file has not
> + * already been created.
> + *
> + * This function locks devres_dbgfs_lock.
> + */
> +static void devres_dbgfs_create_file(struct device *dev)
> +{
> +	struct dentry *debugfsfile = NULL;
> +	struct devres_dbgfs_link *link;
> +	struct dentry *debugfsdir;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&devres_dbgfs_lock, flags);
> +
> +	link = devres_dbgfs_find_filelink(dev);
> +	if (link)
> +		goto out_unlock;
> +
> +	debugfsdir = devres_dbgfs_get_dir();
> +	if (debugfsdir)
> +		/* Create file, n.b. dev goes into inode->i_private */
> +		debugfsfile = debugfs_create_file(dev_name(dev), 0444,
> +						  debugfsdir, dev,
> +						  &devres_dbgfs_seq_fops);

I worry about this, as there is no reason that devices have to have
"unique" names in the system.  Odds are, there's lots of conflicts,
which is why sysfs is a tree, not a flat heirachy.


> +	if (!debugfsfile)
> +		goto out_unlock;

If debugfs is not enabled, this will not trigger, are you sure you want
that?


> +
> +	/* Success: now create filelink entry in linked list */
> +	link = kmalloc(sizeof(*link), GFP_KERNEL);
> +	if (!link) {
> +		debugfs_remove(debugfsfile);
> +		goto out_unlock;
> +	}
> +
> +	INIT_LIST_HEAD(&link->list);
> +	link->dev = dev;
> +	link->dp = debugfsfile;
> +	if (devres_dbgfs_lastused)
> +		list_add(&link->list, &devres_dbgfs_lastused->list);
> +	devres_dbgfs_lastused = link;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&devres_dbgfs_lock, flags);
> +}
> +#endif /* CONFIG_DEVRES_DEBUGFS */
> +
>  /*
>   * Release functions for devres group.  These callbacks are used only
>   * for identification.
> @@ -220,6 +601,9 @@ void devres_add(struct device *dev, void *res)
>  	spin_lock_irqsave(&dev->devres_lock, flags);
>  	add_dr(dev, &dr->node);
>  	spin_unlock_irqrestore(&dev->devres_lock, flags);
> +#ifdef CONFIG_DEVRES_DEBUGFS
> +	devres_dbgfs_create_file(dev);
> +#endif /* CONFIG_DEVRES_DEBUGFS */

ifdefs like this are strongly discouraged.

Overall, I'm curious as to what this code is good for?  What use cases
will benifit for seeing all of this information?  Why would a developer
ever care about all of the resources that a specific device has created,
what could I do with that information?

It seems "neat", but not something I would ever actually care about, as
there's nothing to do with that info.  A device will never allocate
something it doesn't actually need, right?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists