[PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Maciej Matraszek]

Add /sys/kernel/debug/pm_genpd/pm_genpd_summary file, which
lists power domains in the system, their statuses and attached devices,
resembling /sys/kernel/debug/clk/clk_summary.

Currently it is impossible to inspect (from userland) whether
a power domain is on or off. And, if it is on, which device blocks it
from powering down. This change allows developers working on
embedded devices power efficiency to list all necessary information
about generic power domains in one place.

The content of pm_genpd/pm_genpd_summary file is generated by iterating
over all generic power domain in the system, and, for each,
over registered devices.

Example output:
$ cat  /sys/kernel/debug/pm_genpd/pm_genpd_summary
    domain                      status         slaves
           /device                                      runtime status
----------------------------------------------------------------------
isp-power-domain                off
    /devices/soc/12180000.fimc-is                       suspended
    /devices/soc/12180000.fimc-is/120a0000.fimc-lite    suspended
lcd0-power-domain               on
    /devices/soc/11c80000.dsi                           active
    /devices/soc/11c00000.fimd                          active
    /devices/soc/11e20000.sysmmu                        suspended
g3d-power-domain                off
    /devices/soc/13001000.gpu                           suspended
mfc-power-domain                off
    /devices/soc/13400000.mfc-codec                     suspended
    /devices/soc/13620000.sysmmu                        suspended
cam-power-domain                off
    /devices/soc/11830000.jpeg-codec                    suspended
    /devices/soc/11a60000.sysmmu                        suspended
    /devices/soc/12180000.fimc-is/120c0000.csis         suspended

To enable this feature, compile the kernel with debugfs
and CONFIG_PM_ADVANCED_DEBUG enabled.

Signed-off-by: Maciej Matraszek <m.matraszek@samsung.com>
---
 drivers/base/power/domain.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index eee55c1e5fde..d1aa731ba0db 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2189,3 +2189,156 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	list_add(&genpd->gpd_list_node, &gpd_list);
 	mutex_unlock(&gpd_list_lock);
 }
