All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
@ 2007-04-02 14:16 Alan Stern
  2007-04-02 17:09   ` Linus Torvalds
  2007-04-02 19:16   ` David Brownell
  0 siblings, 2 replies; 17+ messages in thread
From: Alan Stern @ 2007-04-02 14:16 UTC (permalink / raw)
  To: David Brownell
  Cc: Maxim Levitsky, Ingo Molnar, Linus Torvalds, Greg KH,
	Thomas Gleixner, Kernel development list, Linux-pm mailing list

On Saturday March 31, 2007, David Brownell wrote:

> I'm about ready to test the appended patch... a "move one device" call
> might be safest at this point in the release cycle though.
> 
> - Dave
> 
> ========================	SNIP!
> Change how the PM list is constructed, so that devices are added right
> after their parents (when they have one) rather than at the end of the
> list.  This preserves sequencing guarantees, but enables sequencing of
> suspend/resume operations by more important characteristics than "when
> device happened to enumerate" ... e.g. clocksources and clockevents
> at a clearly defined point during suspend and resume.
> 
> This patch has a potential downside for devices that have multiple
> power dependencies and which "just happened to work" before.

Dave:

Would you mind retracting this patch?  It will interfere with some work
I've been doing in USB, work that relies on exactly the sort of multiple 
power dependency you mention.

The problem is this: When a low- or full-speed USB device is resumed 
following a power loss (this is part of the "persistent USB" patch), it's 
necessary for the corresponding EHCI root hub to be resumed first so that 
the port's OWNER bit can be set properly.

It works fine as long as devices are resumed in order of registration,
because a low- or full-speed USB device can't be registered before the
high-speed root hub.  (If it was, it would be disconnected when the
high-speed root hub initialized and took control of the port.)  So this
isn't a "just happened to work" situation; it really is a critical
ordering dependency.

The clock source problem can be solved in other ways.  For instance, right 
now things are set up so that clock sources are registered at various 
random times and the clockevents code adapts to use the best available 
device, right?.  So simply arrange for a clock source to "depart" as it is 
suspended; then clockevents can fall back to using other lower-precision 
sources.  As long as devices suspend in reverse order of discovery, the 
system should always function properly.

Alan Stern


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-04-02 14:16 [linux-pm] [PATCH v2] Add suspend/resume for HPET Alan Stern
@ 2007-04-02 17:09   ` Linus Torvalds
  2007-04-02 19:16   ` David Brownell
  1 sibling, 0 replies; 17+ messages in thread
From: Linus Torvalds @ 2007-04-02 17:09 UTC (permalink / raw)
  To: Alan Stern
  Cc: David Brownell, Maxim Levitsky, Ingo Molnar, Greg KH,
	Thomas Gleixner, Kernel development list, Linux-pm mailing list



On Mon, 2 Apr 2007, Alan Stern wrote:
> 
> Would you mind retracting this patch?  It will interfere with some work
> I've been doing in USB, work that relies on exactly the sort of multiple 
> power dependency you mention.

I agree. We should just make the damn timers be added at the right point. 
We can for example *add* the devices early, even if we then actually start 
using them at some later date.

			Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] Add suspend/resume for HPET
