Hi!
As far as I can see, I'm still maintainer of software suspend. That
did not stop you from crying "split those patches" when I tried to
submit changes to my code, and you were pretty pissed off when I tried
to push trivial one liners without contacting maintainers.
And now you pushed ton of crap into Linus' tree, breaking userland
interfaces in the stable series (/proc/acpi/sleep), killing copyrights
(Andy Grover has copyright on drivers/acpi/sleep/main.c), and
rewriting code without even sending diff to maintainer (no, I did not
see a mail from you, and you modified swsusp heavily). You did not
bother to send code to the lists, so that noone could review it.
Great. This way we are going to have stable PM code... in 2056.
Linus, could you make sure Patricks patches are at least reviewed on
the lists?
Pavel
--- a/include/linux/pm.h Fri Aug 15 01:15:23 2003
+++ b/include/linux/pm.h Mon Aug 18 15:31:58 2003
@@ -186,8 +186,30 @@
#endif /* CONFIG_PM */
+
+/*
+ * Callbacks for platform drivers to implement.
+ */
extern void (*pm_idle)(void);
extern void (*pm_power_off)(void);
+
+enum {
+ PM_SUSPEND_ON,
+ PM_SUSPEND_STANDBY,
+ PM_SUSPEND_MEM,
+ PM_SUSPEND_DISK,
+ PM_SUSPEND_MAX,
+};
+
+extern int (*pm_power_down)(u32 state);
If you defined enum, you should also use it.
@@ -1114,7 +986,8 @@
static int __init resume_setup(char *str)
{
- strncpy( resume_file, str, 255 );
+ if (strlen(str))
+ strncpy(resume_file, str, 255);
return 1;
}
Why are you obfuscating the code?
You changed return type of do_magic() to int, but did not bother to
update assembly code, as far as I can see. Did you test those changes?
+Some devices are broken and will inevitably have problems powering
+down or disabling themselves with interrupts enabled. For these
+special cases, they may return -EAGAIN. This will put the device on a
+list to be taken care of later. When interrupts are disabled, before
+we enter the low-power state, their drivers are called again to put
+their device to sleep.
Returning EAGAIN to be called with interrupts disabled is extremely
ugly hack. We were passing suspend level before. Why did you have to
break it?
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
> As far as I can see, I'm still maintainer of software suspend. That
> did not stop you from crying "split those patches" when I tried to
> submit changes to my code, and you were pretty pissed off when I tried
> to push trivial one liners without contacting maintainers.
Ok, I'm sorry. I should not have broken anything in your code, and
actually significantly improved it.
> And now you pushed ton of crap into Linus' tree, breaking userland
> interfaces in the stable series (/proc/acpi/sleep)
/proc/acpi/sleep is useless. For one, we want an interface that handles
quiescing the system that is not tied to a particular low-level details.
The ACPI bits are trivial. And, you really don't want an interface to
swsusp that depends on ACPI, do you?
On top of that, the way it was implemented was broken. You could not
actually enter a low-power state (S4) if you used swsusp. You had to
shutdown the system
> killing copyrights
> (Andy Grover has copyright on drivers/acpi/sleep/main.c)
Everything in that file, with the exception of one line fixes/checks, is
mine, from either 2 years ago or now.
> rewriting code without even sending diff to maintainer (no, I did not
> see a mail from you, and you modified swsusp heavily).
I removed code from swsusp and moved into a central, shared location. I
apoloigize for the hypocrasy on my part, but I contend that the result is
much prettier.
> Great. This way we are going to have stable PM code... in 2056.
Yes, but we should also have it a lot sooner than that.
Note that we have never had stable PM code; we've had crap. It is a lot
more stable now, based solely on the fact that someone has actually taken
the time to look at it, clean it up, and start fixing it.
What is your idea of stability? The point when all the people that report
bugs to you, and you reply 'Fix it yourself' actually buckle down and fix
all the problems? Or, when someone steps up and tries to make it work
reliably for a majority of users?
My intent is to do that, and to do it soon. And, with a minimal amount of
pain during the transitions.
> +
> +enum {
> + PM_SUSPEND_ON,
> + PM_SUSPEND_STANDBY,
> + PM_SUSPEND_MEM,
> + PM_SUSPEND_DISK,
> + PM_SUSPEND_MAX,
> +};
> +
> +extern int (*pm_power_down)(u32 state);
>
> If you defined enum, you should also use it.
As a typedef parameter to the function?
> static int __init resume_setup(char *str)
> {
> - strncpy( resume_file, str, 255 );
> + if (strlen(str))
> + strncpy(resume_file, str, 255);
> return 1;
> }
>
> Why are you obfuscating the code?
Eh? First, why would you want to copy a NULL string?
Secondly, you can actually remove the second command line parameter
("noresume") by simply specifying a NULL partition to this parameter. It
requires about a 5-line change, and makes things simpler.
> You changed return type of do_magic() to int, but did not bother to
> update assembly code, as far as I can see. Did you test those changes?
I noticed that, and it's already fixed.
> +Some devices are broken and will inevitably have problems powering
> +down or disabling themselves with interrupts enabled. For these
> +special cases, they may return -EAGAIN. This will put the device on a
> +list to be taken care of later. When interrupts are disabled, before
> +we enter the low-power state, their drivers are called again to put
> +their device to sleep.
>
> Returning EAGAIN to be called with interrupts disabled is extremely
> ugly hack. We were passing suspend level before. Why did you have to
> break it?
Because you can power down most devices with interrupts enabled, and you
really want to. Especially for devices that support runtime power
management, which by definition, requires interrupts to always be enabled.
-EAGAIN allows the drivers/devices that really need special care to
specify it. Otherwise, we'll end up calling ->suspend() twice for power
down for each device (those that can do w/ interrupts enabled and those
that need interrupts disabled), which also requires every single driver to
check whether or not interrupts are enabled, instead of just those that
need it.
Pat
Pavel Machek wrote:
> static int __init resume_setup(char *str)
> {
> - strncpy( resume_file, str, 255 );
> + if (strlen(str))
> + strncpy(resume_file, str, 255);
> return 1;
> }
Silly me, but if you want to check to be sure a string has a length
greater than zero, wouldn't it be faster to just check to be sure that
the first byte is not zero?
Think about how much work it does if the string is 100 characters long
when all you want to do is determine that it's non-zero.
Hi!
> On top of that, the way it was implemented was broken. You could not
> actually enter a low-power state (S4) if you used swsusp. You had to
> shutdown the system
This is stable series, and /proc/acpi/sleep was fine for at least
entering S3 and swsusp. Anyway, if you killed sleep, you should kill
alarm as well. Its only usefull for sleeping, and IIRC it never worked
properly, anyway.
> > Great. This way we are going to have stable PM code... in 2056.
>
> Yes, but we should also have it a lot sooner than that.
>
> Note that we have never had stable PM code; we've had crap. It is a lot
> more stable now, based solely on the fact that someone has actually taken
> the time to look at it, clean it up, and start fixing it.
>
> What is your idea of stability? The point when all the people that report
> bugs to you, and you reply 'Fix it yourself' actually buckle down and fix
> all the problems? Or, when someone steps up and tries to make it work
> reliably for a majority of users?
I already tried to make it reliable for users, and you moving chunks
of code back and forth while changing semantics of device_suspend() is
not really helpfull.
If you want to help, take a look at drivers/pci/power.c. That file
should not need to exist, but if I kill it bad stuff happens after
resume. Killing pm_register() and friends would be nice.
> My intent is to do that, and to do it soon. And, with a minimal amount of
> pain during the transitions.
Try to post patches to the lists, then, and avoid moving code just
because you can.
> > +enum {
> > + PM_SUSPEND_ON,
> > + PM_SUSPEND_STANDBY,
> > + PM_SUSPEND_MEM,
> > + PM_SUSPEND_DISK,
> > + PM_SUSPEND_MAX,
> > +};
> > +
> > +extern int (*pm_power_down)(u32 state);
> >
> > If you defined enum, you should also use it.
>
> As a typedef parameter to the function?
what about enum action { }; extern int (*pm_power_down)(enum action
state)?
> > static int __init resume_setup(char *str)
> > {
> > - strncpy( resume_file, str, 255 );
> > + if (strlen(str))
> > + strncpy(resume_file, str, 255);
> > return 1;
> > }
> >
> > Why are you obfuscating the code?
>
> Eh? First, why would you want to copy a NULL string?
>
> Secondly, you can actually remove the second command line parameter
> ("noresume") by simply specifying a NULL partition to this parameter. It
> requires about a 5-line change, and makes things simpler.
You'd better not. You are expected to have one "resume=/foo/bar"
specified as append in lilo. You want to able to say noresume and do
one boot without resuming. Turning resume with
"resume=/dev/nonexistent" would be playing roulete with command line
argument order.
> > +Some devices are broken and will inevitably have problems powering
> > +down or disabling themselves with interrupts enabled. For these
> > +special cases, they may return -EAGAIN. This will put the device on a
> > +list to be taken care of later. When interrupts are disabled, before
> > +we enter the low-power state, their drivers are called again to put
> > +their device to sleep.
> >
> > Returning EAGAIN to be called with interrupts disabled is extremely
> > ugly hack. We were passing suspend level before. Why did you have to
> > break it?
>
> Because you can power down most devices with interrupts enabled, and you
> really want to. Especially for devices that support runtime power
> management, which by definition, requires interrupts to always be enabled.
>
> -EAGAIN allows the drivers/devices that really need special care to
> specify it. Otherwise, we'll end up calling ->suspend() twice for power
> down for each device (those that can do w/ interrupts enabled and those
> that need interrupts disabled), which also requires every single driver to
> check whether or not interrupts are enabled, instead of just those that
> need it.
No, you should have simply let it alone and pass "level" parameter
telling driver if interrupts were disabled or not. No need to
constantly change API while trying to stabilise the code.
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
Hi!
> > static int __init resume_setup(char *str)
> > {
> > - strncpy( resume_file, str, 255 );
> > + if (strlen(str))
> > + strncpy(resume_file, str, 255);
> > return 1;
> > }
> >
> > Why are you obfuscating the code?
>
> Eh? First, why would you want to copy a NULL string?
How is strlen(NULL) better than strncpy(_, NULL, _)?
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
> This is stable series, and /proc/acpi/sleep was fine for at least
> entering S3 and swsusp. Anyway, if you killed sleep, you should kill
> alarm as well. Its only usefull for sleeping, and IIRC it never worked
> properly, anyway.
I will fix alarm. I will also update Nigel's suspend scripts (or release
others) that abstract the mechanism for entering sleep away from the user.
> If you want to help, take a look at drivers/pci/power.c. That file
> should not need to exist, but if I kill it bad stuff happens after
> resume. Killing pm_register() and friends would be nice.
I'll get there. Give me a couple of weeks..
> what about enum action { }; extern int (*pm_power_down)(enum action
> state)?
That's doable.
> > Secondly, you can actually remove the second command line parameter
> > ("noresume") by simply specifying a NULL partition to this parameter. It
> > requires about a 5-line change, and makes things simpler.
>
> You'd better not. You are expected to have one "resume=/foo/bar"
> specified as append in lilo. You want to able to say noresume and do
> one boot without resuming. Turning resume with
> "resume=/dev/nonexistent" would be playing roulete with command line
> argument order.
AFAIK, you could have
resume=/dev/hda3 always appended to your command line. Should you suspend
and not want to resume, you should be able to manually add
"resume=" on the command line after the above, and have the setup function
called again, which would reset it to NULL, thereby keeping the same
semantics as "noresume", but with less namespace pollution. Anyway, I'm
not going to do this now. I'll send you a patch if I do.
> > -EAGAIN allows the drivers/devices that really need special care to
> > specify it. Otherwise, we'll end up calling ->suspend() twice for power
> > down for each device (those that can do w/ interrupts enabled and those
> > that need interrupts disabled), which also requires every single driver to
> > check whether or not interrupts are enabled, instead of just those that
> > need it.
>
> No, you should have simply let it alone and pass "level" parameter
> telling driver if interrupts were disabled or not. No need to
> constantly change API while trying to stabilise the code.
...and modify each driver to check for it?
The decision to kill the level parameter came from extensive discussions
with Benh, who convinced me that we only need to call ->suspend() once for
any device; though we still need to somehow provide for those that need to
power down with interrupts disabled. I suggested -EAGAIN, since it allows
us to special case those that need it, with the minimum amount of impact.
Ben agreed with me.
Pat
> > How is strlen(NULL) better than strncpy(_, NULL, _)?
Actually, the string should not NULL, but merely empty. Neither strlen()
nor strncpy() will check for a NULL string.
My other argument still stands.
Pat
> > > static int __init resume_setup(char *str)
> > > {
> > > - strncpy( resume_file, str, 255 );
> > > + if (strlen(str))
> > > + strncpy(resume_file, str, 255);
> > > return 1;
> > > }
> > >
> > > Why are you obfuscating the code?
> >
> > Eh? First, why would you want to copy a NULL string?
>
> How is strlen(NULL) better than strncpy(_, NULL, _)?
Well, it will tell you whether or not you copied anything. Which, like I
mentioned before, can be used to determine whether or not the user really
wants to resume or not, in lieu of a superfluous command line parameter
("noresume").
Pat
Hi!
> > You changed return type of do_magic() to int, but did not bother to
> > update assembly code, as far as I can see. Did you test those changes?
>
> I noticed that, and it's already fixed.
Be sure to fix x86-64, too (mov $0, %rax should do the trick), and try
to Cc: me this time.
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
On Sat, 2003-08-23 at 10:05, Patrick Mochel wrote:
> > This is stable series, and /proc/acpi/sleep was fine for at least
> > entering S3 and swsusp. Anyway, if you killed sleep, you should kill
> > alarm as well. Its only usefull for sleeping, and IIRC it never worked
> > properly, anyway.
>
> I will fix alarm. I will also update Nigel's suspend scripts (or release
> others) that abstract the mechanism for entering sleep away from the user.
Thanks for the credit, but they're not mine. I assume there's credit
already in the suspend.sh; I've never looked at it in much detail.
> > If you want to help, take a look at drivers/pci/power.c. That file
> > should not need to exist, but if I kill it bad stuff happens after
> > resume. Killing pm_register() and friends would be nice.
On this topic, can you guys please sort out for once and for all where
the code is going to end up. I'm trying to make the diff between 2.4 and
2.6 minimal. :>
> > > Secondly, you can actually remove the second command line parameter
> > > ("noresume") by simply specifying a NULL partition to this parameter. It
> > > requires about a 5-line change, and makes things simpler.
> >
> > You'd better not. You are expected to have one "resume=/foo/bar"
> > specified as append in lilo. You want to able to say noresume and do
> > one boot without resuming. Turning resume with
> > "resume=/dev/nonexistent" would be playing roulete with command line
> > argument order.
>
> AFAIK, you could have
>
> resume=/dev/hda3 always appended to your command line. Should you suspend
> and not want to resume, you should be able to manually add
>
> "resume=" on the command line after the above, and have the setup function
> called again, which would reset it to NULL, thereby keeping the same
> semantics as "noresume", but with less namespace pollution. Anyway, I'm
> not going to do this now. I'll send you a patch if I do.
It would also break being able to fix the header - unless you like
having to mkswap after noresume :> (Unless you want us to remember the
value given by the first resume= parameter, but doesn't that negate the
logic to your argument?).
Regards,
Nigel
--
Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand
You see, at just the right time, when we were still powerless,
Christ died for the ungodly.
-- Romans 5:6, NIV.
On Fri, Aug 22, 2003 at 02:25:46PM -0700, Patrick Mochel wrote:
> > As far as I can see, I'm still maintainer of software suspend. That
> > did not stop you from crying "split those patches" when I tried to
> > submit changes to my code, and you were pretty pissed off when I tried
> > to push trivial one liners without contacting maintainers.
>
> Ok, I'm sorry. I should not have broken anything in your code, and
> actually significantly improved it.
There is however a pretty large problem which now needs to be fixed,
and it goes by the name of "power management".
There is a hell of a lot of work which now needs to be done to re-fix
everything which was working. For example, there is no sign of any
power management for platform devices currently. Could you give some
clues as to what you'd like to see there?
There's also a fair number of drivers to update to this new power
management model - eg, ARM device drivers, PCMCIA socket drivers to
name just two.
We also need to fix the device model probing so we can have a generic
PCI bridge driver but override it if we have a more specific driver.
I did ask to see the code and make preparations for this type of
change... Since the driver model and power management is fundamental
to many subsystems, I think it would be a good thing to have at least
a few days for review of the changes on lkml.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
> > If you want to help, take a look at drivers/pci/power.c. That file
> > should not need to exist, but if I kill it bad stuff happens after
> > resume. Killing pm_register() and friends would be nice.
>
> I'll get there. Give me a couple of weeks..
Actually, on ppc, I have no problem removing that old crap. I suppose
part of the problem Pavel is having is the new code never calling PCI
save_state().
The probleme here is related to the new semantics. save_state() is
indeed meaningless now, but a bunch of drivers implemented sleep in
there because this was really what was called on suspend()... So
unless we want to remove save_state from struct pci_driver and fix all
PCI drivers that implement it, we shall call both save_state() and
suspend() from pci-driver.c suspend routine. (Patch sent separately)
> The decision to kill the level parameter came from extensive discussions
> with Benh, who convinced me that we only need to call ->suspend() once for
> any device; though we still need to somehow provide for those that need to
> power down with interrupts disabled. I suggested -EAGAIN, since it allows
> us to special case those that need it, with the minimum amount of impact.
> Ben agreed with me.
Well... I think I told you I don't like much the check on the interrupt
and tended to prefer either a separate power_down_irq callback or a
parameter, but that would mean changing prototype for drivers... I
agreed we can live with your current scheme though.
Ben.
On Sat, Aug 23, 2003 at 11:47:38AM +0100, Russell King wrote:
> I did ask to see the code and make preparations for this type of
> change... Since the driver model and power management is fundamental
> to many subsystems, I think it would be a good thing to have at least
> a few days for review of the changes on lkml.
Pat, can we have a set of configuration options to disable a lot of the
generic (!) code in kernel/power/*.c which isn't useful on embedded
platforms?
I'm looking at stuff like the swsusp infrastructure for clearing out
memory, suspending to disk, the PM state management, etc? This type
of stuff isn't at all useful on embedded platforms.
On these platforms, you'd typically suspend all the devices except for
a select few which would cause wake-up, put the RAM into self-refresh
mode, and send the CPU to sleep and put the whole device into a low
power "off" state.
This requires none of the swsusp-like infrastructure, so I don't need
most of the "generic" power code in kernel/power which I get for free
just by enabling CONFIG_PM.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
Ok, another issue.
UCB1x00 devices. I now have two implementations of drivers for these
devices, one trying to use the device/driver model and failing badly
with only half the functionality supported. The other one is based
around one class, one class device, and several class interfaces.
What we have for the class model is:
MCP driver (on platform bus) ----> UCB1x00 device
UCB1x00 core driver ---> UCB1x00 class device contained within core driver
data structure
Then we have the UCB1x00 touchscreen, audio, random platform specific
GPIO drivers (0 to 13), random ADC drivers (0 to 4) which are class
interfaces. The GPIO signals get used as interrupts, key inputs
(so would be an input device driver), another (slow) control bus,
or random device control.
The advantage with the class implementation is that you can register
as many class interfaces as you like against one class device - so
the UCB1x00 driver itself doesn't have to worry about the platform
specific details. Those can be handled by a separate per-platform
"interface".
The class implementation seems to be better, but I'm hitting a snag -
power management. The touchscreen and audio needs suspend/resume
support to work correctly (so DMA can be shut down, and touchscreen
threads woken up to ensure we aren't missing an interrupt.)
Unfortunately, the class model doesn't provide this functionality.
The device model does, but that then gives us a problem - how to handle
all the platform specific implementation details (ie, gpio and adc
drivers) sanely. We can't create 17 struct devices just to let
platform specific drivers bind to what they need - that'd be insane
in terms of the overhead (struct bus_type + n * struct device
+ m * struct device_driver) but it does get us power management.
And one big UCB1x00 driver would be stuffed full of ifdefs, with
sound, input, and other random subsystem drivers all rolled into one.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
Hi!
> > > Secondly, you can actually remove the second command line parameter
> > > ("noresume") by simply specifying a NULL partition to this parameter. It
> > > requires about a 5-line change, and makes things simpler.
> >
> > You'd better not. You are expected to have one "resume=/foo/bar"
> > specified as append in lilo. You want to able to say noresume and do
> > one boot without resuming. Turning resume with
> > "resume=/dev/nonexistent" would be playing roulete with command line
> > argument order.
>
> AFAIK, you could have
>
> resume=/dev/hda3 always appended to your command line. Should you suspend
> and not want to resume, you should be able to manually add
>
> "resume=" on the command line after the above, and have the setup function
> called again, which would reset it to NULL, thereby keeping the same
I'm not sure if it is easy to guarantee that you are adding *after*
parameters automatically appended with LILO.
> > > -EAGAIN allows the drivers/devices that really need special care to
> > > specify it. Otherwise, we'll end up calling ->suspend() twice for power
> > > down for each device (those that can do w/ interrupts enabled and those
> > > that need interrupts disabled), which also requires every single driver to
> > > check whether or not interrupts are enabled, instead of just those that
> > > need it.
> >
> > No, you should have simply let it alone and pass "level" parameter
> > telling driver if interrupts were disabled or not. No need to
> > constantly change API while trying to stabilise the code.
>
> ...and modify each driver to check for it?
Well, those drivers should have the checks already, otherwise they are
buggy, but I guess I see your point now.
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
> There is a hell of a lot of work which now needs to be done to re-fix
> everything which was working. For example, there is no sign of any
> power management for platform devices currently. Could you give some
> clues as to what you'd like to see there?
How about following the system device scheme: 1 call, with interrupts
disabled?
> There's also a fair number of drivers to update to this new power
> management model - eg, ARM device drivers, PCMCIA socket drivers to
> name just two.
That's fine. I will fix PCMCIA, as I have devices to test with. I have no
ARM devices (nor do I want any). I can take a stab, but won't guarantee
anything. Could you tell me, though, when/if these devices did work with
what power management scheme? APM?
> We also need to fix the device model probing so we can have a generic
> PCI bridge driver but override it if we have a more specific driver.
We talked about that, and it's going to require some changes to the core,
albeit small. We're not prepared to do that right now, though we'll
reconsider depending on necessity and impact of the patch..
Pat
On Mon, Aug 25, 2003 at 08:47:16AM -0700, Patrick Mochel wrote:
> > There is a hell of a lot of work which now needs to be done to re-fix
> > everything which was working. For example, there is no sign of any
> > power management for platform devices currently. Could you give some
> > clues as to what you'd like to see there?
>
> How about following the system device scheme: 1 call, with interrupts
> disabled?
I don't think that's going to work well when you have more conventional
devices below a platform device which need to be power managed (eg, a
USB host.)
I think we need to expand the platform device support to include the
notion of platform drivers. For example:
struct platform_driver {
int (*probe)(struct platform_device *);
int (*remove)(struct platform_device *);
int (*suspend)(struct platform_device *, u32);
int (*resume)(struct platform_device *);
struct device_driver drv;
};
(Aside: I like the movement of the suspend/resume methods to the bus_type,
and I'd like to see the probe/remove methods also move there. For the
vast majority of cases, the probe/remove methods in struct device_driver
end up pointing at the same functions for any particular bus.)
> > There's also a fair number of drivers to update to this new power
> > management model - eg, ARM device drivers, PCMCIA socket drivers to
> > name just two.
>
> That's fine. I will fix PCMCIA, as I have devices to test with. I have no
> ARM devices (nor do I want any). I can take a stab, but won't guarantee
> anything. Could you tell me, though, when/if these devices did work with
> what power management scheme? APM?
I know it works on ARM, but, since APM is completely fscked in 2.6
kernels and no one seems to be interested in the issue, its something
I can't test on x86.
> > We also need to fix the device model probing so we can have a generic
> > PCI bridge driver but override it if we have a more specific driver.
>
> We talked about that, and it's going to require some changes to the core,
> albeit small. We're not prepared to do that right now, though we'll
> reconsider depending on necessity and impact of the patch..
Bear in mind that Red Hat kernels contain a generic PCI bridge driver
in order to save its state across suspend/resume, and that the PPC
people also seem to need it. Also bear in mind that the Mobility Docks
have some non-standard configuration controls in their PCI-PCI bridge
which needs a vendor specific PCI-PCI bridge driver.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Mon, Aug 25, 2003 at 08:47:16AM -0700, Patrick Mochel wrote:
> That's fine. I will fix PCMCIA, as I have devices to test with. I have no
> ARM devices (nor do I want any). I can take a stab, but won't guarantee
> anything. Could you tell me, though, when/if these devices did work with
> what power management scheme? APM?
I omitted to say that I've fixed up most of the non-platform based
PCMCIA socket drivers. However, this set of patches is waiting on
a response from gregkh/jgarzik on a fix to the PCI save/restore state
handling.
In any case, we need to get the platform device situation resolved by
public discussion before making any code changes.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Mon, Aug 25, 2003 at 09:57:20AM -0700, Matt Porter wrote:
> Alternatively, you could leave the platform model as is (it's only
> for really dumb devices).
Thing is that we use the platform model for off chip devices as well.
On ARM, its gets used for any device which the platform code knows
where it is located. ie, a platform device.
> On PPC, we have an OCP (on chip peripheral)
> model that is mostly integrated into the device model now. OCP is
> just another bus/driver type and so PM works in the normal fashion.
Ah, but OCP can't be used to describe a platform dependent SMC91x
network interface that some random designer decided to drop into
their design - it isn't part of the SoC.
> There's a driver API around it as well so we can cleanly share drivers
> across various SoC implementations with different base address,
> IRQ mappings, etc. It might be more useful to extende this across
> the architectures that need it.
Note that we've already done some public work on providing flexible
platform device support to satisfy the needs of platform people -
by adding the variable number of resources to the platform device.
Also note that most of the x86 ISA PCMCIA devices _are_ platform
devices today. As of this new power management model, they're
broken due to the fact that they no longer receive power management
events.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Mon, Aug 25, 2003 at 05:27:37PM +0100, Russell King wrote:
> On Mon, Aug 25, 2003 at 08:47:16AM -0700, Patrick Mochel wrote:
> > > There is a hell of a lot of work which now needs to be done to re-fix
> > > everything which was working. For example, there is no sign of any
> > > power management for platform devices currently. Could you give some
> > > clues as to what you'd like to see there?
> >
> > How about following the system device scheme: 1 call, with interrupts
> > disabled?
>
> I don't think that's going to work well when you have more conventional
> devices below a platform device which need to be power managed (eg, a
> USB host.)
>
> I think we need to expand the platform device support to include the
> notion of platform drivers. For example:
>
> struct platform_driver {
> int (*probe)(struct platform_device *);
> int (*remove)(struct platform_device *);
> int (*suspend)(struct platform_device *, u32);
> int (*resume)(struct platform_device *);
> struct device_driver drv;
> };
Alternatively, you could leave the platform model as is (it's only
for really dumb devices). On PPC, we have an OCP (on chip peripheral)
model that is mostly integrated into the device model now. OCP is
just another bus/driver type and so PM works in the normal fashion.
There's a driver API around it as well so we can cleanly share drivers
across various SoC implementations with different base address,
IRQ mappings, etc. It might be more useful to extende this across
the architectures that need it.
-Matt
On Mon, Aug 25, 2003 at 06:14:15PM +0100, Russell King wrote:
> On Mon, Aug 25, 2003 at 09:57:20AM -0700, Matt Porter wrote:
> > Alternatively, you could leave the platform model as is (it's only
> > for really dumb devices).
>
> Thing is that we use the platform model for off chip devices as well.
> On ARM, its gets used for any device which the platform code knows
> where it is located. ie, a platform device.
>
> > On PPC, we have an OCP (on chip peripheral)
> > model that is mostly integrated into the device model now. OCP is
> > just another bus/driver type and so PM works in the normal fashion.
>
> Ah, but OCP can't be used to describe a platform dependent SMC91x
> network interface that some random designer decided to drop into
> their design - it isn't part of the SoC.
There's nothing inherent in OCP (except the name) that prevents this.
In fact, that's one of the directions we wanted to go with it because
of PPC4xx external bus controller devices that are board specific.
> > There's a driver API around it as well so we can cleanly share drivers
> > across various SoC implementations with different base address,
> > IRQ mappings, etc. It might be more useful to extende this across
> > the architectures that need it.
>
> Note that we've already done some public work on providing flexible
> platform device support to satisfy the needs of platform people -
> by adding the variable number of resources to the platform device.
Yes, I recall asking for more interrupt resources when I thought it
might be useful. However, the PPC4xx stuff is requiring a move
to a finer grained specification of internal bus types to properly
implement PM. It's necessary to know if a device is a child of
the PLB, OPB, EBC, etc. to know when to power down various internal
bus drivers as well. I don't see that happening with platform
devices even with the addition of PM ops. Perhaps this isn't
applicable to more than PPC, though.
> Also note that most of the x86 ISA PCMCIA devices _are_ platform
> devices today. As of this new power management model, they're
> broken due to the fact that they no longer receive power management
> events.
I see.
-Matt
> > The decision to kill the level parameter came from extensive discussions
> > with Benh, who convinced me that we only need to call ->suspend() once for
> > any device; though we still need to somehow provide for those that need to
> > power down with interrupts disabled. I suggested -EAGAIN, since it allows
> > us to special case those that need it, with the minimum amount of impact.
> > Ben agreed with me.
>
> Well... I think I told you I don't like much the check on the interrupt
> and tended to prefer either a separate power_down_irq callback or a
> parameter, but that would mean changing prototype for drivers... I
> agreed we can live with your current scheme though.
How about a flag in the power struct, which would place the device on a
completely separate list from the beginning. The drivers should know
whether a device needs special handling a priori, so we don't even need to
touch it during the first iteration of the lists.
This would eliminate the need to check in the drivers, have no impact on
the majority of drivers, and allow us to easily determine whether or not
the device supports runtime power management.
Pat
On Mon, 2003-08-25 at 21:05, Patrick Mochel wrote:
> How about a flag in the power struct, which would place the device on a
> completely separate list from the beginning. The drivers should know
> whether a device needs special handling a priori, so we don't even need to
> touch it during the first iteration of the lists.
>
> This would eliminate the need to check in the drivers, have no impact on
> the majority of drivers, and allow us to easily determine whether or not
> the device supports runtime power management.
Not sure if we don't actually want both iterations :) It's difficult to
knwo for sure as I lack such a device actually... But I can imagine your
need to run with IRQs enabled to properly "flush" pending requests, then
have IRQs disabled to do the HW shutdown.
For example, if you are a block device, then you need IRQ enabled so you
can send a SUSPEND request down your request queue like IDE does (that's
the best way to get proper synchronisation) and wait on it to complete.
But if your HW is then broken enough that when you actually want to turn
it off (after the queue has been properly suspended) it will leave a
dangling interrupt, then you need to do that last part with IRQs disabled.
Of course, the above only comes out of my imagination, I don't have such
a device, I suspect Alan may know more about the kind of devices we may
expect with an IRQ problem on suspend..
Ben.
> Pat, can we have a set of configuration options to disable a lot of the
> generic (!) code in kernel/power/*.c which isn't useful on embedded
> platforms?
>
> I'm looking at stuff like the swsusp infrastructure for clearing out
> memory, suspending to disk, the PM state management, etc? This type
> of stuff isn't at all useful on embedded platforms.
Sure, no problem.
Pat
Sorry it's taken so long to reply, this has needed a chance to sink in
(and for other things to be resolved).
> I think we need to expand the platform device support to include the
> notion of platform drivers. For example:
>
> struct platform_driver {
> int (*probe)(struct platform_device *);
> int (*remove)(struct platform_device *);
> int (*suspend)(struct platform_device *, u32);
> int (*resume)(struct platform_device *);
> struct device_driver drv;
> };
I see two ways of supporting platform devices that will have a variable
number of logical interfaces -- depending on the platform -- per physical
device, and support power management on them.
The first: put power management methods in struct class.
As we suspend each physical device, we would loop over each class device
that is associated with the physical device, and call its class's
->suspend() method for it. The class would be responsible for stopping the
device and taking that view of the device offline. The class-specific
portion of the driver may optionally have suspend()/resume() methods that
the class could call to save those bits of device state.
This would leave the bus-specific power management calls to only power
down the device and save config-space-like register state of the device.
(Especially if there were per-driver class-specific suspend/resume
methods, as they would be responsible for saving/restoring the interesting
parts of the device.)
The second: Allow mulitple drivers to bind to platform devices.
We would use a similar structure as you have above, with a struct
list_head in it to allow us to chain them together. When we suspended a
platform device, the platform bus would iterate through each driver that's
bound to a device and call ->suspend().
Simple enough, though we'd have to create some sort of notion of a
'platform_class' for each device type, as there could theoretically be
multiple instances of a device type, and a chain of drivers could only
bind to one without dynamically allocating each instance of the driver. It
would be similar to the way system devices are handled, but without
breaking away from the unified device hierarchy.
Either way is relatively easy to implement, and I'm ambivalent about which
to choose this morning. The former may have much added benefit by reducing
the amount of work that needs to be done in each driver of each type,
while the latter focuses on just resolving your issue..
Pat
Hi!
> I think we need to expand the platform device support to include the
> notion of platform drivers. For example:
>
> struct platform_driver {
> int (*probe)(struct platform_device *);
> int (*remove)(struct platform_device *);
> int (*suspend)(struct platform_device *, u32);
> int (*resume)(struct platform_device *);
> struct device_driver drv;
> };
>
> (Aside: I like the movement of the suspend/resume methods to the bus_type,
> and I'd like to see the probe/remove methods also move there. For the
> vast majority of cases, the probe/remove methods in struct device_driver
> end up pointing at the same functions for any particular bus.)
But what about those few devices that need special handling? Like flush write cache on
IDE disk but not on cdrom?
--
Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...
On Mon, Sep 01 2003, Pavel Machek wrote:
> Hi!
>
> > I think we need to expand the platform device support to include the
> > notion of platform drivers. For example:
> >
> > struct platform_driver {
> > int (*probe)(struct platform_device *);
> > int (*remove)(struct platform_device *);
> > int (*suspend)(struct platform_device *, u32);
> > int (*resume)(struct platform_device *);
> > struct device_driver drv;
> > };
> >
> > (Aside: I like the movement of the suspend/resume methods to the bus_type,
> > and I'd like to see the probe/remove methods also move there. For the
> > vast majority of cases, the probe/remove methods in struct device_driver
> > end up pointing at the same functions for any particular bus.)
>
> But what about those few devices that need special handling? Like
> flush write cache on IDE disk but not on cdrom?
ide-cd should have a flush write cache as well, for mtr, dvd-ram, cd-rw
with packet writing, etc.
--
Jens Axboe
> ide-cd should have a flush write cache as well, for mtr, dvd-ram, cd-rw
> with packet writing, etc.
This is currently not done by the ide-cd suspend state machine, I did
the infrastructure and ide-disk implementation, but I'm leaving things
like ide-cd to you :)
Patrick: As we discussed on IRC, the actual PM state constants you
defined don't match the old "S" levels, thus this code in ide-disk
suspend notifier:
if (rq->pm->pm_state == 4)
to avoid stopping the platter on suspend-to-disk will not work.
Should I fix the above to use a PM_* constant or will you fix the
constants ?
Ben.
On Tue, Sep 09 2003, Benjamin Herrenschmidt wrote:
>
> > ide-cd should have a flush write cache as well, for mtr, dvd-ram, cd-rw
> > with packet writing, etc.
>
> This is currently not done by the ide-cd suspend state machine, I did
> the infrastructure and ide-disk implementation, but I'm leaving things
> like ide-cd to you :)
That is fine, I'll take care of it :). Just wanted to point out that it
is indeed _not_ a given that ide-cd doesn't need sync cache support.
--
Jens Axboe
> Patrick: As we discussed on IRC, the actual PM state constants you
> defined don't match the old "S" levels, thus this code in ide-disk
> suspend notifier:
>
> if (rq->pm->pm_state == 4)
>
> to avoid stopping the platter on suspend-to-disk will not work.
>
> Should I fix the above to use a PM_* constant or will you fix the
> constants ?
It should definitely be checking for a defined constant.
However, I don't think we necessarily want to pass the system state we're
entering to the drivers. It's per-driver policy to decide what device
state to enter for each system state, which are orthogonal to the runtime
device states that it could enter.
For most devices, it's going to be 'off' (aka 3) for either suspend-to-ram
or -disk. But, some devices will have varying or non-standard behavior.
Like IDE drives or those few instances of ATI video devices on some power
books that can only go into D2 instead of D3 on STR.
To remedy this, I say we commit to the fact that it is the device state
that's being passed down to the drivers. But, we get that state for each
device from a field in the devices' power structures:
struct dev_pm_info {
...
u32 str_state;
u32 std_state;
};
The bus will be able to set these on device registration, or the driver on
binding. We can also expose them through sysfs to let userspace policy
modify them. Otherwise they will default to the standard (3 aka 'off').
What do you think?
Pat
Hi!
> > Patrick: As we discussed on IRC, the actual PM state constants you
> > defined don't match the old "S" levels, thus this code in ide-disk
> > suspend notifier:
> >
> > if (rq->pm->pm_state == 4)
> >
> > to avoid stopping the platter on suspend-to-disk will not work.
> >
> > Should I fix the above to use a PM_* constant or will you fix the
> > constants ?
>
> It should definitely be checking for a defined constant.
>
> However, I don't think we necessarily want to pass the system state we're
> entering to the drivers. It's per-driver policy to decide what device
> state to enter for each system state, which are orthogonal to the runtime
> device states that it could enter.
>
> For most devices, it's going to be 'off' (aka 3) for either suspend-to-ram
> or -disk. But, some devices will have varying or non-standard behavior.
> Like IDE drives or those few instances of ATI video devices on some power
> books that can only go into D2 instead of D3 on STR.
>
> To remedy this, I say we commit to the fact that it is the device state
> that's being passed down to the drivers. But, we get that state for each
> device from a field in the devices' power structures:
>
>
> struct dev_pm_info {
> ...
> u32 str_state;
> u32 std_state;
> };
>
> The bus will be able to set these on device registration, or the driver on
> binding. We can also expose them through sysfs to let userspace policy
> modify them. Otherwise they will default to the standard (3 aka 'off').
>
> What do you think?
I do think this is a bit complicated. I believe passing level, along
with type of the suspend (aka swsusp vs. S4bios) should be enough.
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
> I do think this is a bit complicated. I believe passing level, along
> with type of the suspend (aka swsusp vs. S4bios) should be enough.
What about suspend-to-ram, APM, and runtime states?
That actually makes it quite a bit more complicated, globally. By forcing
the policy down to the drivers, you force each one to interpret the value
themselves and make the decision. By doing it centrally, the only thing
the low-level drivers have to worry about is going into the state.
Pat
Hi!
> > ide-cd should have a flush write cache as well, for mtr, dvd-ram, cd-rw
> > with packet writing, etc.
>
> This is currently not done by the ide-cd suspend state machine, I did
> the infrastructure and ide-disk implementation, but I'm leaving things
> like ide-cd to you :)
>
> Patrick: As we discussed on IRC, the actual PM state constants you
> defined don't match the old "S" levels, thus this code in ide-disk
> suspend notifier:
>
> if (rq->pm->pm_state == 4)
>
> to avoid stopping the platter on suspend-to-disk will not work.
>
> Should I fix the above to use a PM_* constant or will you fix the
> constants ?
Well, saying "4" is not enough, as you want to stop platter ... no in
this case it is enough. s4bios and swsusp behave the same here. [I
still think there may be cases where s4bios needs one behaviour and
swsusp needs another one.]
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
Hi!
> > I do think this is a bit complicated. I believe passing level, along
> > with type of the suspend (aka swsusp vs. S4bios) should be enough.
>
> What about suspend-to-ram, APM, and runtime states?
I'd not worry about runtime states for now. [If user wants to sleep
one device, we probably can allow that, but I do not think it is
reasonable to do much more for 2.6.X]. That leaves us with:
APM suspend-to-ram
APM suspend-to-disk
ACPI standby (S1)
ACPI suspend-to-ram (S3)
ACPI suspend-to-disk (S4bios)
swsusp
Do we want to support ACPI S2? I don't think so. That list is not
*that* bad.
> That actually makes it quite a bit more complicated, globally. By forcing
> the policy down to the drivers, you force each one to interpret the value
> themselves and make the decision. By doing it centrally, the only thing
> the low-level drivers have to worry about is going into the state.
Yes, but you'll have to have central database saying "nvidia VGA needs
to be in D1 during S3". I don't think that's good idea. Better put it
into the driver... Hopefully not many drivers will need such hacks.
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
On Mer, 2003-09-10 at 00:07, Patrick Mochel wrote:
> What about suspend-to-ram, APM, and runtime states?
>
> That actually makes it quite a bit more complicated, globally. By forcing
> the policy down to the drivers, you force each one to interpret the value
> themselves and make the decision. By doing it centrally, the only thing
> the low-level drivers have to worry about is going into the state.
APM and ACPI suspend/resume (especially resume) are different. Very
different with some hardware in fact. For IDE to be done perfectly you
want to know if its ACPI S4 or APM suspend. The driver needs to be able
to get the actual aim but most I agree wont care which.
> I'd not worry about runtime states for now. [If user wants to sleep
> one device, we probably can allow that, but I do not think it is
> reasonable to do much more for 2.6.X]. That leaves us with:
We've always supported runtime states. There is just no drivers which
support it.
> APM suspend-to-ram
> APM suspend-to-disk
> ACPI standby (S1)
> ACPI suspend-to-ram (S3)
> ACPI suspend-to-disk (S4bios)
> swsusp
>
> Do we want to support ACPI S2? I don't think so. That list is not
> *that* bad.
ACPI S2 is irrelevant. Nonetheless, you're suggesting that we add manual
checks at runtime for each device to determine what state to go into,
depending on whether the system is entering suspend-to-disk or suspend-to-
ram.
That's a bad idea because:
- It doesn't need to be done at runtime, only initialization. Though it's
not a permformant path, it's still more efficient to do it once only.
- It forces policy into the drivers, instead of having them specify a
changeable default.
- It can only be changed by editing and recompiling the driver, unless
they manually export the variable.
> Yes, but you'll have to have central database saying "nvidia VGA needs
> to be in D1 during S3". I don't think that's good idea. Better put it
> into the driver... Hopefully not many drivers will need such hacks.
No, read the first email on this subject. You can have the bus do it for
all devices of a certain type (e.g. IDE), or you can have the drivers do
it when they're bound to specific devices.
Pat
Hi!
> > APM suspend-to-ram
> > APM suspend-to-disk
> > ACPI standby (S1)
> > ACPI suspend-to-ram (S3)
> > ACPI suspend-to-disk (S4bios)
> > swsusp
> >
> > Do we want to support ACPI S2? I don't think so. That list is not
> > *that* bad.
>
> ACPI S2 is irrelevant. Nonetheless, you're suggesting that we add manual
> checks at runtime for each device to determine what state to go into,
> depending on whether the system is entering suspend-to-disk or suspend-to-
> ram.
>
> That's a bad idea because:
>
> - It doesn't need to be done at runtime, only initialization. Though it's
> not a permformant path, it's still more efficient to do it once only.
>
> - It forces policy into the drivers, instead of having them specify a
> changeable default.
Okay, so you suggest "driver has table of default things to do on
suspend-to-X, changeable by user", while I say "driver has table of
default things to do on suspend-to-X". I do not thing changeability by
user is so important here, but I'm not going to argue too much about
that.
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
On Wed, 10 Sep 2003 01:07:55 +0200 Pavel Machek <[email protected]> wrote:
>
> I'd not worry about runtime states for now. [If user wants to sleep
> one device, we probably can allow that, but I do not think it is
> reasonable to do much more for 2.6.X]. That leaves us with:
>
> APM suspend-to-ram
> APM suspend-to-disk
We do not know which of these is going to happen, it is entirely up to
the BIOS writers and the BIOS configuration ... i.e. on my laptop, I
have two suspend buttons - one for suspend to ram, one for suspend to disk,
but under APM the kernel merely gets a SUSPEND event no matter which
button I press.
However, there is APM standby
On another note, APM does allow for individual device power management
(i.e. you can tell the BIOS "suspend the first disk" or "power off
all displays"). I have been wondering if we want to disable the BIOS's
device power management now that we are begining to do it ourselves.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
On Mer, 2003-09-10 at 07:12, Stephen Rothwell wrote:
> On another note, APM does allow for individual device power management
> (i.e. you can tell the BIOS "suspend the first disk" or "power off
> all displays"). I have been wondering if we want to disable the BIOS's
> device power management now that we are begining to do it ourselves.
APM knows more about the system than we do, certainly for laptops and it
can do stuff we can't do without ACPI. Most BIOSes also don't bother
supporting most of the "suspend first foo", or for that matter always
agree with us what is first.