+
+
+/***        debugfs support        ***/
+
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+#include <linux/pm.h>
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/kobject.h>
+static struct dentry *pm_genpd_debugfs_dir;
+
+/*
+ * TODO: This function is a slightly modified version of rtpm_status_show
+ * from sysfs.c, but dependencies between PM_GENERIC_DOMAINS and PM_RUNTIME
+ * are too loose to generalize it.
+ */
+#ifdef CONFIG_PM_RUNTIME
+static void rtpm_status_str(struct seq_file *s, struct device *dev)
+{
+	static const char * const status_lookup[] = {
+		[RPM_ACTIVE] = "active",
+		[RPM_RESUMING] = "resuming",
+		[RPM_SUSPENDED] = "suspended",
+		[RPM_SUSPENDING] = "suspending"
+	};
+	const char *p = "";
+
+	if (dev->power.runtime_error)
+		p = "error";
+	else if (dev->power.disable_depth)
+		p = "unsupported";
+	else if (dev->power.runtime_status < ARRAY_SIZE(status_lookup))
+		p = status_lookup[dev->power.runtime_status];
+	else
+		WARN_ON(1);
+
+	seq_puts(s, p);
+}
+#else
+static void rtpm_status_str(struct seq_file *s, struct device *dev)
+{
+	seq_puts(s, "active");
+}
+#endif
+
+static int pm_genpd_summary_one(struct seq_file *s,
+		struct generic_pm_domain *gpd)
+{
+	static const char * const status_lookup[] = {
+		[GPD_STATE_ACTIVE] = "on",
+		[GPD_STATE_WAIT_MASTER] = "wait-master",
+		[GPD_STATE_BUSY] = "busy",
+		[GPD_STATE_REPEAT] = "off-in-progress",
+		[GPD_STATE_POWER_OFF] = "off"
+	};
+	struct pm_domain_data *pm_data;
+	const char *kobj_path;
+	struct gpd_link *link;
+	int ret;
+
+	ret = mutex_lock_interruptible(&gpd->lock);
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (WARN_ON(gpd->status >= ARRAY_SIZE(status_lookup)))
+		goto exit;
+	seq_printf(s, "%-30s  %-15s  ", gpd->name, status_lookup[gpd->status]);
+
+	/*
+	 * Modifications on the list require holding locks on both
+	 * master and slave, so we are safe.
+	 * Also gpd->name is immutable.
+	 */
+	list_for_each_entry(link, &gpd->master_links, master_node) {
+		seq_printf(s, "%s", link->slave->name);
+		if (!list_is_last(&link->master_node, &gpd->master_links))
+			seq_puts(s, ", ");
+	}
+
+	list_for_each_entry(pm_data, &gpd->dev_list, list_node) {
+		kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
+		if (kobj_path == NULL)
+			continue;
+
+		seq_printf(s, "\n    %-50s  ", kobj_path);
+		rtpm_status_str(s, pm_data->dev);
+		kfree(kobj_path);
+	}
+
+	seq_puts(s, "\n");
+exit:
+	mutex_unlock(&gpd->lock);
+
+	return 0;
+}
+
+static int pm_genpd_summary_show(struct seq_file *s, void *data)
+{
+	struct generic_pm_domain *gpd;
+	int ret = 0;
+
+	seq_puts(s, "    domain                      status         slaves\n");
+	seq_puts(s, "           /device                                      runtime status\n");
+	seq_puts(s, "----------------------------------------------------------------------\n");
+
+	ret = mutex_lock_interruptible(&gpd_list_lock);
+	if (ret)
+		return -ERESTARTSYS;
+
+	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+		ret = pm_genpd_summary_one(s, gpd);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
+
+static int pm_genpd_summary_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pm_genpd_summary_show, inode->i_private);
+}
+
+static const struct file_operations pm_genpd_summary_fops = {
+	.open = pm_genpd_summary_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static int __init pm_genpd_debug_init(void)
+{
+	struct dentry *d;
+
+	pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);
+
+	/* No debugfs */
+	if (IS_ERR(pm_genpd_debugfs_dir))
+		return PTR_ERR(pm_genpd_debugfs_dir);
+
+	if (!pm_genpd_debugfs_dir)
+		return -ENOMEM;
+
+	d = debugfs_create_file("pm_genpd_summary", S_IRUGO,
+			pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);
+	if (!d)
+		return -ENOMEM;
+
+	return 0;
+}
+late_initcall(pm_genpd_debug_init);
+#endif /* CONFIG_PM_ADVANCED_DEBUG */
-- 
1.9.1

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Greg Kroah-Hartman]