@ 2007-04-02 17:09   ` Linus Torvalds
  0 siblings, 0 replies; 17+ messages in thread
From: Linus Torvalds @ 2007-04-02 17:09 UTC (permalink / raw)
  To: Alan Stern
  Cc: Maxim Levitsky, Ingo Molnar, Linux-pm mailing list,
	Thomas Gleixner, Kernel development list



On Mon, 2 Apr 2007, Alan Stern wrote:
> 
> Would you mind retracting this patch?  It will interfere with some work
> I've been doing in USB, work that relies on exactly the sort of multiple 
> power dependency you mention.

I agree. We should just make the damn timers be added at the right point. 
We can for example *add* the devices early, even if we then actually start 
using them at some later date.

			Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-04-02 14:16 [linux-pm] [PATCH v2] Add suspend/resume for HPET Alan Stern
@ 2007-04-02 19:16   ` David Brownell
  2007-04-02 19:16   ` David Brownell
  1 sibling, 0 replies; 17+ messages in thread
From: David Brownell @ 2007-04-02 19:16 UTC (permalink / raw)
  To: Alan Stern
  Cc: Maxim Levitsky, Ingo Molnar, Linus Torvalds, Greg KH,
	Thomas Gleixner, Kernel development list, Linux-pm mailing list

On Monday 02 April 2007 7:16 am, Alan Stern wrote:
> On Saturday March 31, 2007, David Brownell wrote:
> 
> > Change how the PM list is constructed, so that devices are added right
> > after their parents (when they have one) rather than at the end of the
> > list.  ...
> > 
> > This patch has a potential downside for devices that have multiple
> > power dependencies and which "just happened to work" before.
> 
> Dave:
> 
> Would you mind retracting this patch?  It will interfere with some work
> I've been doing in USB, work that relies on exactly the sort of multiple 
> power dependency you mention.

It's out there for discussion right now more than merging.

What the driver model does now is problematic.  Even if the clockevent
stuff gets different fixes, those driver model issues still exist:  the
existence of dependencies that are not part of the device tree.


> The problem is this: When a low- or full-speed USB device is resumed 
> following a power loss (this is part of the "persistent USB" patch), it's 

Yeah, that "persistent USB" stuff that I don't like at all!   Power off
is power off, and should be treated just like any other case of the USB
circuit being broken.  (Offtopic here, of course.)


> necessary for the corresponding EHCI root hub to be resumed first so that 
> the port's OWNER bit can be set properly.
> 
> It works fine as long as devices are resumed in order of registration,
> because a low- or full-speed USB device can't be registered before the
> high-speed root hub.  (If it was, it would be disconnected when the
> high-speed root hub initialized and took control of the port.)  So this
> isn't a "just happened to work" situation; it really is a critical
> ordering dependency.

No, it's a "just happened to work" because the only ordering promise
that was explicitly made is that the parent/child relationships will
be obeyed.  Any additional ordering dependency is out-of-scope of the
current driver model -- and that's a proble that eventually needs to
be fixed.

This is the kind of thing that the pm_parent relationship was (AFAICT)
originally supposed to handle.  Of course, it doesn't/can't, given the
current implementation ... that relationship is never used.


> The clock source problem can be solved in other ways.  For instance, right 
> now things are set up so that clock sources are registered at various 
> random times and the clockevents code adapts to use the best available 
> device, right?.

Clocksource ~= counter, clockevent ~= timer irq ... you're mixing the
two, they're different!  Both can be registered at various times; and
the best available instance of both is used.  (Unfortunately "best" is
not a static policy on x86 hardware.)


> So simply arrange for a clock source to "depart" as it is  
> suspended; then clockevents can fall back to using other lower-precision 
> sources.  As long as devices suspend in reverse order of discovery, the 
> system should always function properly.

It's not that simple though, especially with HPET.  The BIOS may expect
the PIT to work, but Linux currently (and problematically!) uses HPET in
"legacy replacement mode".  And ISTR the problems are coming up when the
system is already in a low-functionality state:  IRQs off everywhere,
even timer ticks have stopped.

- Dave

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] Add suspend/resume for HPET
@ 2007-04-02 19:16   ` David Brownell
  0 siblings, 0 replies; 17+ messages in thread
From: David Brownell @ 2007-04-02 19:16 UTC (permalink / raw)
  To: Alan Stern
  Cc: Linux-pm mailing list, Maxim Levitsky, Ingo Molnar,
	Linus Torvalds, Thomas Gleixner, Kernel development list

On Monday 02 April 2007 7:16 am, Alan Stern wrote:
> On Saturday March 31, 2007, David Brownell wrote:
> 
> > Change how the PM list is constructed, so that devices are added right
> > after their parents (when they have one) rather than at the end of the
> > list.  ...
> > 
> > This patch has a potential downside for devices that have multiple
> > power dependencies and which "just happened to work" before.
> 
> Dave:
> 
> Would you mind retracting this patch?  It will interfere with some work
> I've been doing in USB, work that relies on exactly the sort of multiple 
> power dependency you mention.

It's out there for discussion right now more than merging.

What the driver model does now is problematic.  Even if the clockevent
stuff gets different fixes, those driver model issues still exist:  the
existence of dependencies that are not part of the device tree.


> The problem is this: When a low- or full-speed USB device is resumed 
> following a power loss (this is part of the "persistent USB" patch), it's 

Yeah, that "persistent USB" stuff that I don't like at all!   Power off
is power off, and should be treated just like any other case of the USB
circuit being broken.  (Offtopic here, of course.)


> necessary for the corresponding EHCI root hub to be resumed first so that 
> the port's OWNER bit can be set properly.
> 
> It works fine as long as devices are resumed in order of registration,
> because a low- or full-speed USB device can't be registered before the
> high-speed root hub.  (If it was, it would be disconnected when the
> high-speed root hub initialized and took control of the port.)  So this
> isn't a "just happened to work" situation; it really is a critical
> ordering dependency.

