Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Alan Stern]

On Fri, 11 Jan 2008, Greg KH wrote:

> On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:

> > err, no.  pm-introduce-destroy_suspended_device.patch demolishes
> > pm-acquire-device-locks-on-suspend-rev-3.patch
> > 
> > Confused, giving up.
> 
> I'm confused too, I have no idea what the proper order of things should
> be either.  Anyone want to give me a hint?

Sorry for the confusion.  The correct patch to apply is 
pm-acquire-device-locks-on-suspend-rev-3 (plus the attending 
style-fixups).  It encompasses those earlier patches.

The real problem is that our current email workflow patterns don't 
provide a standardized way for maintainers to tell when a new patch 
submission is meant to override or replace an earlier submission (or 
even a set of earlier submissions).  Does anybody have some suggestions 
for a good way to do this?

Alan Stern

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Andi Kleen]

> The real problem is that our current email workflow patterns don't 
> provide a standardized way for maintainers to tell when a new patch 
> submission is meant to override or replace an earlier submission (or 
> even a set of earlier submissions).  Does anybody have some suggestions 
> for a good way to do this?

The versioning approach pioneered by Christoph Lameter seems to work
reasonably well.

If you post a new version increase a version number and add it with "vXXX" to the
Subject.

Also add a short change log between versions at the bottom; e.g. v1->v2: .... etc.

Then it is always clear what is the latest'n'greatest.

-Andi

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Andrew Morton]

On Fri, 11 Jan 2008 22:11:52 -0500 (EST) Alan Stern <stern@rowland.harvard.edu> wrote:

> On Fri, 11 Jan 2008, Greg KH wrote:
> 
> > On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
> 
> > > err, no.  pm-introduce-destroy_suspended_device.patch demolishes
> > > pm-acquire-device-locks-on-suspend-rev-3.patch
> > > 
> > > Confused, giving up.
> > 
> > I'm confused too, I have no idea what the proper order of things should
> > be either.  Anyone want to give me a hint?
> 
> Sorry for the confusion.  The correct patch to apply is 
> pm-acquire-device-locks-on-suspend-rev-3 (plus the attending 
> style-fixups).  It encompasses those earlier patches.
> 
> The real problem is that our current email workflow patterns don't 
> provide a standardized way for maintainers to tell when a new patch 
> submission is meant to override or replace an earlier submission (or 
> even a set of earlier submissions).  Does anybody have some suggestions 
> for a good way to do this?
> 

Don't formally send it until it's ready.  Seriously.  You can always resend
it if it didn't get applied anywhere.

Once a patch reaches a sufficient level of maturity for it to be ready to
be merged into a subsystem tree, any subsequent changes should be
sufficiently small that incremental patches are the way to apply touchups.

The core problem here is that (lots of) people are flinging patches at
tree-owners before they are sufficiently baked.

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Greg KH]

On Fri, Jan 11, 2008 at 10:11:52PM -0500, Alan Stern wrote:
> On Fri, 11 Jan 2008, Greg KH wrote:
> 
> > On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
> 
> > > err, no.  pm-introduce-destroy_suspended_device.patch demolishes
> > > pm-acquire-device-locks-on-suspend-rev-3.patch
> > > 
> > > Confused, giving up.
> > 
> > I'm confused too, I have no idea what the proper order of things should
> > be either.  Anyone want to give me a hint?
> 
> Sorry for the confusion.  The correct patch to apply is 
> pm-acquire-device-locks-on-suspend-rev-3 (plus the attending 
> style-fixups).  It encompasses those earlier patches.

Can someone resend this to me?  Do I need to drop the patch I currently
have in my tree as well?  Or put it before/after that one?

> The real problem is that our current email workflow patterns don't 
> provide a standardized way for maintainers to tell when a new patch 
> submission is meant to override or replace an earlier submission (or 
> even a set of earlier submissions).  Does anybody have some suggestions 
> for a good way to do this?

Yeah, just tell me what you want me to do with it (drop an old one,
replace it, add it, etc.)  We usually can handle this pretty well :)

thanks,

greg k-h

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Rafael J. Wysocki]