On Wed, Sep 10, 2014 at 01:28:56PM +0200, Maciej Matraszek wrote:
> Add /sys/kernel/debug/pm_genpd/pm_genpd_summary file, which
> lists power domains in the system, their statuses and attached devices,
> resembling /sys/kernel/debug/clk/clk_summary.
> 
> Currently it is impossible to inspect (from userland) whether
> a power domain is on or off. And, if it is on, which device blocks it
> from powering down. This change allows developers working on
> embedded devices power efficiency to list all necessary information
> about generic power domains in one place.
> 
> The content of pm_genpd/pm_genpd_summary file is generated by iterating
> over all generic power domain in the system, and, for each,
> over registered devices.
> 
> Example output:
> $ cat  /sys/kernel/debug/pm_genpd/pm_genpd_summary
>     domain                      status         slaves
>            /device                                      runtime status
> ----------------------------------------------------------------------
> isp-power-domain                off
>     /devices/soc/12180000.fimc-is                       suspended
>     /devices/soc/12180000.fimc-is/120a0000.fimc-lite    suspended
> lcd0-power-domain               on
>     /devices/soc/11c80000.dsi                           active
>     /devices/soc/11c00000.fimd                          active
>     /devices/soc/11e20000.sysmmu                        suspended
> g3d-power-domain                off
>     /devices/soc/13001000.gpu                           suspended
> mfc-power-domain                off
>     /devices/soc/13400000.mfc-codec                     suspended
>     /devices/soc/13620000.sysmmu                        suspended
> cam-power-domain                off
>     /devices/soc/11830000.jpeg-codec                    suspended
>     /devices/soc/11a60000.sysmmu                        suspended
>     /devices/soc/12180000.fimc-is/120c0000.csis         suspended
> 
> To enable this feature, compile the kernel with debugfs
> and CONFIG_PM_ADVANCED_DEBUG enabled.
> 
> Signed-off-by: Maciej Matraszek <m.matraszek@samsung.com>
> ---
>  drivers/base/power/domain.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index eee55c1e5fde..d1aa731ba0db 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2189,3 +2189,156 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>  	list_add(&genpd->gpd_list_node, &gpd_list);
>  	mutex_unlock(&gpd_list_lock);
>  }
> +
> +
> +/***        debugfs support        ***/
> +
> +#ifdef CONFIG_PM_ADVANCED_DEBUG
> +#include <linux/pm.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/kobject.h>
> +static struct dentry *pm_genpd_debugfs_dir;
> +
> +/*
> + * TODO: This function is a slightly modified version of rtpm_status_show
> + * from sysfs.c, but dependencies between PM_GENERIC_DOMAINS and PM_RUNTIME
> + * are too loose to generalize it.
> + */
> +#ifdef CONFIG_PM_RUNTIME
> +static void rtpm_status_str(struct seq_file *s, struct device *dev)
> +{
> +	static const char * const status_lookup[] = {
> +		[RPM_ACTIVE] = "active",
> +		[RPM_RESUMING] = "resuming",
> +		[RPM_SUSPENDED] = "suspended",
> +		[RPM_SUSPENDING] = "suspending"
> +	};
> +	const char *p = "";
> +
> +	if (dev->power.runtime_error)
> +		p = "error";
> +	else if (dev->power.disable_depth)
> +		p = "unsupported";
> +	else if (dev->power.runtime_status < ARRAY_SIZE(status_lookup))
> +		p = status_lookup[dev->power.runtime_status];
> +	else
> +		WARN_ON(1);
> +
> +	seq_puts(s, p);
> +}
> +#else
> +static void rtpm_status_str(struct seq_file *s, struct device *dev)
> +{
> +	seq_puts(s, "active");
> +}
> +#endif
> +
> +static int pm_genpd_summary_one(struct seq_file *s,
> +		struct generic_pm_domain *gpd)
> +{
> +	static const char * const status_lookup[] = {
> +		[GPD_STATE_ACTIVE] = "on",
> +		[GPD_STATE_WAIT_MASTER] = "wait-master",
> +		[GPD_STATE_BUSY] = "busy",
> +		[GPD_STATE_REPEAT] = "off-in-progress",
> +		[GPD_STATE_POWER_OFF] = "off"
> +	};
> +	struct pm_domain_data *pm_data;
> +	const char *kobj_path;
> +	struct gpd_link *link;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&gpd->lock);
> +	if (ret)
> +		return -ERESTARTSYS;
> +
> +	if (WARN_ON(gpd->status >= ARRAY_SIZE(status_lookup)))
> +		goto exit;
> +	seq_printf(s, "%-30s  %-15s  ", gpd->name, status_lookup[gpd->status]);
> +
> +	/*
> +	 * Modifications on the list require holding locks on both
> +	 * master and slave, so we are safe.
> +	 * Also gpd->name is immutable.
> +	 */
> +	list_for_each_entry(link, &gpd->master_links, master_node) {
> +		seq_printf(s, "%s", link->slave->name);
> +		if (!list_is_last(&link->master_node, &gpd->master_links))
> +			seq_puts(s, ", ");
> +	}
> +
> +	list_for_each_entry(pm_data, &gpd->dev_list, list_node) {
> +		kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
> +		if (kobj_path == NULL)
> +			continue;
> +
> +		seq_printf(s, "\n    %-50s  ", kobj_path);
> +		rtpm_status_str(s, pm_data->dev);
> +		kfree(kobj_path);
> +	}
> +
> +	seq_puts(s, "\n");
> +exit:
> +	mutex_unlock(&gpd->lock);
> +
> +	return 0;
> +}
> +
> +static int pm_genpd_summary_show(struct seq_file *s, void *data)
> +{
> +	struct generic_pm_domain *gpd;
> +	int ret = 0;
> +
> +	seq_puts(s, "    domain                      status         slaves\n");
> +	seq_puts(s, "           /device                                      runtime status\n");
> +	seq_puts(s, "----------------------------------------------------------------------\n");
> +
> +	ret = mutex_lock_interruptible(&gpd_list_lock);
> +	if (ret)
> +		return -ERESTARTSYS;
> +
> +	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +		ret = pm_genpd_summary_one(s, gpd);
> +		if (ret)
> +			break;
> +	}
> +	mutex_unlock(&gpd_list_lock);
> +
> +	return ret;
> +}
> +
> +static int pm_genpd_summary_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, pm_genpd_summary_show, inode->i_private);
> +}
> +
> +static const struct file_operations pm_genpd_summary_fops = {
> +	.open = pm_genpd_summary_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static int __init pm_genpd_debug_init(void)
> +{
> +	struct dentry *d;
> +
> +	pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);
> +
> +	/* No debugfs */
> +	if (IS_ERR(pm_genpd_debugfs_dir))
> +		return PTR_ERR(pm_genpd_debugfs_dir);

You shouldn't care at all, just do this check:

> +	if (!pm_genpd_debugfs_dir)
> +		return -ENOMEM;

And all will be fine, because:

> +	d = debugfs_create_file("pm_genpd_summary", S_IRUGO,
> +			pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);

That call will resolve away to nothing if debugfs is not enabled, and it
doesn't make much sense to do anything with an "error" of no debugfs.

> +	if (!d)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +late_initcall(pm_genpd_debug_init);
> +#endif /* CONFIG_PM_ADVANCED_DEBUG */

No tear-down of the debugfs files on shutdown?

thanks,

greg k-h

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Maciej Matraszek]