No, it's a "just happened to work" because the only ordering promise
that was explicitly made is that the parent/child relationships will
be obeyed.  Any additional ordering dependency is out-of-scope of the
current driver model -- and that's a proble that eventually needs to
be fixed.

This is the kind of thing that the pm_parent relationship was (AFAICT)
originally supposed to handle.  Of course, it doesn't/can't, given the
current implementation ... that relationship is never used.


> The clock source problem can be solved in other ways.  For instance, right 
> now things are set up so that clock sources are registered at various 
> random times and the clockevents code adapts to use the best available 
> device, right?.

Clocksource ~= counter, clockevent ~= timer irq ... you're mixing the
two, they're different!  Both can be registered at various times; and
the best available instance of both is used.  (Unfortunately "best" is
not a static policy on x86 hardware.)


> So simply arrange for a clock source to "depart" as it is  
> suspended; then clockevents can fall back to using other lower-precision 
> sources.  As long as devices suspend in reverse order of discovery, the 
> system should always function properly.

It's not that simple though, especially with HPET.  The BIOS may expect
the PIT to work, but Linux currently (and problematically!) uses HPET in
"legacy replacement mode".  And ISTR the problems are coming up when the
system is already in a low-functionality state:  IRQs off everywhere,
even timer ticks have stopped.

- Dave

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-04-02 19:16   ` David Brownell
@ 2007-04-02 20:04     ` Alan Stern
  -1 siblings, 0 replies; 17+ messages in thread
From: Alan Stern @ 2007-04-02 20:04 UTC (permalink / raw)
  To: David Brownell
  Cc: Maxim Levitsky, Ingo Molnar, Linus Torvalds, Greg KH,
	Thomas Gleixner, Kernel development list, Linux-pm mailing list

On Mon, 2 Apr 2007, David Brownell wrote:

> What the driver model does now is problematic.  Even if the clockevent
> stuff gets different fixes, those driver model issues still exist:  the
> existence of dependencies that are not part of the device tree.

> No, it's a "just happened to work" because the only ordering promise
> that was explicitly made is that the parent/child relationships will
> be obeyed.  Any additional ordering dependency is out-of-scope of the
> current driver model -- and that's a proble that eventually needs to
> be fixed.
> 
> This is the kind of thing that the pm_parent relationship was (AFAICT)
> originally supposed to handle.  Of course, it doesn't/can't, given the
> current implementation ... that relationship is never used.

Just so.  In fact, there almost certainly are other dependencies that 
nobody is aware of, simply because they have never had a chance to bite.

Such things can be rather difficult to pin down when they occur.  I would
be happy enough to leave matters as they are, with a strict LIFO approach.  
If someone ever tries to parallelize suspend/resume in multiple threads,
they will have their work cut out for them -- even probing ran into
trouble when attempts were made to parallelize it, and it's a lot simpler.


> It's not that simple though, especially with HPET.  The BIOS may expect
> the PIT to work, but Linux currently (and problematically!) uses HPET in
> "legacy replacement mode".  And ISTR the problems are coming up when the
> system is already in a low-functionality state:  IRQs off everywhere,
> even timer ticks have stopped.

I know nothing about the workings of the HPET and other clock code.  My 
point was this: Suspend passes through various intermediate stages in 
which some devices are available and others aren't.  So long as those 
stages are exact duplicates (in reverse order) of the stages that occurred 
during startup, it should be possible to make them all work.

