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@xxxxxxxxxxxxx>
> > > ---
> > >
> > > 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
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