Hi Greg,
thanks for your reply!

> > +static int __init pm_genpd_debug_init(void)
> > +{
> > +	struct dentry *d;
> > +
> > +	pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);
> > +
> > +	/* No debugfs */
> > +	if (IS_ERR(pm_genpd_debugfs_dir))
> > +		return PTR_ERR(pm_genpd_debugfs_dir);
> 
> You shouldn't care at all, just do this check:
> 
> > +	if (!pm_genpd_debugfs_dir)
> > +		return -ENOMEM;
> 
> And all will be fine, because:
> 
> > +	d = debugfs_create_file("pm_genpd_summary", S_IRUGO,
> > +			pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);
> 
> That call will resolve away to nothing if debugfs is not enabled, and it
> doesn't make much sense to do anything with an "error" of no debugfs.

Great! I'll fix this bizarre double check.

> 
> > +	if (!d)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +late_initcall(pm_genpd_debug_init);
> > +#endif /* CONFIG_PM_ADVANCED_DEBUG */
> 
> No tear-down of the debugfs files on shutdown?

In drivers/clk/clk.c, there is no tear-down of debugfs files paired
with clk_debug_init(), same with wakeup_sources_debugfs_init() from
drivers/base/power/wakeup.c. On the other hand, some subsystems
perform it. What would you prefer?


> 
> thanks,
> 
> greg k-h

Thanks,
--
Maciej Matraszek
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Greg Kroah-Hartman]

On Wed, Sep 10, 2014 at 05:38:13PM +0200, Maciej Matraszek wrote:
> Hi Greg,
> thanks for your reply!
> 
> > > +static int __init pm_genpd_debug_init(void)
> > > +{
> > > +	struct dentry *d;
> > > +
> > > +	pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);
> > > +
> > > +	/* No debugfs */
> > > +	if (IS_ERR(pm_genpd_debugfs_dir))
> > > +		return PTR_ERR(pm_genpd_debugfs_dir);
> > 
> > You shouldn't care at all, just do this check:
> > 
> > > +	if (!pm_genpd_debugfs_dir)
> > > +		return -ENOMEM;
> > 
> > And all will be fine, because:
> > 
> > > +	d = debugfs_create_file("pm_genpd_summary", S_IRUGO,
> > > +			pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);
> > 
> > That call will resolve away to nothing if debugfs is not enabled, and it
> > doesn't make much sense to do anything with an "error" of no debugfs.
> 
> Great! I'll fix this bizarre double check.
> 
> > 
> > > +	if (!d)
> > > +		return -ENOMEM;
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall(pm_genpd_debug_init);
> > > +#endif /* CONFIG_PM_ADVANCED_DEBUG */
> > 
> > No tear-down of the debugfs files on shutdown?
> 
> In drivers/clk/clk.c, there is no tear-down of debugfs files paired
> with clk_debug_init(), same with wakeup_sources_debugfs_init() from
> drivers/base/power/wakeup.c. On the other hand, some subsystems
> perform it. What would you prefer?