Alan Stern


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] Add suspend/resume for HPET
@ 2007-04-02 20:04     ` Alan Stern
  0 siblings, 0 replies; 17+ messages in thread
From: Alan Stern @ 2007-04-02 20:04 UTC (permalink / raw)
  To: David Brownell
  Cc: Linux-pm mailing list, Maxim Levitsky, Ingo Molnar,
	Linus Torvalds, Thomas Gleixner, Kernel development list

On Mon, 2 Apr 2007, David Brownell wrote:

> What the driver model does now is problematic.  Even if the clockevent
> stuff gets different fixes, those driver model issues still exist:  the
> existence of dependencies that are not part of the device tree.

> No, it's a "just happened to work" because the only ordering promise
> that was explicitly made is that the parent/child relationships will
> be obeyed.  Any additional ordering dependency is out-of-scope of the
> current driver model -- and that's a proble that eventually needs to
> be fixed.
> 
> This is the kind of thing that the pm_parent relationship was (AFAICT)
> originally supposed to handle.  Of course, it doesn't/can't, given the
> current implementation ... that relationship is never used.

Just so.  In fact, there almost certainly are other dependencies that 
nobody is aware of, simply because they have never had a chance to bite.

Such things can be rather difficult to pin down when they occur.  I would
be happy enough to leave matters as they are, with a strict LIFO approach.  
If someone ever tries to parallelize suspend/resume in multiple threads,
they will have their work cut out for them -- even probing ran into
trouble when attempts were made to parallelize it, and it's a lot simpler.


> It's not that simple though, especially with HPET.  The BIOS may expect
> the PIT to work, but Linux currently (and problematically!) uses HPET in
> "legacy replacement mode".  And ISTR the problems are coming up when the
> system is already in a low-functionality state:  IRQs off everywhere,
> even timer ticks have stopped.

I know nothing about the workings of the HPET and other clock code.  My 
point was this: Suspend passes through various intermediate stages in 
which some devices are available and others aren't.  So long as those 
stages are exact duplicates (in reverse order) of the stages that occurred 
during startup, it should be possible to make them all work.

Alan Stern

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-04-02 20:04     ` Alan Stern
@ 2007-04-03  5:54       ` Thomas Gleixner
  -1 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2007-04-03  5:54 UTC (permalink / raw)
  To: Alan Stern
  Cc: David Brownell, Maxim Levitsky, Ingo Molnar, Linus Torvalds,
	Greg KH, Kernel development list, Linux-pm mailing list

On Mon, 2007-04-02 at 16:04 -0400, Alan Stern wrote:
> > It's not that simple though, especially with HPET.  The BIOS may expect
> > the PIT to work, but Linux currently (and problematically!) uses HPET in
> > "legacy replacement mode".  And ISTR the problems are coming up when the
> > system is already in a low-functionality state:  IRQs off everywhere,
> > even timer ticks have stopped.
> 
> I know nothing about the workings of the HPET and other clock code.  My 
> point was this: Suspend passes through various intermediate stages in 
> which some devices are available and others aren't.  So long as those 
> stages are exact duplicates (in reverse order) of the stages that occurred 
> during startup, it should be possible to make them all work.

Unfortunately it is not a fully linear problem. Devices are initialized
late and put the system into a more complex state (i.e. dynticks,
highres) which needs to be suspended and resumed. If we want to do this
completely linear we need to do a full reverse rollback of the system
states, which moves even more complexity into such systems.

Also the linear approach is not working with other devices, as one can
see with the still unresolved "IRQ#X nobody cared" issues at resume,
which break my laptop. It works nice on startup of the system, but
breaks on resume.

	tglx



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] Add suspend/resume for HPET
@ 2007-04-03  5:54       ` Thomas Gleixner
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2007-04-03  5:54 UTC (permalink / raw)
  To: Alan Stern
  Cc: Linux-pm mailing list, Maxim Levitsky, Ingo Molnar,
	Linus Torvalds, Kernel development list

On Mon, 2007-04-02 at 16:04 -0400, Alan Stern wrote:
> > It's not that simple though, especially with HPET.  The BIOS may expect
> > the PIT to work, but Linux currently (and problematically!) uses HPET in
> > "legacy replacement mode".  And ISTR the problems are coming up when the
> > system is already in a low-functionality state:  IRQs off everywhere,
> > even timer ticks have stopped.
> 
> I know nothing about the workings of the HPET and other clock code.  My 
> point was this: Suspend passes through various intermediate stages in 
> which some devices are available and others aren't.  So long as those 
> stages are exact duplicates (in reverse order) of the stages that occurred 
> during startup, it should be possible to make them all work.

Unfortunately it is not a fully linear problem. Devices are initialized
late and put the system into a more complex state (i.e. dynticks,
highres) which needs to be suspended and resumed. If we want to do this
completely linear we need to do a full reverse rollback of the system
states, which moves even more complexity into such systems.

Also the linear approach is not working with other devices, as one can
see with the still unresolved "IRQ#X nobody cared" issues at resume,
which break my laptop. It works nice on startup of the system, but
breaks on resume.

	tglx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-04-02 20:04     ` Alan Stern
@ 2007-04-04 15:06       ` David Brownell
  -1 siblings, 0 replies; 17+ messages in thread
From: David Brownell @ 2007-04-04 15:06 UTC (permalink / raw)
  To: Alan Stern
  Cc: Maxim Levitsky, Ingo Molnar, Linus Torvalds, Greg KH,
	Thomas Gleixner, Kernel development list, Linux-pm mailing list

On Monday 02 April 2007 1:04 pm, Alan Stern wrote:
> On Mon, 2 Apr 2007, David Brownell wrote:

> > This is the kind of thing that the pm_parent relationship was (AFAICT)
> > originally supposed to handle.  Of course, it doesn't/can't, given the
> > current implementation ... that relationship is never used.
> 
> Just so.  In fact, there almost certainly are other dependencies that 
> nobody is aware of, simply because they have never had a chance to bite.