On Saturday, 12 of January 2008, Greg KH wrote:
> On Fri, Jan 11, 2008 at 10:11:52PM -0500, Alan Stern wrote:
> > On Fri, 11 Jan 2008, Greg KH wrote:
> > 
> > > On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
> > 
> > > > err, no.  pm-introduce-destroy_suspended_device.patch demolishes
> > > > pm-acquire-device-locks-on-suspend-rev-3.patch
> > > > 
> > > > Confused, giving up.
> > > 
> > > I'm confused too, I have no idea what the proper order of things should
> > > be either.  Anyone want to give me a hint?
> > 
> > Sorry for the confusion.  The correct patch to apply is 
> > pm-acquire-device-locks-on-suspend-rev-3 (plus the attending 
> > style-fixups).  It encompasses those earlier patches.
> 
> Can someone resend this to me?  Do I need to drop the patch I currently
> have in my tree as well?  Or put it before/after that one?
> 
> > The real problem is that our current email workflow patterns don't 
> > provide a standardized way for maintainers to tell when a new patch 
> > submission is meant to override or replace an earlier submission (or 
> > even a set of earlier submissions).  Does anybody have some suggestions 
> > for a good way to do this?
> 
> Yeah, just tell me what you want me to do with it (drop an old one,
> replace it, add it, etc.)  We usually can handle this pretty well :)

I'll repost the new patch along with instructions what to do with it.

Greetings,
Rafael

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> Hi,
> 
> Some device drivers register CPU hotplug notifiers and use them to 
> destroy device objects when removing the corresponding CPUs and to 
> create these objects when adding the CPUs back.
> 
> Unfortunately, this is not the right thing to do during 
> suspend/hibernation, since in that cases the CPU hotplug notifiers are 
> called after suspending devices and before resuming them, so the 
> operations in question are carried out on the objects representing 
> suspended devices which shouldn't be unregistered behing the PM core's 
> back.  Although right now it usually doesn't lead to any practical 
> complications, it will predictably deadlock if 
> gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is 
> applied.
> 
> The solution is to prevent drivers from removing/adding devices from 
> within CPU hotplug notifiers during suspend/hibernation using the 
> FROZEN bit in the notifier's action argument.  However, this has to be 
> done with care, since the devices objects related to the nonboot CPUs 
> that failed to go online during resume should not be present in the 
> system.  For this reason, it seems reasonable to introduce a mechanism 
> allowing drivers to ask the PM core to remove device objects 
> corresponding to suspended devices on their behalf.
> 
> The first patch in the series introduces such a mechanism.  The 
> remaining three patches modify the MSR, x86-64 MCE and cpuid drivers 
> in accordance with the above approach.

btw., it would be really, really cool if there was a scriptable way i 
could test suspend/resume functionality. Pavel has this /dev/rtc thing 
to set up an alarm (not sure how functional it is) - would it be 
possible to have it as a "suspend for 10 seconds then resume" debug 
functionality? That way any suspend breakage would be detectable (and 
bisectable) in automated testing - if the resume does not come back 
after 10-20 seconds then the test failed.

	Ingo

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Rafael J. Wysocki]

On Wednesday, 2 of January 2008, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > Hi,
> > 
> > Some device drivers register CPU hotplug notifiers and use them to 
> > destroy device objects when removing the corresponding CPUs and to 
> > create these objects when adding the CPUs back.
> > 
> > Unfortunately, this is not the right thing to do during 
> > suspend/hibernation, since in that cases the CPU hotplug notifiers are 
> > called after suspending devices and before resuming them, so the 
> > operations in question are carried out on the objects representing 
> > suspended devices which shouldn't be unregistered behing the PM core's 
> > back.  Although right now it usually doesn't lead to any practical 
> > complications, it will predictably deadlock if 
> > gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is 
> > applied.
> > 
> > The solution is to prevent drivers from removing/adding devices from 
> > within CPU hotplug notifiers during suspend/hibernation using the 
> > FROZEN bit in the notifier's action argument.  However, this has to be 
> > done with care, since the devices objects related to the nonboot CPUs 
> > that failed to go online during resume should not be present in the 
> > system.  For this reason, it seems reasonable to introduce a mechanism 
> > allowing drivers to ask the PM core to remove device objects 
> > corresponding to suspended devices on their behalf.
> > 
> > The first patch in the series introduces such a mechanism.  The 
> > remaining three patches modify the MSR, x86-64 MCE and cpuid drivers 
> > in accordance with the above approach.
> 
> btw., it would be really, really cool if there was a scriptable way i 
> could test suspend/resume functionality.

First, there are patches queued for 2.6.25 that allow you to test various
phases of suspend (specifically, patches 09-11 in the series at
http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.24-rc6/patches/).