In my quest for symmetry I prefer to see files you add to be removed at
the proper time. :)

thanks,

greg k-h

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Al Viro]

On Wed, Sep 10, 2014 at 01:28:56PM +0200, Maciej Matraszek wrote:

> +static int pm_genpd_summary_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, pm_genpd_summary_show, inode->i_private);
							^^^^^^^^^^^^^^^^
							what for?

Where in pm_genpd_summary_show() are you using ->private?

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Geert Uytterhoeven]

Hi Maciej,

On Wed, Sep 10, 2014 at 1:28 PM, Maciej Matraszek
<m.matraszek@samsung.com> wrote:
> Add /sys/kernel/debug/pm_genpd/pm_genpd_summary file, which
> lists power domains in the system, their statuses and attached devices,
> resembling /sys/kernel/debug/clk/clk_summary.
>
> Currently it is impossible to inspect (from userland) whether
> a power domain is on or off. And, if it is on, which device blocks it
> from powering down. This change allows developers working on
> embedded devices power efficiency to list all necessary information
> about generic power domains in one place.

Thanks, this is _very_ useful!

> The content of pm_genpd/pm_genpd_summary file is generated by iterating
> over all generic power domain in the system, and, for each,
> over registered devices.

... and over the subdomains, if present.

I can confirm that part is working, too, cfr. the slaves column on r8a7740

    domain                      status         slaves
           /device                                      runtime status
----------------------------------------------------------------------
a4su                            off
a3sg                            off
a3sm                            on
a3sp                            on
    /devices/e6600000.pwm                               suspended
    /devices/e6c50000.serial                            active
    /devices/e6850000.sd                                suspended
    /devices/e6bd0000.mmc                               active
a4s                             on               a3sp, a3sm, a3sg
    /devices/e6900000.irqpin                            unsupported
    /devices/e6900004.irqpin                            unsupported
    /devices/e6900008.irqpin                            unsupported
    /devices/e690000c.irqpin                            unsupported
    /devices/e9a00000.ethernet                          active
a3rv                            off
a4r                             off              a3rv
    /devices/fff20000.i2c                               suspended
a4lc                            off
c5                              on               a4lc, a4r, a4s, a4su
    /devices/e6050000.pfc                               unsupported
    /devices/e6138000.timer                             active

> Signed-off-by: Maciej Matraszek <m.matraszek@samsung.com>

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Maciej Matraszek]

Hi Geert!

On Thu, 2014-09-11 at 10:51 +0200, Geert Uytterhoeven wrote:
> Hi Maciej,
> 
> On Wed, Sep 10, 2014 at 1:28 PM, Maciej Matraszek
> <m.matraszek@samsung.com> wrote:
> > Add /sys/kernel/debug/pm_genpd/pm_genpd_summary file, which
> > lists power domains in the system, their statuses and attached devices,
> > resembling /sys/kernel/debug/clk/clk_summary.
> >
> > Currently it is impossible to inspect (from userland) whether
> > a power domain is on or off. And, if it is on, which device blocks it
> > from powering down. This change allows developers working on
> > embedded devices power efficiency to list all necessary information
> > about generic power domains in one place.
> 
> Thanks, this is _very_ useful!
> 
> > The content of pm_genpd/pm_genpd_summary file is generated by iterating
> > over all generic power domain in the system, and, for each,
> > over registered devices.
> 
> ... and over the subdomains, if present.
> 
> I can confirm that part is working, too, cfr. the slaves column on r8a7740
> 
>     domain                      status         slaves
>            /device                                      runtime status
> ----------------------------------------------------------------------
> a4su                            off
> a3sg                            off
> a3sm                            on
> a3sp                            on
>     /devices/e6600000.pwm                               suspended
>     /devices/e6c50000.serial                            active
>     /devices/e6850000.sd                                suspended
>     /devices/e6bd0000.mmc                               active
> a4s                             on               a3sp, a3sm, a3sg
>     /devices/e6900000.irqpin                            unsupported
>     /devices/e6900004.irqpin                            unsupported
>     /devices/e6900008.irqpin                            unsupported
>     /devices/e690000c.irqpin                            unsupported
>     /devices/e9a00000.ethernet                          active
> a3rv                            off
> a4r                             off              a3rv
>     /devices/fff20000.i2c                               suspended
> a4lc                            off
> c5                              on               a4lc, a4r, a4s, a4su
>     /devices/e6050000.pfc                               unsupported
>     /devices/e6138000.timer                             active