In any given system, yes there are bugs lurking.  But I was more concerned
with a provably wrong assumption made by the current framework.  Such things
cause cascading fragility.

As Thomas mentioned, HPET isn't the only place where a "linear" model fails.


> Such things can be rather difficult to pin down when they occur.  I would
> be happy enough to leave matters as they are, with a strict LIFO approach.

I wouldn't.  Much better to have a solid handle on the interdependencies
than to need to cope, long term, with a framework that doesn't allow that.

Remember also that a LIFO model assumes that there's only one sequence by
which the hardware powers up/down ... i.e. that there's no runtime PM going
on, whereby large chunks are regularly powered down/up based on usage.
Better runtime PM becomes more important as system complexity rises.

- Dave

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] Add suspend/resume for HPET
@ 2007-04-04 15:06       ` David Brownell
  0 siblings, 0 replies; 17+ messages in thread
From: David Brownell @ 2007-04-04 15:06 UTC (permalink / raw)
  To: Alan Stern
  Cc: Linux-pm mailing list, Maxim Levitsky, Ingo Molnar,
	Linus Torvalds, Thomas Gleixner, Kernel development list

On Monday 02 April 2007 1:04 pm, Alan Stern wrote:
> On Mon, 2 Apr 2007, David Brownell wrote:

> > This is the kind of thing that the pm_parent relationship was (AFAICT)
> > originally supposed to handle.  Of course, it doesn't/can't, given the
> > current implementation ... that relationship is never used.
> 
> Just so.  In fact, there almost certainly are other dependencies that 
> nobody is aware of, simply because they have never had a chance to bite.

In any given system, yes there are bugs lurking.  But I was more concerned
with a provably wrong assumption made by the current framework.  Such things
cause cascading fragility.

As Thomas mentioned, HPET isn't the only place where a "linear" model fails.


> Such things can be rather difficult to pin down when they occur.  I would
> be happy enough to leave matters as they are, with a strict LIFO approach.

I wouldn't.  Much better to have a solid handle on the interdependencies
than to need to cope, long term, with a framework that doesn't allow that.

Remember also that a LIFO model assumes that there's only one sequence by
which the hardware powers up/down ... i.e. that there's no runtime PM going
on, whereby large chunks are regularly powered down/up based on usage.
Better runtime PM becomes more important as system complexity rises.

- Dave

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-04-01  3:13           ` Jeff Chua
@ 2007-04-01  4:13             ` David Brownell
  0 siblings, 0 replies; 17+ messages in thread
From: David Brownell @ 2007-04-01  4:13 UTC (permalink / raw)
  To: Jeff Chua
  Cc: linux-pm, Adrian Bunk, Andrew Morton, Eric W. Biederman, gregkh,
	Ingo Molnar, Jens Axboe, jgarzik, Linus Torvalds, linux-acpi,
	linux-ide, Linux Kernel Mailing List, linux-pci, Maxim Levitsky,
	Michael S. Tsirkin, Sergei Shtylyov, Thomas Gleixner

On Saturday 31 March 2007 8:13 pm, Jeff Chua wrote:
> On 4/1/07, David Brownell <david-b@pacbell.net> wrote:
> > for those will all get grouped together ... suspended "very late" and
> > resumed "very early", regardless of when they get registered.  Pretty
> > much the driver model parts of what Linus was suggesting; clockevent
> > bits would still be needed.
> 
> I tested your patch with CONFIG_NO_HZ=y, but it still hangs while
> suspending to disk (before the percent saving).

Of course.  No clockevent updates...


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-03-31 19:32         ` David Brownell
@ 2007-04-01  3:13           ` Jeff Chua
  2007-04-01  4:13             ` David Brownell
  0 siblings, 1 reply; 17+ messages in thread
From: Jeff Chua @ 2007-04-01  3:13 UTC (permalink / raw)
  To: David Brownell
  Cc: linux-pm, Adrian Bunk, Andrew Morton, Eric W. Biederman, gregkh,
	Ingo Molnar, Jens Axboe, jgarzik, Linus Torvalds, linux-acpi,
	linux-ide, Linux Kernel Mailing List, linux-pci, Maxim Levitsky,
	Michael S. Tsirkin, Sergei Shtylyov, Thomas Gleixner

On 4/1/07, David Brownell <david-b@pacbell.net> wrote:
> for those will all get grouped together ... suspended "very late" and
> resumed "very early", regardless of when they get registered.  Pretty
> much the driver model parts of what Linus was suggesting; clockevent
> bits would still be needed.

