Re: [PATCH v2] component: add debugfs support

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Nov 15, 2017 at 01:05:26PM +0100, Maciej Purski wrote:
> Add 'component' directory to debugfs. Create a new file for each master,
> when a master is added. Remove it on a master deletion.
> 
> Show a list of devices matched with master and indicate if
> master's components were successfully added and if the whole master is
> bound.
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
> Changes in v2:
> - use seq_printf() instead of seq_puts() when printing headers
> - move whole debugfs code to the file beginning in order to avoid
>   forward declarations or using multiple ifdefs
> ---
>  drivers/base/component.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 89b032f..8745ad9 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> +#include <linux/debugfs.h>
>  
>  struct component;
>  
> @@ -41,6 +42,7 @@ struct master {
>  	const struct component_master_ops *ops;
>  	struct device *dev;
>  	struct component_match *match;
> +	struct dentry *dentry;
>  };
>  
>  struct component {
> @@ -56,6 +58,85 @@ static DEFINE_MUTEX(component_mutex);
>  static LIST_HEAD(component_list);
>  static LIST_HEAD(masters);
>  
> +#ifdef CONFIG_DEBUG_FS
> +
> +static struct dentry *component_debugfs_dir;
> +
> +static int component_devices_show(struct seq_file *s, void *data)
> +{
> +	struct master *m = s->private;
> +	struct component_match *match = m->match;
> +	size_t i;
> +
> +	mutex_lock(&component_mutex);
> +	seq_printf(s, "%-40s %20s\n", "master name", "status");
> +	seq_puts(s, "-------------------------------------------------------------\n");
> +	seq_printf(s, "%-40s %20s\n\n",
> +		   dev_name(m->dev), m->bound ? "bound" : "not bound");
> +
> +	seq_printf(s, "%-40s %20s\n", "device name", "status");
> +	seq_puts(s, "-------------------------------------------------------------\n");
> +	for (i = 0; i < match->num; i++) {
> +		struct device *d = (struct device *)match->compare[i].data;
> +
> +		seq_printf(s, "%-40s %20s\n", dev_name(d),
> +			   match->compare[i].component ?
> +			   "registered" : "not registered");
> +	}
> +	mutex_unlock(&component_mutex);
> +
> +	return 0;
> +}
> +
> +static int component_devices_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, component_devices_show, inode->i_private);
> +}
> +
> +static const struct file_operations component_devices_fops = {
> +	.open = component_devices_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static int __init component_debug_init(void)
> +{
> +	component_debugfs_dir = debugfs_create_dir("component", NULL);
> +
> +	if (!component_debugfs_dir)
> +		return -ENOMEM;

No need to test this at all, you should never fail anything if debugfs
is not working properly, just continue and move on.  The result of any
debugfs call can be fed back into any other debugfs call without any
problems.

> +
> +	return 0;
> +}
> +
> +core_initcall(component_debug_init);
> +
> +static void component_master_debugfs_add(struct master *m)
> +{
> +	m->dentry = debugfs_create_file(dev_name(m->dev), 0444,
> +					component_debugfs_dir,
> +					m, &component_devices_fops);

See, you do it well here, do the same thing when you create the initial
debugfs directory.

Also, "component" is very vague, can you think of a better term for
this?  "device_component"?  "dev_component"?  Something else?  But I
don't care, if you really like "component", that's fine.

thanks,

greg k-h