With these patches applied you can do something like:
# echo core > /sys/power/pm_test
# echo mem > /sys/power/state
and it will run the suspend code up to, but not including, entering the sleep
state (it will busy wait for 5 sec. instead).  Then, it will run the resume
code.

There are 6 testing levels available, documented in patch 11 and in the
changelogs.

Second, there's the rtc wakealarm thing that can be used to test the real
suspend.

> Pavel has this /dev/rtc thing to set up an alarm (not sure how functional it
> is) - would it be possible to have it as a "suspend for 10 seconds then
> resume" debug functionality?

Well, we have the following test script in the userland suspend package that
is supposed to work right now:

#!/bin/bash
date
cd /sys/class/rtc/rtc0
echo $(( $(cat since_epoch) + 20 )) > wakealarm
s2ram
date

provided that the new rtc driver code is compiled (and the old one is not).

> That way any suspend breakage would be detectable (and bisectable) in
> automated testing - if the resume does not come back after 10-20 seconds then
> the test failed. 

Yes, but please note that some systems require user space manipulations of the
graphics adapter for suspend to work and to detect a breakage of such a system
you need to boot it into X and use s2ram to suspend.

Greetings,
Rafael

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Ingo Molnar]

(David Brownell Cc:-ed too)

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> Well, we have the following test script in the userland suspend 
> package that is supposed to work right now:
> 
> #!/bin/bash
> date
> cd /sys/class/rtc/rtc0
> echo $(( $(cat since_epoch) + 20 )) > wakealarm
> s2ram
> date
> 
> provided that the new rtc driver code is compiled (and the old one is not).

ok, will try that. A stupid question. The old RTC driver is in 
drivers/char/rtc.c, and maps to:

  crw-r--r-- 1 root root  10, 135 Oct 25 18:02 /dev/rtc