I tested your patch with CONFIG_NO_HZ=y, but it still hangs while
suspending to disk (before the percent saving).

But one discovery. I get tired of all these hangs, so I decided to
press some keys and the power button. Accidentally, the suspend
proceeded and successfully suspended!

I tried many more times, and discovered that to proceed with the
suspend, hit any 4 keys slowly. (e.g. "F1 F2 F3 F4", or "1 2 3 4").

My .config has CONFIG_NO_HZ=y and CONFIG_HIGH_RES_TIMERS=y, but I
suppose CONFIG_HIGH_RES_TIMERS=y gots nothing to do with the hang.

I went back with my vanilla 2.6.21-rc5 with Maxim's patch, and with
hitting the keys, I could suspend to disk with CONFIG_NO_HZ=y and
CONFIG_HIGH_RES_TIMERS=y.

Jeff.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-03-31 18:18       ` [linux-pm] " David Brownell
@ 2007-03-31 19:32         ` David Brownell
  2007-04-01  3:13           ` Jeff Chua
  0 siblings, 1 reply; 17+ messages in thread
From: David Brownell @ 2007-03-31 19:32 UTC (permalink / raw)
  To: linux-pm
  Cc: Adrian Bunk, Andrew Morton, Eric W. Biederman, gregkh,
	Ingo Molnar, Jeff Chua, Jens Axboe, jgarzik, Linus Torvalds,
	linux-acpi, linux-ide, Linux Kernel Mailing List, linux-pci,
	Maxim Levitsky, Michael S. Tsirkin, Sergei Shtylyov,
	Thomas Gleixner

On Saturday 31 March 2007 11:18 am, David Brownell wrote:
> ( please remove obsolute linux-pm@lists.osdl.org  from further messages!! )
> 
> On Saturday 31 March 2007 10:02 am, Ingo Molnar wrote:
> > 
> > i dont think there's any particular problem here because suspend/resume 
> > wont be done during bootup - but we might need a way to move a device to 
> > earlier spots in the device tree, even if they got registered later on - 
> > instead of forcing the time devices to be registered very early?
> 
> I'm about ready to test the appended patch... a "move one device" call
> might be safest at this point in the release cycle though.

As expected, this behaved OK on an x86 laptop.  I'll see if it breaks
some of the ARM boards I have handy.

To be clear, what this means is that if "clockevent" and "clocksource"
devices get registered "very early", and the various clockevent and
clock source devices get registered, then the suspend/resume methods
for those will all get grouped together ... suspended "very late" and
resumed "very early", regardless of when they get registered.  Pretty
much the driver model parts of what Linus was suggesting; clockevent
bits would still be needed.

- Dave



> - Dave
> 
> ========================	SNIP!
> Change how the PM list is constructed, so that devices are added right
> after their parents (when they have one) rather than at the end of the
> list.  This preserves sequencing guarantees, but enables sequencing of
> suspend/resume operations by more important characteristics than "when
> device happened to enumerate" ... e.g. clocksources and clockevents
> at a clearly defined point during suspend and resume.
> 
> This patch has a potential downside for devices that have multiple
> power dependencies and which "just happened to work" before.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> 
> --- g26.orig/drivers/base/power/main.c	2006-07-02 12:30:30.000000000 -0700
> +++ g26/drivers/base/power/main.c	2007-03-31 11:02:28.000000000 -0700
> @@ -52,12 +52,17 @@ EXPORT_SYMBOL_GPL(device_pm_set_parent);
>  int device_pm_add(struct device * dev)
>  {
>  	int error;
> +	struct device *parent = dev->parent;
>  
> -	pr_debug("PM: Adding info for %s:%s\n",
> -		 dev->bus ? dev->bus->name : "No Bus", dev->kobj.name);
> +	pr_debug("PM: Adding info for %s:%s, after %s\n",
> +		 dev->bus ? dev->bus->name : "No Bus", dev->kobj.name,
> +		 parent ? parent->bus_id : "(no parent)");
>  	down(&dpm_list_sem);
> -	list_add_tail(&dev->power.entry, &dpm_active);
> -	device_pm_set_parent(dev, dev->parent);
> +	if (parent)
> +		list_add(&dev->power.entry, &parent->power.entry);
> +	else
> +		list_add_tail(&dev->power.entry, &dpm_active);
> +	device_pm_set_parent(dev, parent);
>  	if ((error = dpm_sysfs_add(dev)))
>  		list_del(&dev->power.entry);
>  	up(&dpm_list_sem);
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-03-31 17:02     ` Ingo Molnar
@ 2007-03-31 18:18       ` David Brownell
  2007-03-31 19:32         ` David Brownell
  0 siblings, 1 reply; 17+ messages in thread
From: David Brownell @ 2007-03-31 18:18 UTC (permalink / raw)
  To: linux-pm
  Cc: Adrian Bunk, Andrew Morton, Eric W. Biederman, gregkh,
	Ingo Molnar, Jeff Chua, Jens Axboe, jgarzik, Linus Torvalds,
	linux-acpi, linux-ide, Linux Kernel Mailing List, linux-pci,
	Maxim Levitsky, Michael S. Tsirkin, Sergei Shtylyov,
	Thomas Gleixner

( please remove obsolute linux-pm@lists.osdl.org  from further messages!! )

On Saturday 31 March 2007 10:02 am, Ingo Molnar wrote:
> 
> i dont think there's any particular problem here because suspend/resume 
> wont be done during bootup - but we might need a way to move a device to 
> earlier spots in the device tree, even if they got registered later on - 
> instead of forcing the time devices to be registered very early?

I'm about ready to test the appended patch... a "move one device" call
might be safest at this point in the release cycle though.

- Dave

========================	SNIP!
Change how the PM list is constructed, so that devices are added right
after their parents (when they have one) rather than at the end of the
list.  This preserves sequencing guarantees, but enables sequencing of
suspend/resume operations by more important characteristics than "when
device happened to enumerate" ... e.g. clocksources and clockevents
at a clearly defined point during suspend and resume.

This patch has a potential downside for devices that have multiple
power dependencies and which "just happened to work" before.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

--- g26.orig/drivers/base/power/main.c	2006-07-02 12:30:30.000000000 -0700
+++ g26/drivers/base/power/main.c	2007-03-31 11:02:28.000000000 -0700
@@ -52,12 +52,17 @@ EXPORT_SYMBOL_GPL(device_pm_set_parent);
 int device_pm_add(struct device * dev)
 {
 	int error;
+	struct device *parent = dev->parent;
 
-	pr_debug("PM: Adding info for %s:%s\n",
-		 dev->bus ? dev->bus->name : "No Bus", dev->kobj.name);
+	pr_debug("PM: Adding info for %s:%s, after %s\n",
+		 dev->bus ? dev->bus->name : "No Bus", dev->kobj.name,
+		 parent ? parent->bus_id : "(no parent)");
 	down(&dpm_list_sem);
-	list_add_tail(&dev->power.entry, &dpm_active);
-	device_pm_set_parent(dev, dev->parent);
+	if (parent)
+		list_add(&dev->power.entry, &parent->power.entry);
+	else
+		list_add_tail(&dev->power.entry, &dpm_active);
+	device_pm_set_parent(dev, parent);
 	if ((error = dpm_sysfs_add(dev)))
 		list_del(&dev->power.entry);
 	up(&dpm_list_sem);

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
  2007-03-31 16:53   ` Linus Torvalds
@ 2007-03-31 17:55       ` David Brownell
  2007-03-31 17:55       ` David Brownell
  1 sibling, 0 replies; 17+ messages in thread
From: David Brownell @ 2007-03-31 17:55 UTC (permalink / raw)
  To: linux-pm
  Cc: Linus Torvalds, Thomas Gleixner, Maxim Levitsky, Jeff Chua,
	linux-ide, Sergei Shtylyov, gregkh, linux-pm,
	Linux Kernel Mailing List, Adrian Bunk, linux-acpi, linux-pci,
	Eric W. Biederman, Jens Axboe, Michael S. Tsirkin, Ingo Molnar,
	jgarzik, Andrew Morton

On Saturday 31 March 2007 9:53 am, Linus Torvalds wrote:
> 
> On Sat, 31 Mar 2007, Thomas Gleixner wrote:
> > 
> > Right, but clock - sources/events need to be extremly late suspended and
> > early resumed. How can we ensure this ?
> 
> Make them be at the top of the device tree by adding them early. That's 
> the whole point of the device tree after all - we have an ordering that is 
> enforced by its topology, and that we sort by when things were added.

Right, but "when things get added" doesn't correlate well to
"when they should get suspended/resumed".  It's also in basic
conflict with runtime PM models, where devices may be suspended
at essentially any time.  And sysdevs are even stranger.


One way out:  rather than constructing that list as devices get
enumerated, it could be constructed by a (linear-time, non-recursive)
walk of the device tree(s) before they get suspended.

(Or equivalently:  construct lists at enumeration time, but just
adding them *right after their parent* rather than at the end of
the list.)

Would that solve the problem here?  Potentially ... if the tree is
structured to meet Thomas' rules:

> > The required resume order is:
> > 
> > clocksources
> > timekeeping
> > clockevents
> > tick management

> So the only thing that needs to be done is to make sure that we add the 
> timer devices early during bootup - something we have to do *anyway*. If a 
> device is added early in bootup, that automatically means that it will be 
> suspended late, and resumed early - because we maintain that order all the 
> way through..

>             -- "clocksource" -- +-- HPET
>                                 |
>                                 +-- TSC
>                                 |
>                                 +-- i8259
>                                 |
>                                 +-- lapic timer
>                                 |
>                                 .. whatever else

If each of those were a device node, with "clocksource" suspend/resume
methods handling Thomas' "timekeeping" item, and simlarly for "clockevent"
devices ... I could see that all working neatly.

- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [linux-pm] [PATCH v2] Add suspend/resume for HPET
@ 2007-03-31 17:55       ` David Brownell
  0 siblings, 0 replies; 17+ messages in thread
From: David Brownell @ 2007-03-31 17:55 UTC (permalink / raw)
  To: linux-pm
  Cc: Linus Torvalds, Thomas Gleixner, Maxim Levitsky, Jeff Chua,
	linux-ide, Sergei Shtylyov, gregkh, linux-pm,
	Linux Kernel Mailing List, Adrian Bunk, linux-acpi, linux-pci,
	Eric W. Biederman, Jens Axboe, Michael S. Tsirkin, Ingo Molnar,
	jgarzik, Andrew Morton

On Saturday 31 March 2007 9:53 am, Linus Torvalds wrote:
> 
> On Sat, 31 Mar 2007, Thomas Gleixner wrote:
> > 
> > Right, but clock - sources/events need to be extremly late suspended and
> > early resumed. How can we ensure this ?
> 
> Make them be at the top of the device tree by adding them early. That's 
> the whole point of the device tree after all - we have an ordering that is 
> enforced by its topology, and that we sort by when things were added.

Right, but "when things get added" doesn't correlate well to
"when they should get suspended/resumed".  It's also in basic
conflict with runtime PM models, where devices may be suspended
at essentially any time.  And sysdevs are even stranger.


One way out:  rather than constructing that list as devices get
enumerated, it could be constructed by a (linear-time, non-recursive)
walk of the device tree(s) before they get suspended.

(Or equivalently:  construct lists at enumeration time, but just
adding them *right after their parent* rather than at the end of
the list.)

Would that solve the problem here?  Potentially ... if the tree is
structured to meet Thomas' rules:

> > The required resume order is:
> > 
> > clocksources
> > timekeeping
> > clockevents
> > tick management

> So the only thing that needs to be done is to make sure that we add the 
> timer devices early during bootup - something we have to do *anyway*. If a 
> device is added early in bootup, that automatically means that it will be 
> suspended late, and resumed early - because we maintain that order all the 
> way through..

>             -- "clocksource" -- +-- HPET
>                                 |
>                                 +-- TSC
>                                 |
>                                 +-- i8259
>                                 |
>                                 +-- lapic timer
>                                 |
>                                 .. whatever else

If each of those were a device node, with "clocksource" suspend/resume
methods handling Thomas' "timekeeping" item, and simlarly for "clockevent"
devices ... I could see that all working neatly.

- Dave


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2007-04-04 16:33 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-04-02 14:16 [linux-pm] [PATCH v2] Add suspend/resume for HPET Alan Stern
2007-04-02 17:09 ` Linus Torvalds
2007-04-02 17:09   ` Linus Torvalds
2007-04-02 19:16 ` [linux-pm] " David Brownell
2007-04-02 19:16   ` David Brownell
2007-04-02 20:04   ` [linux-pm] " Alan Stern
2007-04-02 20:04     ` Alan Stern
2007-04-03  5:54     ` [linux-pm] " Thomas Gleixner
2007-04-03  5:54       ` Thomas Gleixner
2007-04-04 15:06     ` [linux-pm] " David Brownell
2007-04-04 15:06       ` David Brownell
  -- strict thread matches above, loose matches on Subject: below --
2007-03-16 16:33 Linux 2.6.21-rc4 Linus Torvalds
2007-03-31 16:33 ` [PATCH v2] Add suspend/resume for HPET Thomas Gleixner
2007-03-31 16:53   ` Linus Torvalds
2007-03-31 17:02     ` Ingo Molnar
2007-03-31 18:18       ` [linux-pm] " David Brownell
2007-03-31 19:32         ` David Brownell
2007-04-01  3:13           ` Jeff Chua
2007-04-01  4:13             ` David Brownell
2007-03-31 17:55     ` David Brownell
2007-03-31 17:55       ` David Brownell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.