> > Signed-off-by: Maciej Matraszek <m.matraszek@samsung.com>
> 
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks for testing, Geert, I'm glad you like it :)!
BTW Can I use your output as example in the commit message for v2?
It's much more informative (though 'runtime status' alignment seems
odd).

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Maciej Matraszek
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Maciej Matraszek]

Hi Al!

On śro, 2014-09-10 at 20:18 +0100, Al Viro wrote:
> On Wed, Sep 10, 2014 at 01:28:56PM +0200, Maciej Matraszek wrote:
> 
> > +static int pm_genpd_summary_open(struct inode *inode, struct file *file)
> > +{
> > +	return single_open(file, pm_genpd_summary_show, inode->i_private);
> 							^^^^^^^^^^^^^^^^
> 							what for?
> 
> Where in pm_genpd_summary_show() are you using ->private?

Nowhere, thanks! (Just a mindless copy-paste from clk_summary_open().)

Regards,
-- 
Maciej Matraszek
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Maciej Matraszek]

On Wed, 2014-09-10 at 11:27 -0700, Greg Kroah-Hartman wrote:
> > > 
> > > > +	if (!d)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +late_initcall(pm_genpd_debug_init);
> > > > +#endif /* CONFIG_PM_ADVANCED_DEBUG */
> > > 
> > > No tear-down of the debugfs files on shutdown?
> > 
> > In drivers/clk/clk.c, there is no tear-down of debugfs files paired
> > with clk_debug_init(), same with wakeup_sources_debugfs_init() from
> > drivers/base/power/wakeup.c. On the other hand, some subsystems
> > perform it. What would you prefer?
> 
> In my quest for symmetry I prefer to see files you add to be removed at
> the proper time. :)

And they will be! (in v2 :)


Thanks,
-- 
Maciej Matraszek
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Geert Uytterhoeven]

Hi Maciej,

On Thu, Sep 11, 2014 at 6:45 PM, Maciej Matraszek
<m.matraszek@samsung.com> wrote:
> BTW Can I use your output as example in the commit message for v2?
> It's much more informative

Sure, no problem.

> (though 'runtime status' alignment seems odd).

What's wrong with it?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Maciej Matraszek]

On czw, 2014-09-11 at 19:27 +0200, Geert Uytterhoeven wrote:
> Hi Maciej,
> 
> On Thu, Sep 11, 2014 at 6:45 PM, Maciej Matraszek
> <m.matraszek@samsung.com> wrote:
> > BTW Can I use your output as example in the commit message for v2?
> > It's much more informative
> 
> Sure, no problem.
> 
> > (though 'runtime status' alignment seems odd).
> 
> What's wrong with it?

Nothing big, just for such short device names it doesn't look aesthetic to me :).

Regards,
-- 
Maciej Matraszek
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] PM / Domains: add debugfs listing of struct generic_pm_domain-s

[Geert Uytterhoeven]

Hi Maciej,

On Fri, Sep 12, 2014 at 6:19 PM, Maciej Matraszek
<m.matraszek@samsung.com> wrote:
>> > (though 'runtime status' alignment seems odd).
>>
>> What's wrong with it?
>
> Nothing big, just for such short device names it doesn't look aesthetic to me :).

In legacy mode, using platform code (without DT), the device names are
longer :-)

BTW, the list of slaves is indented by two more spaces than the column title:

    domain                      status         slaves
a4s                             on               a3sp, a3sm, a3sg

This is also the case in V2.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds