Re: [PATCH v2] component: add debugfs support

[Maciej Purski]

On 11/15/2017 03:01 PM, Greg Kroah-Hartman wrote:
> 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@xxxxxxxxxxx>
> > ---
> > 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.

I'll to this the way you suggest. Thanks.

> 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.

I'm not really attached to that name, so I can change it to one that you suggested.
Maybe Russell King has some preference on that?

Best regards,

Maciej Purski

> thanks,
>
> greg k-h