[PATCH 0/2] pm_trace: Add locking and add sysfs attr for rechecking dev hash.

[PATCH 0/2] pm_trace: Add locking and add sysfs attr for rechecking dev hash.

[James Hogan]

A fix and an improvement to the pm_trace code.

The improvement allows finding out via sysfs which device was last traced when 
the device is part of a kernel module (which means it doesn't get printed on 
next boot since it isn't loaded yet).

Unsure as always about what to name stuff, so comments most welcome.

James Hogan (2):
  pm_trace: Lock pm device list mutex.
  pm_trace: Add sysfs attr for rechecking dev hash.

 Documentation/power/s2ram.txt |    7 +++++++
 drivers/base/power/trace.c    |   32 +++++++++++++++++++++++++++++++-
 include/linux/resume-trace.h  |    2 ++
 kernel/power/main.c           |   18 ++++++++++++++++++
 4 files changed, 58 insertions(+), 1 deletions(-)

-- 
1.7.2.3

[PATCH 1/2] pm_trace: Lock pm device list mutex.

[James Hogan]

Lock the pm device list mutex using device_pm_lock() and
device_pm_unlock() around the list iteration in show_dev_hash().

show_dev_hash() was reverse iterating dpm_list without first locking the
mutex that the functions in drivers/base/power/main.c lock. I assume
this was unintentional since there is no comment suggesting why the lock
might not be necessary.

Signed-off-by: James Hogan <james@albanarts.com>
---
 drivers/base/power/trace.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index 0a1a2c4..17e24e3 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c
@@ -188,8 +188,10 @@ static int show_file_hash(unsigned int value)
 static int show_dev_hash(unsigned int value)
 {
 	int match = 0;
-	struct list_head *entry = dpm_list.prev;
+	struct list_head *entry;
 
+	device_pm_lock();
+	entry = dpm_list.prev;
 	while (entry != &dpm_list) {
 		struct device * dev = to_device(entry);
 		unsigned int hash = hash_string(DEVSEED, dev_name(dev), DEVHASH);
@@ -199,6 +201,7 @@ static int show_dev_hash(unsigned int value)
 		}
 		entry = entry->prev;
 	}
+	device_pm_unlock();
 	return match;
 }
 
-- 
1.7.2.3

Re: [PATCH 1/2] pm_trace: Lock pm device list mutex.

[Rafael J. Wysocki]

On Sunday, October 10, 2010, James Hogan wrote:
> Lock the pm device list mutex using device_pm_lock() and
> device_pm_unlock() around the list iteration in show_dev_hash().
> 
> show_dev_hash() was reverse iterating dpm_list without first locking the
> mutex that the functions in drivers/base/power/main.c lock. I assume
> this was unintentional since there is no comment suggesting why the lock
> might not be necessary.
> 
> Signed-off-by: James Hogan <james@albanarts.com>

Looks good.

> ---
>  drivers/base/power/trace.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
> index 0a1a2c4..17e24e3 100644
> --- a/drivers/base/power/trace.c
> +++ b/drivers/base/power/trace.c
> @@ -188,8 +188,10 @@ static int show_file_hash(unsigned int value)
>  static int show_dev_hash(unsigned int value)
>  {
>  	int match = 0;
> -	struct list_head *entry = dpm_list.prev;
> +	struct list_head *entry;
>  
> +	device_pm_lock();
> +	entry = dpm_list.prev;
>  	while (entry != &dpm_list) {
>  		struct device * dev = to_device(entry);
>  		unsigned int hash = hash_string(DEVSEED, dev_name(dev), DEVHASH);
> @@ -199,6 +201,7 @@ static int show_dev_hash(unsigned int value)
>  		}
>  		entry = entry->prev;
>  	}
> +	device_pm_unlock();
>  	return match;
>  }
>  
> 

[PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[James Hogan]

If the device which fails to resume is part of a loadable kernel module
it won't be checked at startup against the magic number stored in the
RTC.

Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
contains a list of newline separated devices (usually just the one)
which currently match the last magic number. This allows the device
which is failing to resume to be found after the modules are loaded
again.

Signed-off-by: James Hogan <james@albanarts.com>
---
 Documentation/power/s2ram.txt |    7 +++++++
 drivers/base/power/trace.c    |   27 +++++++++++++++++++++++++++
 include/linux/resume-trace.h  |    2 ++
 kernel/power/main.c           |   18 ++++++++++++++++++
 4 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/s2ram.txt b/Documentation/power/s2ram.txt
index 514b94f..3a2801a 100644
--- a/Documentation/power/s2ram.txt
+++ b/Documentation/power/s2ram.txt
@@ -49,6 +49,13 @@ machine that doesn't boot) is:
    device (lspci and /sys/devices/pci* is your friend), and see if you can
    fix it, disable it, or trace into its resume function.
 
+   If no device matches the hash, it may be a device from a loadable kernel
+   module that is not loaded until after the hash is checked. You can check
+   the hash against the current devices again after more modules are loaded
+   using sysfs:
+
+	cat /sys/power/pm_trace_dev_hash
+
 For example, the above happens to be the VGA device on my EVO, which I
 used to run with "radeonfb" (it's an ATI Radeon mobility). It turns out
 that "radeonfb" simply cannot resume that device - it tries to set the
diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index 17e24e3..e0cdba1 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c
@@ -207,6 +207,33 @@ static int show_dev_hash(unsigned int value)
 
 static unsigned int hash_value_early_read;
 
+int snprint_trace_dev_hash(char *buf, size_t size)
+{
+	unsigned int value = hash_value_early_read / (USERHASH * FILEHASH);
+	int ret = 0;
+	struct list_head *entry;
+
+	device_pm_lock();
+	entry = dpm_list.prev;
+	while (size && entry != &dpm_list) {
+		struct device *dev = to_device(entry);
+		unsigned int hash = hash_string(DEVSEED, dev_name(dev),
+						DEVHASH);
+		if (hash == value) {
+			int len = snprintf(buf, size, "%s\n",
+					    dev_driver_string(dev));
+			if (len > size)
+				len = size;
+			buf += len;
+			ret += len;
+			size -= len;
+		}
+		entry = entry->prev;
+	}
+	device_pm_unlock();
+	return ret;
+}
+
 static int early_resume_init(void)
 {
 	hash_value_early_read = read_magic_time();
diff --git a/include/linux/resume-trace.h b/include/linux/resume-trace.h
index bc8c388..02b114c 100644
--- a/include/linux/resume-trace.h
+++ b/include/linux/resume-trace.h
@@ -3,6 +3,7 @@
 
 #ifdef CONFIG_PM_TRACE
 #include <asm/resume-trace.h>
+#include <linux/types.h>
 
 extern int pm_trace_enabled;
 
@@ -14,6 +15,7 @@ static inline int pm_trace_is_enabled(void)
 struct device;
 extern void set_trace_device(struct device *);
 extern void generate_resume_trace(const void *tracedata, unsigned int user);
+extern int snprint_trace_dev_hash(char *buf, size_t size);
 
 #define TRACE_DEVICE(dev) do { \
 	if (pm_trace_enabled) \
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 62b0bc6..42f8d1d 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -281,12 +281,30 @@ pm_trace_store(struct kobject *kobj, struct kobj_attribute *attr,
 }
 
 power_attr(pm_trace);
+
+static ssize_t pm_trace_dev_hash_show(struct kobject *kobj,
+				  struct kobj_attribute *attr,
+				  char *buf)
+{
+	return snprint_trace_dev_hash(buf, PAGE_SIZE);
+}
+
+static ssize_t
+pm_trace_dev_hash_store(struct kobject *kobj, struct kobj_attribute *attr,
+	       const char *buf, size_t n)
+{
+	return -EINVAL;
+}
+
+power_attr(pm_trace_dev_hash);
+
 #endif /* CONFIG_PM_TRACE */
 
 static struct attribute * g[] = {
 	&state_attr.attr,
 #ifdef CONFIG_PM_TRACE
 	&pm_trace_attr.attr,
+	&pm_trace_dev_hash_attr.attr,
 #endif
 #ifdef CONFIG_PM_SLEEP
 	&pm_async_attr.attr,
-- 
1.7.2.3

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[Rafael J. Wysocki]

On Sunday, October 10, 2010, James Hogan wrote:
> If the device which fails to resume is part of a loadable kernel module
> it won't be checked at startup against the magic number stored in the
> RTC.
> 
> Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> contains a list of newline separated devices (usually just the one)
> which currently match the last magic number. This allows the device
> which is failing to resume to be found after the modules are loaded
> again.
> 
> Signed-off-by: James Hogan <james@albanarts.com>
> ---
>  Documentation/power/s2ram.txt |    7 +++++++
>  drivers/base/power/trace.c    |   27 +++++++++++++++++++++++++++
>  include/linux/resume-trace.h  |    2 ++
>  kernel/power/main.c           |   18 ++++++++++++++++++
>  4 files changed, 54 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/power/s2ram.txt b/Documentation/power/s2ram.txt
> index 514b94f..3a2801a 100644
> --- a/Documentation/power/s2ram.txt
> +++ b/Documentation/power/s2ram.txt
> @@ -49,6 +49,13 @@ machine that doesn't boot) is:
>     device (lspci and /sys/devices/pci* is your friend), and see if you can
>     fix it, disable it, or trace into its resume function.
>  
> +   If no device matches the hash, it may be a device from a loadable kernel
> +   module that is not loaded until after the hash is checked. You can check
> +   the hash against the current devices again after more modules are loaded
> +   using sysfs:
> +
> +	cat /sys/power/pm_trace_dev_hash
> +

/sys/power/pm_trace_match perhaps?

How do we ensure it prints things that make sense when the last resume was
successful or the system hasn't suspended at all?

>  For example, the above happens to be the VGA device on my EVO, which I
>  used to run with "radeonfb" (it's an ATI Radeon mobility). It turns out
>  that "radeonfb" simply cannot resume that device - it tries to set the
> diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
> index 17e24e3..e0cdba1 100644
> --- a/drivers/base/power/trace.c
> +++ b/drivers/base/power/trace.c
> @@ -207,6 +207,33 @@ static int show_dev_hash(unsigned int value)
>  
>  static unsigned int hash_value_early_read;
>  
> +int snprint_trace_dev_hash(char *buf, size_t size)
> +{
> +	unsigned int value = hash_value_early_read / (USERHASH * FILEHASH);
> +	int ret = 0;
> +	struct list_head *entry;
> +
> +	device_pm_lock();
> +	entry = dpm_list.prev;
> +	while (size && entry != &dpm_list) {
> +		struct device *dev = to_device(entry);
> +		unsigned int hash = hash_string(DEVSEED, dev_name(dev),
> +						DEVHASH);
> +		if (hash == value) {
> +			int len = snprintf(buf, size, "%s\n",
> +					    dev_driver_string(dev));
> +			if (len > size)
> +				len = size;
> +			buf += len;
> +			ret += len;
> +			size -= len;

Don't we want to break; here and if so then why?

> +		}
> +		entry = entry->prev;
> +	}
> +	device_pm_unlock();
> +	return ret;
> +}
> +
>  static int early_resume_init(void)
>  {
>  	hash_value_early_read = read_magic_time();

Thanks,
Rafael

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[James Hogan]

Thanks for taking a look...

On Saturday 09 October 2010 23:49:15 Rafael J. Wysocki wrote:
> On Sunday, October 10, 2010, James Hogan wrote:
> > If the device which fails to resume is part of a loadable kernel module
> > it won't be checked at startup against the magic number stored in the
> > RTC.
> > 
> > Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> > contains a list of newline separated devices (usually just the one)
> > which currently match the last magic number. This allows the device
> > which is failing to resume to be found after the modules are loaded
> > again.
> > 
> > Signed-off-by: James Hogan <james@albanarts.com>
> > ---
> > 
> >  Documentation/power/s2ram.txt |    7 +++++++
> >  drivers/base/power/trace.c    |   27 +++++++++++++++++++++++++++
> >  include/linux/resume-trace.h  |    2 ++
> >  kernel/power/main.c           |   18 ++++++++++++++++++
> >  4 files changed, 54 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/power/s2ram.txt
> > b/Documentation/power/s2ram.txt index 514b94f..3a2801a 100644
> > --- a/Documentation/power/s2ram.txt
> > +++ b/Documentation/power/s2ram.txt
> > 
> > @@ -49,6 +49,13 @@ machine that doesn't boot) is:
> >     device (lspci and /sys/devices/pci* is your friend), and see if you
> >     can fix it, disable it, or trace into its resume function.
> > 
> > +   If no device matches the hash, it may be a device from a loadable
> > kernel +   module that is not loaded until after the hash is checked.
> > You can check +   the hash against the current devices again after more
> > modules are loaded +   using sysfs:
> > +
> > +	cat /sys/power/pm_trace_dev_hash
> > +
> 
> /sys/power/pm_trace_match perhaps?

The magic number stores 1 "user" number (given in a RESUME_TRACE call) and 2 
hashes (representing source file/line and device) in the RTC, but this sysfs 
attribute only returns the matches for the device part, so I think it's 
important to have dev or device in there in case we want attributes for 
file/line (which doesn't work with modules at the moment either, but the 
"user" number can be used as that's printed at boot directly), but I agree 
that match is better than hash so I'm happy to change it to pm_trace_dev_match 
or pm_trace_dev_matches.

> 
> How do we ensure it prints things that make sense when the last resume was
> successful or the system hasn't suspended at all?

If the last resume was successful, then the stored magic number won't have 
changed since the original boot, since it is read from the RTC in a 
core_initcall function (early_resume_init()).

The case of when pm_trace wasn't in use before boot is impossible to avoid 
when using the RTC. There is a chance that a genuine RTC value will not match 
any of the device hashes (there are 1009 possible device hash values), in 
which case nothing will be output, but the same thing happens at boot when it 
does it's first comparison against the devices and printk's any matches.

> 
> >  For example, the above happens to be the VGA device on my EVO, which I
> >  used to run with "radeonfb" (it's an ATI Radeon mobility). It turns out
> >  that "radeonfb" simply cannot resume that device - it tries to set the
> > 
> > diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
> > index 17e24e3..e0cdba1 100644
> > --- a/drivers/base/power/trace.c
> > +++ b/drivers/base/power/trace.c
> > @@ -207,6 +207,33 @@ static int show_dev_hash(unsigned int value)
> > 
> >  static unsigned int hash_value_early_read;
> > 
> > +int snprint_trace_dev_hash(char *buf, size_t size)
> > +{
> > +	unsigned int value = hash_value_early_read / (USERHASH * FILEHASH);
> > +	int ret = 0;
> > +	struct list_head *entry;
> > +
> > +	device_pm_lock();
> > +	entry = dpm_list.prev;
> > +	while (size && entry != &dpm_list) {
> > +		struct device *dev = to_device(entry);
> > +		unsigned int hash = hash_string(DEVSEED, dev_name(dev),
> > +						DEVHASH);
> > +		if (hash == value) {
> > +			int len = snprintf(buf, size, "%s\n",
> > +					    dev_driver_string(dev));
> > +			if (len > size)
> > +				len = size;
> > +			buf += len;
> > +			ret += len;
> > +			size -= len;
> 
> Don't we want to break; here and if so then why?

No, because it's possible that two devices will hash to the same value, in 
which case it is better to print both out so we know that the problem could be 
in either one of them. I'll add a comment to that effect.

Thanks
James

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[Rafael J. Wysocki]

On Sunday, October 10, 2010, James Hogan wrote:
> Thanks for taking a look...
> 
> On Saturday 09 October 2010 23:49:15 Rafael J. Wysocki wrote:
> > On Sunday, October 10, 2010, James Hogan wrote:
> > > If the device which fails to resume is part of a loadable kernel module
> > > it won't be checked at startup against the magic number stored in the
> > > RTC.
> > > 
> > > Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> > > contains a list of newline separated devices (usually just the one)
> > > which currently match the last magic number. This allows the device
> > > which is failing to resume to be found after the modules are loaded
> > > again.
> > > 
> > > Signed-off-by: James Hogan <james@albanarts.com>
> > > ---
> > > 
> > >  Documentation/power/s2ram.txt |    7 +++++++
> > >  drivers/base/power/trace.c    |   27 +++++++++++++++++++++++++++
> > >  include/linux/resume-trace.h  |    2 ++
> > >  kernel/power/main.c           |   18 ++++++++++++++++++
> > >  4 files changed, 54 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/Documentation/power/s2ram.txt
> > > b/Documentation/power/s2ram.txt index 514b94f..3a2801a 100644
> > > --- a/Documentation/power/s2ram.txt
> > > +++ b/Documentation/power/s2ram.txt
> > > 
> > > @@ -49,6 +49,13 @@ machine that doesn't boot) is:
> > >     device (lspci and /sys/devices/pci* is your friend), and see if you
> > >     can fix it, disable it, or trace into its resume function.
> > > 
> > > +   If no device matches the hash, it may be a device from a loadable
> > > kernel +   module that is not loaded until after the hash is checked.
> > > You can check +   the hash against the current devices again after more
> > > modules are loaded +   using sysfs:
> > > +
> > > +	cat /sys/power/pm_trace_dev_hash
> > > +
> > 
> > /sys/power/pm_trace_match perhaps?
> 
> The magic number stores 1 "user" number (given in a RESUME_TRACE call) and 2 
> hashes (representing source file/line and device) in the RTC, but this sysfs 
> attribute only returns the matches for the device part, so I think it's 
> important to have dev or device in there in case we want attributes for 
> file/line (which doesn't work with modules at the moment either, but the 
> "user" number can be used as that's printed at boot directly), but I agree 
> that match is better than hash so I'm happy to change it to pm_trace_dev_match 
> or pm_trace_dev_matches.

I'd prefer pm_trace_dev_match.

> > 
> > How do we ensure it prints things that make sense when the last resume was
> > successful or the system hasn't suspended at all?
> 
> If the last resume was successful, then the stored magic number won't have 
> changed since the original boot, since it is read from the RTC in a 
> core_initcall function (early_resume_init()).
> 
> The case of when pm_trace wasn't in use before boot is impossible to avoid 
> when using the RTC. There is a chance that a genuine RTC value will not match 
> any of the device hashes (there are 1009 possible device hash values), in 
> which case nothing will be output, but the same thing happens at boot when it 
> does it's first comparison against the devices and printk's any matches.

OK

I think it would make sense to document that this somehow.  Like for example
in Documentation/ABI/testing/sysfs-power (you should add the new attribute to
the list in there anyway).

> > 
> > >  For example, the above happens to be the VGA device on my EVO, which I
> > >  used to run with "radeonfb" (it's an ATI Radeon mobility). It turns out
> > >  that "radeonfb" simply cannot resume that device - it tries to set the
> > > 
> > > diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
> > > index 17e24e3..e0cdba1 100644
> > > --- a/drivers/base/power/trace.c
> > > +++ b/drivers/base/power/trace.c
> > > @@ -207,6 +207,33 @@ static int show_dev_hash(unsigned int value)
> > > 
> > >  static unsigned int hash_value_early_read;
> > > 
> > > +int snprint_trace_dev_hash(char *buf, size_t size)
> > > +{
> > > +	unsigned int value = hash_value_early_read / (USERHASH * FILEHASH);
> > > +	int ret = 0;
> > > +	struct list_head *entry;
> > > +
> > > +	device_pm_lock();
> > > +	entry = dpm_list.prev;
> > > +	while (size && entry != &dpm_list) {
> > > +		struct device *dev = to_device(entry);
> > > +		unsigned int hash = hash_string(DEVSEED, dev_name(dev),
> > > +						DEVHASH);
> > > +		if (hash == value) {
> > > +			int len = snprintf(buf, size, "%s\n",
> > > +					    dev_driver_string(dev));
> > > +			if (len > size)
> > > +				len = size;
> > > +			buf += len;
> > > +			ret += len;
> > > +			size -= len;
> > 
> > Don't we want to break; here and if so then why?
> 
> No, because it's possible that two devices will hash to the same value, in 
> which case it is better to print both out so we know that the problem could be 
> in either one of them. I'll add a comment to that effect.

OK

Thanks,
Rafael

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[Pavel Machek]

Hi!

> If the device which fails to resume is part of a loadable kernel module
> it won't be checked at startup against the magic number stored in the
> RTC.
> 
> Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> contains a list of newline separated devices (usually just the one)
> which currently match the last magic number. This allows the device
> which is failing to resume to be found after the modules are loaded
> again.

> --- a/Documentation/power/s2ram.txt
> +++ b/Documentation/power/s2ram.txt
> @@ -49,6 +49,13 @@ machine that doesn't boot) is:
>     device (lspci and /sys/devices/pci* is your friend), and see if you can
>     fix it, disable it, or trace into its resume function.
>  
> +   If no device matches the hash, it may be a device from a loadable kernel
> +   module that is not loaded until after the hash is checked. You can check
> +   the hash against the current devices again after more modules are loaded
> +   using sysfs:
> +
> +	cat /sys/power/pm_trace_dev_hash
> +

Yep, but exact semantics of that sysfs file should probably be linked
in the sysfs documentation...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[James Hogan]

Hi Pavel,

On Sunday 10 October 2010 19:04:00 Pavel Machek wrote:
> Hi!
> 
> > If the device which fails to resume is part of a loadable kernel module
> > it won't be checked at startup against the magic number stored in the
> > RTC.
> > 
> > Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> > contains a list of newline separated devices (usually just the one)
> > which currently match the last magic number. This allows the device
> > which is failing to resume to be found after the modules are loaded
> > again.
> > 
> > --- a/Documentation/power/s2ram.txt
> > +++ b/Documentation/power/s2ram.txt
> > 
> > @@ -49,6 +49,13 @@ machine that doesn't boot) is:
> >     device (lspci and /sys/devices/pci* is your friend), and see if you
> >     can fix it, disable it, or trace into its resume function.
> > 
> > +   If no device matches the hash, it may be a device from a loadable
> > kernel +   module that is not loaded until after the hash is checked.
> > You can check +   the hash against the current devices again after more
> > modules are loaded +   using sysfs:
> > +
> > +	cat /sys/power/pm_trace_dev_hash
> > +
> 
> Yep, but exact semantics of that sysfs file should probably be linked
> in the sysfs documentation...
> 								Pavel

To clarify, do you mean I should link to Documentation/ABI/testing/sysfs-power 
from Documentation/power/s2ram.txt, or just make sure the syfs file is 
documented in Documentation/ABI/testing/sysfs-power (which is done in v2 of 
this patch)?

Thanks for taking a look
James

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[Pavel Machek]

On Sun 2010-10-10 20:47:01, James Hogan wrote:
> Hi Pavel,
> 
> On Sunday 10 October 2010 19:04:00 Pavel Machek wrote:
> > Hi!
> > 
> > > If the device which fails to resume is part of a loadable kernel module
> > > it won't be checked at startup against the magic number stored in the
> > > RTC.
> > > 
> > > Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> > > contains a list of newline separated devices (usually just the one)
> > > which currently match the last magic number. This allows the device
> > > which is failing to resume to be found after the modules are loaded
> > > again.
> > > 
> > > --- a/Documentation/power/s2ram.txt
> > > +++ b/Documentation/power/s2ram.txt
> > > 
> > > @@ -49,6 +49,13 @@ machine that doesn't boot) is:
> > >     device (lspci and /sys/devices/pci* is your friend), and see if you
> > >     can fix it, disable it, or trace into its resume function.
> > > 
> > > +   If no device matches the hash, it may be a device from a loadable
> > > kernel +   module that is not loaded until after the hash is checked.
> > > You can check +   the hash against the current devices again after more
> > > modules are loaded +   using sysfs:
> > > +
> > > +	cat /sys/power/pm_trace_dev_hash
> > > +
> > 
> > Yep, but exact semantics of that sysfs file should probably be linked
> > in the sysfs documentation...
> > 								Pavel
> 
> To clarify, do you mean I should link to Documentation/ABI/testing/sysfs-power 
> from Documentation/power/s2ram.txt, or just make sure the syfs file is 
> documented in Documentation/ABI/testing/sysfs-power (which is done in v2 of 
> this patch)?

v2 of the patch is probably ok.

Also, sysfs should be one entry per file, and strictly speaking, this
one is not. That may be fine... but as this is debugging facility,
perhaps it should go to debugfs? Maybe cc gregkh...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[James Hogan]

On Sunday 10 October 2010 21:47:19 Pavel Machek wrote:
> On Sun 2010-10-10 20:47:01, James Hogan wrote:
> > Hi Pavel,
> > 
> > On Sunday 10 October 2010 19:04:00 Pavel Machek wrote:
> > > Hi!
> > > 
> > > > If the device which fails to resume is part of a loadable kernel
> > > > module it won't be checked at startup against the magic number
> > > > stored in the RTC.
> > > > 
> > > > Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> > > > contains a list of newline separated devices (usually just the one)
> > > > which currently match the last magic number. This allows the device
> > > > which is failing to resume to be found after the modules are loaded
> > > > again.
> > > > 
> > > > --- a/Documentation/power/s2ram.txt
> > > > +++ b/Documentation/power/s2ram.txt
> > > > 
> > > > @@ -49,6 +49,13 @@ machine that doesn't boot) is:
> > > >     device (lspci and /sys/devices/pci* is your friend), and see if
> > > >     you can fix it, disable it, or trace into its resume function.
> > > > 
> > > > +   If no device matches the hash, it may be a device from a loadable
> > > > kernel +   module that is not loaded until after the hash is checked.
> > > > You can check +   the hash against the current devices again after
> > > > more modules are loaded +   using sysfs:
> > > > +
> > > > +	cat /sys/power/pm_trace_dev_hash
> > > > +
> > > 
> > > Yep, but exact semantics of that sysfs file should probably be linked
> > > in the sysfs documentation...
> > > 
> > > 								Pavel
> > 
> > To clarify, do you mean I should link to
> > Documentation/ABI/testing/sysfs-power from
> > Documentation/power/s2ram.txt, or just make sure the syfs file is
> > documented in Documentation/ABI/testing/sysfs-power (which is done in v2
> > of this patch)?
> 
> v2 of the patch is probably ok.
> 
> Also, sysfs should be one entry per file, and strictly speaking, this
> one is not. That may be fine... but as this is debugging facility,
> perhaps it should go to debugfs? Maybe cc gregkh...
> 									Pavel

I thought about the one entry per file issue.
Documentation/filesystems/sysfs.txt says:
> It is noted that it may not be efficient to contain only one
> value per file, so it is socially acceptable to express an array of
> values of the same type.

which appears to cover my use of it as they're all the same type (and 
considering that ideally there will be exactly 1 device listed anyway).

I wonder if I should separate them with spaces rather than newlines too 
(newlines just seemed more appropriate for a variable sized list at the time).

I ackowledge your point that debugfs may be a more appropriate place for it, 
but would that also apply to the pm_trace file?

(gregkh is cc'd)

Thanks
James

Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

[Rafael J. Wysocki]

On Sunday, October 10, 2010, James Hogan wrote:
> On Sunday 10 October 2010 21:47:19 Pavel Machek wrote:
> > On Sun 2010-10-10 20:47:01, James Hogan wrote:
> > > Hi Pavel,
> > > 
> > > On Sunday 10 October 2010 19:04:00 Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > If the device which fails to resume is part of a loadable kernel
> > > > > module it won't be checked at startup against the magic number
> > > > > stored in the RTC.
> > > > > 
> > > > > Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> > > > > contains a list of newline separated devices (usually just the one)
> > > > > which currently match the last magic number. This allows the device
> > > > > which is failing to resume to be found after the modules are loaded
> > > > > again.
> > > > > 
> > > > > --- a/Documentation/power/s2ram.txt
> > > > > +++ b/Documentation/power/s2ram.txt
> > > > > 
> > > > > @@ -49,6 +49,13 @@ machine that doesn't boot) is:
> > > > >     device (lspci and /sys/devices/pci* is your friend), and see if
> > > > >     you can fix it, disable it, or trace into its resume function.
> > > > > 
> > > > > +   If no device matches the hash, it may be a device from a loadable
> > > > > kernel +   module that is not loaded until after the hash is checked.
> > > > > You can check +   the hash against the current devices again after
> > > > > more modules are loaded +   using sysfs:
> > > > > +
> > > > > +	cat /sys/power/pm_trace_dev_hash
> > > > > +
> > > > 
> > > > Yep, but exact semantics of that sysfs file should probably be linked
> > > > in the sysfs documentation...
> > > > 
> > > > 								Pavel
> > > 
> > > To clarify, do you mean I should link to
> > > Documentation/ABI/testing/sysfs-power from
> > > Documentation/power/s2ram.txt, or just make sure the syfs file is
> > > documented in Documentation/ABI/testing/sysfs-power (which is done in v2
> > > of this patch)?
> > 
> > v2 of the patch is probably ok.
> > 
> > Also, sysfs should be one entry per file, and strictly speaking, this
> > one is not. That may be fine... but as this is debugging facility,
> > perhaps it should go to debugfs? Maybe cc gregkh...
> > 									Pavel
> 
> I thought about the one entry per file issue.
> Documentation/filesystems/sysfs.txt says:
> > It is noted that it may not be efficient to contain only one
> > value per file, so it is socially acceptable to express an array of
> > values of the same type.
> 
> which appears to cover my use of it as they're all the same type (and 
> considering that ideally there will be exactly 1 device listed anyway).
> 
> I wonder if I should separate them with spaces rather than newlines too 
> (newlines just seemed more appropriate for a variable sized list at the time).
> 
> I ackowledge your point that debugfs may be a more appropriate place for it, 
> but would that also apply to the pm_trace file?

Yes, it would.  Let's keep the things cosistent, ie. v2 of your patch is fine
and I'm going to apply it.

Thanks,
Rafael