the new driver is in drivers/rtc/*, and maps to:

  crw-r--r-- 1 root root 254,   0 Dec 12 02:30 /dev/rtc0

but all the x86 distro boxes i have access to make use of /dev/rtc. 
There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it 
appears to me that the new RTC driver isnt actually utilized on most 
distributions.

shouldnt we provide a Kconfig way of replacing dev 10:135 with the new 
driver's 254:0 device? (while keeping all the current modes of operation 
as well, of course.) It's all supposed to be 100% ioctl ABI compatible 
with the old driver, right? That way distros could start migrating to it 
as well, without depending on any udev hackery.

> > That way any suspend breakage would be detectable (and bisectable) 
> > in automated testing - if the resume does not come back after 10-20 
> > seconds then the test failed.
> 
> Yes, but please note that some systems require user space 
> manipulations of the graphics adapter for suspend to work and to 
> detect a breakage of such a system you need to boot it into X and use 
> s2ram to suspend.

yeah, i wouldnt expect graphics mode to come back without quirks. But it 
should still work fine over the network, right? (which is my main mode 
of testing anyway)

	Ingo

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Rafael J. Wysocki]

On Wednesday, 2 of January 2008, Ingo Molnar wrote:
> 
> (David Brownell Cc:-ed too)
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > Well, we have the following test script in the userland suspend 
> > package that is supposed to work right now:
> > 
> > #!/bin/bash
> > date
> > cd /sys/class/rtc/rtc0
> > echo $(( $(cat since_epoch) + 20 )) > wakealarm
> > s2ram
> > date
> > 
> > provided that the new rtc driver code is compiled (and the old one is not).
> 
> ok, will try that. A stupid question. The old RTC driver is in 
> drivers/char/rtc.c, and maps to:
> 
>   crw-r--r-- 1 root root  10, 135 Oct 25 18:02 /dev/rtc
> 
> the new driver is in drivers/rtc/*, and maps to:
> 
>   crw-r--r-- 1 root root 254,   0 Dec 12 02:30 /dev/rtc0
> 
> but all the x86 distro boxes i have access to make use of /dev/rtc. 
> There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it 
> appears to me that the new RTC driver isnt actually utilized on most 
> distributions.

I think so.

> shouldnt we provide a Kconfig way of replacing dev 10:135 with the new 
> driver's 254:0 device? (while keeping all the current modes of operation 
> as well, of course.) It's all supposed to be 100% ioctl ABI compatible 
> with the old driver, right? That way distros could start migrating to it 
> as well, without depending on any udev hackery.

Question for Dave. ;-)

> > > That way any suspend breakage would be detectable (and bisectable) 
> > > in automated testing - if the resume does not come back after 10-20 
> > > seconds then the test failed.
> > 
> > Yes, but please note that some systems require user space 
> > manipulations of the graphics adapter for suspend to work and to 
> > detect a breakage of such a system you need to boot it into X and use 
> > s2ram to suspend.
> 
> yeah, i wouldnt expect graphics mode to come back without quirks. But it 
> should still work fine over the network, right? (which is my main mode 
> of testing anyway)

Well, if the graphics is sufficiently broken, it won't resume at all.

The "echo core > /sys/power/pm_test && echo mem > /sys/power/state" thing
should always work, though (IOW, if this doesn't work, we cartainly have a
bug).

Rafael

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Kay Sievers]

On Jan 2, 2008 2:15 PM, Ingo Molnar <mingo@elte.hu> wrote:
> A stupid question. The old RTC driver is in
> drivers/char/rtc.c, and maps to:
>
>   crw-r--r-- 1 root root  10, 135 Oct 25 18:02 /dev/rtc
>
> the new driver is in drivers/rtc/*, and maps to:
>
>   crw-r--r-- 1 root root 254,   0 Dec 12 02:30 /dev/rtc0
>
> but all the x86 distro boxes i have access to make use of /dev/rtc.
> There's no symlink set up from /dev/rtc to /dev/rtc0 either. So it
> appears to me that the new RTC driver isnt actually utilized on most
> distributions.
>
> shouldnt we provide a Kconfig way of replacing dev 10:135 with the new
> driver's 254:0 device? (while keeping all the current modes of operation
> as well, of course.) It's all supposed to be 100% ioctl ABI compatible
> with the old driver, right?

It's not compatible enough to "fake" only the old major/minor. Userspace
must be fixed not to depend on stuff like:  /proc/sys/dev/rtc/max-user-freq:
  http://lkml.org/lkml/2007/8/4/183

> That way distros could start migrating to it
> as well, without depending on any udev hackery.

It's in the default udev setup:
  KERNEL=="rtc|rtc0",             MODE="0644"
  KERNEL=="rtc0",                 SYMLINK+="rtc"

Kay

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Ingo Molnar]

* Kay Sievers <kay.sievers@vrfy.org> wrote:

> > shouldnt we provide a Kconfig way of replacing dev 10:135 with the 
> > new driver's 254:0 device? (while keeping all the current modes of 
> > operation as well, of course.) It's all supposed to be 100% ioctl 
> > ABI compatible with the old driver, right?
> 
> It's not compatible enough to "fake" only the old major/minor. Userspace
> must be fixed not to depend on stuff like:  /proc/sys/dev/rtc/max-user-freq:
>   http://lkml.org/lkml/2007/8/4/183

i think that should be fixed by providing a compatible 
/proc/sys/dev/rtc/max-user-freq implementation in the new driver too.

> > That way distros could start migrating to it
> > as well, without depending on any udev hackery.
> 
> It's in the default udev setup:
>   KERNEL=="rtc|rtc0",             MODE="0644"
>   KERNEL=="rtc0",                 SYMLINK+="rtc"

but if it breaks max-user-freq (which is needed by qemu for example) 
then distros would likely disable it, right?

or this rule might be broken in some way. For example my Fedora 8 box 
has this in /etc/udev/rules.d/50-udev-default.rules:

 # miscellaneous
 KERNEL=="fuse",                 MODE="0666"
 KERNEL=="rtc|rtc0",             MODE="0644"
 KERNEL=="rtc0",                 SYMLINK+="rtc"

still i've got:

 crw------- 1 root root  10, 135 Dec 28 08:13 /dev/rtc
 crw-r--r-- 1 root root 254,   0 Dec 28 08:13 /dev/rtc0

_and_ my distro kernel doesnt even have CONFIG_RTC enabled - i run the 
Fedora 9 devel/rawhide kernel on this box:

 # CONFIG_RTC is not set
 # CONFIG_GEN_RTC is not set
 # CONFIG_HPET_RTC_IRQ is not set
 CONFIG_RTC_LIB=y
 CONFIG_RTC_CLASS=y
 # CONFIG_RTC_HCTOSYS is not set
 # CONFIG_RTC_DEBUG is not set
 # RTC interfaces
 CONFIG_RTC_INTF_SYSFS=y
 CONFIG_RTC_INTF_PROC=y
 CONFIG_RTC_INTF_DEV=y
 # CONFIG_RTC_INTF_DEV_UIE_EMUL is not set
 # CONFIG_RTC_DRV_TEST is not set

udev-116-3.fc8. Maybe i just misunderstood what the grand plan was here 
- i assumed it was to smoothly convert from old driver to new driver, 
without forcing any changes on user-space.

	Ingo

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[David Brownell]

> > > shouldnt we provide a Kconfig way of replacing dev 10:135 with the 
> > > new driver's 254:0 device? (while keeping all the current modes of 
> > > operation as well, of course.) It's all supposed to be 100% ioctl 
> > > ABI compatible with the old driver, right?
> > 
> > It's not compatible enough to "fake" only the old major/minor. Userspace
> > must be fixed not to depend on stuff like:  /proc/sys/dev/rtc/max-user-freq:
> >   http://lkml.org/lkml/2007/8/4/183
>
> i think that should be fixed by providing a compatible 
> /proc/sys/dev/rtc/max-user-freq implementation in the new driver too.

I see /sys/class/rtc/rtc0/max_user_freq, fwiw ...


> > > That way distros could start migrating to it
> > > as well, without depending on any udev hackery.
> > 
> > It's in the default udev setup:
> >   KERNEL=="rtc|rtc0",             MODE="0644"
> >   KERNEL=="rtc0",                 SYMLINK+="rtc"
>
> but if it breaks max-user-freq (which is needed by qemu for example) 
> then distros would likely disable it, right?
>
> or this rule might be broken in some way. For example my Fedora 8 box 
> has this in /etc/udev/rules.d/50-udev-default.rules:
>
>  # miscellaneous
>  KERNEL=="fuse",                 MODE="0666"
>  KERNEL=="rtc|rtc0",             MODE="0644"
>  KERNEL=="rtc0",                 SYMLINK+="rtc"
>
> still i've got:
>
>  crw------- 1 root root  10, 135 Dec 28 08:13 /dev/rtc
>  crw-r--r-- 1 root root 254,   0 Dec 28 08:13 /dev/rtc0

I suspect the /dev/rtc node was provided by the distro, so those
udev rules won't trigger.


> _and_ my distro kernel doesnt even have CONFIG_RTC enabled - i run the 
> Fedora 9 devel/rawhide kernel on this box:
>
>  # CONFIG_RTC is not set
>  # CONFIG_GEN_RTC is not set
>  # CONFIG_HPET_RTC_IRQ is not set
>  CONFIG_RTC_LIB=y
>  CONFIG_RTC_CLASS=y
>  # CONFIG_RTC_HCTOSYS is not set
>  # CONFIG_RTC_DEBUG is not set
>  # RTC interfaces
>  CONFIG_RTC_INTF_SYSFS=y
>  CONFIG_RTC_INTF_PROC=y
>  CONFIG_RTC_INTF_DEV=y
>  # CONFIG_RTC_INTF_DEV_UIE_EMUL is not set
>  # CONFIG_RTC_DRV_TEST is not set
>
> udev-116-3.fc8.

I run my x86 systems like that (also with RTC_DRV_CMOS), but
the ARM systems usually have RTC_HCTOSYS set too (plus some
RTC driver other than rtc-cmos).  The x86 system init code
pokes directly at the legacy RTC hardware in several cases.


>	Maybe i just misunderstood what the grand plan was here 
> - i assumed it was to smoothly convert from old driver to new driver, 
> without forcing any changes on user-space.

If there was a Grand Plan, I never heard of it. :)

It'd need to have some NTP sync solution for RTC_LIB devices, but
ISTR the gentime stuff still assumes an update_persistent_clock()
that doesn't sleep ... and hence can't be used with I2C based RTCs.


I've been trying to make sure the x86 world could realistically
switch to the RTC framework used by other Linux platforms, hence
e.g. the util-unix-ng updates, but never assumed there would be
no userspace changes.  After all, userspace was using a mishmash
of tools that were far from platform-agnostic, and moving away
from x86-dependency implies such things will change.

However I *do* think that symlinking rtc to /dev/rtc0 should
make it practical to use most older binaries.

- Dave

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Alessandro Zummo]

On Wed, 02 Jan 2008 09:54:00 -0800
David Bro
> It'd need to have some NTP sync solution for RTC_LIB devices, but
> ISTR the gentime stuff still assumes an update_persistent_clock()
> that doesn't sleep ... and hence can't be used with I2C based RTCs.

 I still believe NTP sync stuff should be done outside of the kernel.
 given the mean accuracy of RTC chips and other sync factors, imho
 you haven't so much to gain with an in-kernel sync code.


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[David Brownell]

> > It'd need to have some NTP sync solution for RTC_LIB devices, but
> > ISTR the gentime stuff still assumes an update_persistent_clock()
> > that doesn't sleep ... and hence can't be used with I2C based RTCs.
>
> I still believe NTP sync stuff should be done outside of the kernel.
> given the mean accuracy of RTC chips and other sync factors, imho
> you haven't so much to gain with an in-kernel sync code.

Then the kernel/time/ntp.c stuff should be removed on all systems?

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Alessandro Zummo]

On Wed, 02 Jan 2008 10:12:54 -0800
David Brownell <david-b@pacbell.net> wrote:

> > > It'd need to have some NTP sync solution for RTC_LIB devices, but
> > > ISTR the gentime stuff still assumes an update_persistent_clock()
> > > that doesn't sleep ... and hence can't be used with I2C based RTCs.
> >
> > I still believe NTP sync stuff should be done outside of the kernel.
> > given the mean accuracy of RTC chips and other sync factors, imho
> > you haven't so much to gain with an in-kernel sync code.
> 
> Then the kernel/time/ntp.c stuff should be removed on all systems?

 I'd say yes, but I think that would be dangerous to my own life :)

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> I've been trying to make sure the x86 world could realistically switch 
> to the RTC framework used by other Linux platforms, hence e.g. the 
> util-unix-ng updates, but never assumed there would be no userspace 
> changes.  After all, userspace was using a mishmash of tools that were 
> far from platform-agnostic, and moving away from x86-dependency 
> implies such things will change.
> 
> However I *do* think that symlinking rtc to /dev/rtc0 should make it 
> practical to use most older binaries.

then please provide a kernel config option for the new driver to take 
over 10:135 too. There's nothing worse to the adoption of new kernel 
features necessiating user-space attention. I've got several images of 
old distros that i dont want to reconfigure in any way, and it would be 
nice to have a drop-in /dev/rtc replacement for them. Really. This is 
how we do it for just about everything else too.

	Ingo

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[David Brownell]

On Wednesday 02 January 2008, Ingo Molnar wrote:
> 
> then please provide a kernel config option for the new driver to take 
> over 10:135 too. There's nothing worse to the adoption of new kernel 
> features necessiating user-space attention. I've got several images of 
> old distros that i dont want to reconfigure in any way, and it would be 
> nice to have a drop-in /dev/rtc replacement for them. Really. This is 
> how we do it for just about everything else too.

I'll let someone else chase that particular issue.  :)

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[David Brownell]

(Alessandro Zummo Cc:-ed too -- RTC subsystem maintainer)

> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> > Well, we have the following test script in the userland suspend 
> > package that is supposed to work right now:
> > 
> > #!/bin/bash
> > date
> > cd /sys/class/rtc/rtc0
> > echo $(( $(cat since_epoch) + 20 )) > wakealarm
> > s2ram
> > date
> > 
> > provided that the new rtc driver code is compiled (and the old one is not).

Eventually, swapping driver modules ought to work too.  The
legacy /proc/acpi/wakeup files would ISTR cause problems in
current code.

Of course, one reason to want to use the RTC framework code
is to stop depending on x86-isms like ACPI or "s2ram", and
thus to work on more Linux platforms.  ;)


> ok, will try that. A stupid question. The old RTC driver is in 
> drivers/char/rtc.c, and maps to:
>
>   crw-r--r-- 1 root root  10, 135 Oct 25 18:02 /dev/rtc
>
> the new driver is in drivers/rtc/*, and maps to:
>
>   crw-r--r-- 1 root root 254,   0 Dec 12 02:30 /dev/rtc0
>
> but all the x86 distro boxes i have access to make use of /dev/rtc. 
> There's no symlink set up from /dev/rtc to /dev/rtc0 either.

Current util-linux-ng code uses either RTC device file; and udev
sets up /dev/rtc0 as needed.  (But not /dev/rtc, as I recall...)
Have distros switched away from the old unmaintained util-linux?


>	So it 
> appears to me that the new RTC driver isnt actually utilized on most 
> distributions.

That might be so.  There are some HPET issues, but those show up with
both drivers.

The main other issue I know about which would seem to argue for using
the legacy driver, instead of the RTC framework, is that some BIOSes
don't seem to provide PNPACPI entries for their RTCs.  I got one report
of a newish HP Opteron system that doesn't.  I have no idea how common
that is.  The drivers/acpi/glue.c code could detect that, but maybe a
better place to address that would be in PNP code; in that case, ISTR
that PNP0c01 claimed the RTC ioports, and so would be the natural place
to make provide a real driver model node for that hardware.



> shouldnt we provide a Kconfig way of replacing dev 10:135 with the new 
> driver's 254:0 device? (while keeping all the current modes of operation 
> as well, of course.)

The major number 254 is not statically allocated, ISTR; that should
be managed only by udev.


> 			 It's all supposed to be 100% ioctl ABI compatible 
> with the old driver, right? That way distros could start migrating to it 
> as well, without depending on any udev hackery.

I don't know of any ioctl differences userspace would care about.

- Dave

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> > shouldnt we provide a Kconfig way of replacing dev 10:135 with the 
> > new driver's 254:0 device? (while keeping all the current modes of 
> > operation as well, of course.)
> 
> The major number 254 is not statically allocated, ISTR; that should be 
> managed only by udev.

sorry, i meant a .config option that would cause 10:135 to be managed by 
the new RTC code too. (Config option can be default-off.)

	Ingo

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Pavel Machek]

Hi!

> > > > That way any suspend breakage would be detectable (and bisectable) 
> > > > in automated testing - if the resume does not come back after 10-20 
> > > > seconds then the test failed.
> > > 
> > > Yes, but please note that some systems require user space 
> > > manipulations of the graphics adapter for suspend to work and to 
> > > detect a breakage of such a system you need to boot it into X and use 
> > > s2ram to suspend.
> > 
> > yeah, i wouldnt expect graphics mode to come back without quirks. But it 
> > should still work fine over the network, right? (which is my main mode 
> > of testing anyway)
> 
> Well, if the graphics is sufficiently broken, it won't resume at
> all.

Actually, no. Unless you try to boot the bios, it should come up
without graphics. 

Hmm... first framebuffer access may kill the machine at that
point... so disable framebuffer...? ;-). 

vga=1 and no acpi_sleep options usually does the trick for me. That
should work everywhere, independend of graphics options, AFAICT.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Andrew Morton]

On Wed, 2 Jan 2008 00:32:44 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> Some device drivers register CPU hotplug notifiers and use them to destroy
> device objects when removing the corresponding CPUs and to create these objects
> when adding the CPUs back.
> 
> Unfortunately, this is not the right thing to do during suspend/hibernation,
> since in that cases the CPU hotplug notifiers are called after suspending
> devices and before resuming them, so the operations in question are carried
> out on the objects representing suspended devices which shouldn't be
> unregistered behing the PM core's back. __Although right now it usually doesn't
> lead to any practical complications, it will predictably deadlock if
> gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.
> 
> The solution is to prevent drivers from removing/adding devices from within
> CPU hotplug notifiers during suspend/hibernation using the FROZEN bit
> in the notifier's action argument.  However, this has to be done with care,
> since the devices objects related to the nonboot CPUs that failed to go online
> during resume should not be present in the system.  For this reason, it seems
> reasonable to introduce a mechanism allowing drivers to ask the PM core to
> remove device objects corresponding to suspended devices on their behalf.
> 
> The first patch in the series introduces such a mechanism.  The remaining three
> patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the
> above approach.

These patches are a preresuisite to
gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its
later fixed-up versions.

So what I have now is

revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
pm-introduce-destroy_suspended_device.patch
pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch
pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch
pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch
pm-acquire-device-locks-on-suspend-rev-3.patch
pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch
pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch

So what needs to happen in Greg's tree is

a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch

b) apply these four patches

c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Andrew Morton]

On Fri, 11 Jan 2008 16:46:13 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> > The first patch in the series introduces such a mechanism.  The remaining three
> > patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the
> > above approach.
> 
> These patches are a preresuisite to
> gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its
> later fixed-up versions.
> 
> So what I have now is
> 
> revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
> pm-introduce-destroy_suspended_device.patch
> pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch
> pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch
> pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch
> pm-acquire-device-locks-on-suspend-rev-3.patch
> pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch
> pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch
> 
> So what needs to happen in Greg's tree is
> 
> a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
> 
> b) apply these four patches
> 
> c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)

err, no.  pm-introduce-destroy_suspended_device.patch demolishes
pm-acquire-device-locks-on-suspend-rev-3.patch

Confused, giving up.

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Greg KH]

On Fri, Jan 11, 2008 at 04:49:04PM -0800, Andrew Morton wrote:
> On Fri, 11 Jan 2008 16:46:13 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > The first patch in the series introduces such a mechanism.  The remaining three
> > > patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the
> > > above approach.
> > 
> > These patches are a preresuisite to
> > gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its
> > later fixed-up versions.
> > 
> > So what I have now is
> > 
> > revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
> > pm-introduce-destroy_suspended_device.patch
> > pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch
> > pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch
> > pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch
> > pm-acquire-device-locks-on-suspend-rev-3.patch
> > pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch
> > pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch
> > 
> > So what needs to happen in Greg's tree is
> > 
> > a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
> > 
> > b) apply these four patches
> > 
> > c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)
> 
> err, no.  pm-introduce-destroy_suspended_device.patch demolishes
> pm-acquire-device-locks-on-suspend-rev-3.patch
> 
> Confused, giving up.

I'm confused too, I have no idea what the proper order of things should
be either.  Anyone want to give me a hint?

thanks,

greg k-h

Re: [PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Rafael J. Wysocki]

On Saturday, 12 of January 2008, Andrew Morton wrote:
> On Fri, 11 Jan 2008 16:46:13 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > The first patch in the series introduces such a mechanism.  The remaining three
> > > patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the
> > > above approach.
> > 
> > These patches are a preresuisite to
> > gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch and its
> > later fixed-up versions.
> > 
> > So what I have now is
> > 
> > revert-gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
> > pm-introduce-destroy_suspended_device.patch
> > pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch
> > pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch
> > pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch
> > pm-acquire-device-locks-on-suspend-rev-3.patch
> > pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch
> > pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch
> > 
> > So what needs to happen in Greg's tree is
> > 
> > a) drop the old gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch
> > 
> > b) apply these four patches
> > 
> > c) apply the new pm-acquire-device-locks-on-suspend-rev-3.patch (and its fixups)
> 
> err, no.  pm-introduce-destroy_suspended_device.patch demolishes
> pm-acquire-device-locks-on-suspend-rev-3.patch
> 
> Confused, giving up.

Sorry, I didn't know you still had pm-introduce-destroy_suspended_device.patch.

Please drop:

pm-do-not-destroy-create-devices-while-suspended-in-msrc-rev-2.patch
pm-do-not-destroy-create-devices-while-suspended-in-mce_64c.patch
pm-do-not-destroy-create-devices-while-suspended-in-cpuidc.patch
pm-acquire-device-locks-on-suspend-rev-3.patch
pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes.patch
pm-acquire-device-locks-on-suspend-rev-3-checkpatch-fixes-2.patch

and I'll resend a new checkpatch.pl-compliant version of the
pm-acquire-device-locks-on-suspend patch with appropriate sign-offs to you
and Greg.

[PATCH 0/4] PM: Do not destroy/create devices while suspended (rev. 2)

[Rafael J. Wysocki]

Hi,

Some device drivers register CPU hotplug notifiers and use them to destroy
device objects when removing the corresponding CPUs and to create these objects
when adding the CPUs back.

Unfortunately, this is not the right thing to do during suspend/hibernation,
since in that cases the CPU hotplug notifiers are called after suspending
devices and before resuming them, so the operations in question are carried
out on the objects representing suspended devices which shouldn't be
unregistered behing the PM core's back.  Although right now it usually doesn't
lead to any practical complications, it will predictably deadlock if
gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch is applied.

The solution is to prevent drivers from removing/adding devices from within
CPU hotplug notifiers during suspend/hibernation using the FROZEN bit
in the notifier's action argument.  However, this has to be done with care,
since the devices objects related to the nonboot CPUs that failed to go online
during resume should not be present in the system.  For this reason, it seems
reasonable to introduce a mechanism allowing drivers to ask the PM core to
remove device objects corresponding to suspended devices on their behalf.

The first patch in the series introduces such a mechanism.  The remaining three
patches modify the MSR, x86-64 MCE and cpuid drivers in accordance with the
above approach.

Thanks,
Rafael