Re: userspace notification from module

Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 2 stycznia 2010 13:36 	Daniel Borkmann <llug.dan@googlemail.com> napisał(a):

> Hi Bartłomiej,
> 
> Bartłomiej Zimoń wrote:
> > Use inotify from userspace could be interesting for such data,
> > but looks like sysfs/procfs doesn't send signal about data changed in file or maybe i'm wrong?
> > 
> > If sysfs/procfs is wrong for that so must create device file, but here secound problem 
> > how to notify that file/buffer has new data?
> 
> Actually, the file content of a procfs file is volatile and generated on
> the fly, just have a look at some device drivers or at the API.
> 

As I fought. Reading now sources of some char drivers.

> Did you have a look at the netlink protocol for communication from
> kernel to userspace?
> 

Ok i will :)

I will explain more what i'm going to do.
The idea is simple. Kernel before suspend/resume sends notification to 
registered kernel objects. So i have written small module to register
there and want now pass these data to /dev/file. It is 1byte so not a big deal.

I'm searching now for notification about new byte in buffer, as it is almost 
clear for async: http://www.xml.com/ldd/chapter/book/ch05.html#t4
but for sync still haven't find.

It could be interesting for NetworkManager to hook on such event.

Looks like my post arrives 2nd time in lkml.

Thx for reply.

Best Regards.
Bartłomiej Zimoń
PLD Linux, Kadu Team

Re: userspace notification from module

[Daniel Borkmann]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

Hi Bartłomiej,

Bartłomiej Zimoń wrote:
> Dnia 2 stycznia 2010 13:36 	Daniel Borkmann <llug.dan@googlemail.com> napisał(a):
> I will explain more what i'm going to do.
> The idea is simple. Kernel before suspend/resume sends notification to 
> registered kernel objects. So i have written small module to register
> there and want now pass these data to /dev/file. It is 1byte so not a big deal.
> 
> I'm searching now for notification about new byte in buffer, as it is almost 
> clear for async: http://www.xml.com/ldd/chapter/book/ch05.html#t4
> but for sync still haven't find.

This sounds very interesting... a "hack" could be the following:

Register your pids to the kernel module, e.g. via ioctl, and if the
(intra-kernel) notification will be delivered to your module you just
send a signal to your registered processes via kill_proc_info(). Guess
this is at least better than polling a file or sth similar.

I guess netlink could be the "cleaner" solution, but with more overhead?!

Cheers,
Daniel


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

[suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 2 stycznia 2010 15:04 	Daniel Borkmann <danborkmann@googlemail.com> napisał(a):
> Hi Bartłomiej,
> 
> Bartłomiej Zimoń wrote:
> > Dnia 2 stycznia 2010 13:36 	Daniel Borkmann <llug.dan@googlemail.com> napisał(a):
> > I will explain more what i'm going to do.
> > The idea is simple. Kernel before suspend/resume sends notification to 
> > registered kernel objects. So i have written small module to register
> > there and want now pass these data to /dev/file. It is 1byte so not a big deal.
> > 
> > I'm searching now for notification about new byte in buffer, as it is almost 
> > clear for async: http://www.xml.com/ldd/chapter/book/ch05.html#t4
> > but for sync still haven't find.
> 
> This sounds very interesting... a "hack" could be the following:
> 
> Register your pids to the kernel module, e.g. via ioctl, and if the
> (intra-kernel) notification will be delivered to your module you just
> send a signal to your registered processes via kill_proc_info(). Guess
> this is at least better than polling a file or sth similar.
> 
> I guess netlink could be the "cleaner" solution, but with more overhead?!
> 

This looks like more simple and could be good, after all i think about sending this 
module here to add to kernel. So if this solution could be ok i will start to code this part.

Question is now what signal to choose to not terminate process and to be compatible 
with rest of Linux kernel?

Best regards
Bartłomiej Zimoń

Re: [suspend/resume] Re: userspace notification from module

[Oliver Neukum]

Am Samstag, 2. Januar 2010 15:27:12 schrieb Bartłomiej Zimoń:
> > Register your pids to the kernel module, e.g. via ioctl, and if the
> > (intra-kernel) notification will be delivered to your module you just
> > send a signal to your registered processes via kill_proc_info(). Guess
> > this is at least better than polling a file or sth similar.
> > 
> > I guess netlink could be the "cleaner" solution, but with more overhead?!
> > 
> 
> This looks like more simple and could be good, after all i think about sending this 
> module here to add to kernel. So if this solution could be ok i will start to code this part.

This has all sorts of implications starting with fork() and going to passing
fds over sockets and so on. It is a very bad idea. Use a standard device
and all infrastructure for getting SIGIO is in place.

	Regards
		Oliver

Re: [suspend/resume] Re: userspace notification from module

[Daniel Borkmann]

Hi Bartłomiej,

2010/1/2 Bartłomiej Zimoń <uzi18@o2.pl>
>
> This looks like more simple and could be good, after all i think about sending this
> module here to add to kernel. So if this solution could be ok i will start to code this part.
>
> Question is now what signal to choose to not terminate process and to be compatible
> with rest of Linux kernel?

Have a look at linux/signal.h. There's a section for non-POSIX
signals. May be you
could add a SIGSUS or sth similar into that list with a free signal
number to stay
compatible with the rest of the numbers.

Cool thing is that you now will have a kind of "observer" principle:
Don't call us, we call you ;)

Cheers,
Daniel Borkmann

Re: [suspend/resume] Re: userspace notification from module

[Andy Walls]

On Sat, 2010-01-02 at 16:15 +0100, Daniel Borkmann wrote:
> Hi Bartłomiej,
> 
> 2010/1/2 Bartłomiej Zimoń <uzi18@o2.pl>
> >
> > This looks like more simple and could be good, after all i think about sending this
> > module here to add to kernel. So if this solution could be ok i will start to code this part.
> >
> > Question is now what signal to choose to not terminate process and to be compatible
> > with rest of Linux kernel?
> 
> Have a look at linux/signal.h. There's a section for non-POSIX
> signals. May be you
> could add a SIGSUS or sth similar into that list with a free signal
> number to stay
> compatible with the rest of the numbers.
> 
> Cool thing is that you now will have a kind of "observer" principle:
> Don't call us, we call you ;)


Why not:

a. write a module that implements a device node that supports poll(),
and

b. have a user space process select() on the fd for read or exception
notification

?

Regards,
Andy

> Cheers,
> Daniel Borkmann

Re: [suspend/resume] Re: userspace notification from module

[Daniel Borkmann]

Hi Andy,

2010/1/2 Andy Walls <awalls@radix.net>:
> Why not:
>
> a. write a module that implements a device node that supports poll(),
> and
>
> b. have a user space process select() on the fd for read or exception
> notification
>
> ?

This is, of course, another possible solution that is more "cleaner"
than the one with the signals.
Then, your userspace program would have another thread polling for the
device node. Question is which timeout would be appropriate to be "CPU
friendly" and to keep notification latency short?

Cheers,
Daniel

Re: [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 2 stycznia 2010 16:56 	Daniel Borkmann <danborkmann@googlemail.com> napisał(a):
> Hi Andy,
> 
> 2010/1/2 Andy Walls <awalls@radix.net>:
> > Why not:
> >
> > a. write a module that implements a device node that supports poll(),
> > and
> >
> > b. have a user space process select() on the fd for read or exception
> > notification
> >
> > ?
> 
> This is, of course, another possible solution that is more "cleaner"
> than the one with the signals.
> Then, your userspace program would have another thread polling for the
> device node. Question is which timeout would be appropriate to be "CPU
> friendly" and to keep notification latency short?
> 

Just need as fast as possible solution and on the other hand acceptable for kernel sources.
Usually programs needs just to disconnect something or set one flag.
Even if program will have no time for this it could be enough just to send this precious info.

Best regards.
Barłomiej Zimoń
PLD Linux, Kadu Team

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 02 January 2010, Bartłomiej Zimoń wrote:
> Dnia 2 stycznia 2010 16:56 	Daniel Borkmann <danborkmann@googlemail.com> napisał(a):
> > Hi Andy,
> > 
> > 2010/1/2 Andy Walls <awalls@radix.net>:
> > > Why not:
> > >
> > > a. write a module that implements a device node that supports poll(),
> > > and
> > >
> > > b. have a user space process select() on the fd for read or exception
> > > notification
> > >
> > > ?
> > 
> > This is, of course, another possible solution that is more "cleaner"
> > than the one with the signals.
> > Then, your userspace program would have another thread polling for the
> > device node. Question is which timeout would be appropriate to be "CPU
> > friendly" and to keep notification latency short?
> > 
> 
> Just need as fast as possible solution and on the other hand acceptable for kernel sources.
> Usually programs needs just to disconnect something or set one flag.
> Even if program will have no time for this it could be enough just to send this precious info.

Perhaps I don't understand correctly what you're trying to achieve, but at the
moment suspend is always started from user space, this way or another, and on
the majority (all?) of the modern distros pm-utils is involved in that.
So, why don't you provide a pm-utils hook for your process (like, for example,
NetworkManager)?

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 02 January 2010, Bartłomiej Zimoń wrote:
> 
> Dnia 2 stycznia 2010 22:01 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> 
> > On Saturday 02 January 2010, Bartłomiej Zimoń wrote:
> > > Dnia 2 stycznia 2010 16:56 	Daniel Borkmann <danborkmann@googlemail.com> napisał(a):
> > > > Hi Andy,
> > > > 
> > > > 2010/1/2 Andy Walls <awalls@radix.net>:
> > > > > Why not:
> > > > >
> > > > > a. write a module that implements a device node that supports poll(),
> > > > > and
> > > > >
> > > > > b. have a user space process select() on the fd for read or exception
> > > > > notification
> > > > >
> > > > > ?
> > > > 
> > > > This is, of course, another possible solution that is more "cleaner"
> > > > than the one with the signals.
> > > > Then, your userspace program would have another thread polling for the
> > > > device node. Question is which timeout would be appropriate to be "CPU
> > > > friendly" and to keep notification latency short?
> > > > 
> > > 
> > > Just need as fast as possible solution and on the other hand acceptable for kernel sources.
> > > Usually programs needs just to disconnect something or set one flag.
> > > Even if program will have no time for this it could be enough just to send this precious info.
> > 
> > Perhaps I don't understand correctly what you're trying to achieve, but at the
> > moment suspend is always started from user space, this way or another, and on
> > the majority (all?) of the modern distros pm-utils is involved in that.
> > So, why don't you provide a pm-utils hook for your process (like, for example,
> > NetworkManager)?
> > 
> 
> Thanks for Your answare.
> Some points of my idea:
> - don't think everyone want to use pm-utils (didn't say it is bad)

That certainly is true, but I think these people won't have a problem setting
up their suspend scripts to trigger the notification anyway. :-)

Point is, suspend hooks are used anyway by almost everyone (due to graphics,
networking, faulty drivers), so I think you could just use this mechanism, be it
pm-utils or something else.

To put it in a different way, you apparently want the kernel to notify the user
space of an event originating from the user space and my question is why not
to set up the user space to generate the notification without relying on the
kernel to do that.

> - this code is standard for all implementation of suspend/hibernate/resume

I guess you mean the existing kernel-side suspend/hibernate code.

Sure, it is, but your module is going to export the kernel's internal interface
outside the kernel, turning it into a real API and that is a _big_ _deal_.  The
reason why it is a big deal is that while we often change the kernel's internal
interfaces, we don't change APIs.  Once created, an API (I mean a real
userland-kernel interface) is very difficult to change, because that most often
leads to regressions which are quite nasty from the user's point of view.

So, basically, you want us to declare that the kernel's internal
suspend/hibernate notification mechanism won't change in the future, which is
not something I'm going to do at this point.  Moreover, it hasn't been designed
as an API and I don't think it's really suitable for this purpose.

At least, that requires some more discussion, so please tell us why you need
the kernel to notify the user space about suspend/hibernation.  IOW, what's the
final purpose of this?  [Added some CCs.]

> - it is small
> - it have less overhead, dont need dbus and all rest services.

That could have been a good argument if those services had not been used
already, but that's not the case.

> - could be even used partialy by pm-utils
> - it is perfect just to notify about event

OK, but why exactly do _you_ need that to happen?

There's one important drawback of making the kernel generate the notification.
Namely, even if your userspace task is notified by the kernel of a PM event,
that doesn't mean it'll have the time to act upon that, because the kernel will
attempt to freeze it right after the notification has been sent.  This means
you'd need a way to make the kernel wait for your user space process to finish
it's job before continuing the suspend/hibernate process.  Otherwise it's not
going to be very useful.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Daniel Borkmann]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

Bartłomiej Zimoń wrote:
> Dnia 3 stycznia 2010 0:29 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
>>> Thanks for Your answare.
>>> Some points of my idea:
>>> - don't think everyone want to use pm-utils (didn't say it is bad)
>> That certainly is true, but I think these people won't have a problem setting
>> up their suspend scripts to trigger the notification anyway. :-)
>>
> 
> But it means almoust always create dbus interface and send message by script.
> For what? only to know if system going resume or suspend?

Hmm.. dbus looks like a bit too much overhead for me, just to bring this
information to your process, there should be an easier way to do this.

> Abosulutly no!
> It's so primitive module and even with different api it could be easy to adept.
> Next if it can't be in kernel source tree for someone could be very usefull.
> 
> This module could only sends bool/ioctl - system resumed.
> 
>> At least, that requires some more discussion, so please tell us why you need
>> the kernel to notify the user space about suspend/hibernation.  IOW, what's the
>> final purpose of this?  [Added some CCs.]
> 
> Yes, it is only first step.
> Have created different point of view, not all linux boxes are desktops/laptops.
> What about embedeed solutions?
> Why app must implement all other to know about resume/suspend?
> Why not open file and know this easily?

I actually don't like the idea to put such information into a file
either. Anyways, if you'd have a thread polling for these file contents,
I think a thread that does netlink comunication (from the userspace
side) would be the better soultion. Then, your module takes care to
broadcast the message to the registered clients.

Have a look at:

http://www.linuxjournal.com/article/7356

> What about this discusion:
> http://lists.freedesktop.org/archives/devkit-devel/2009-December/000617.html
> 
> I will perform some tests to know what amount of time is usualy needed to disconnect
> nicely client or something.

Actually I think this is what signals are there for and bringing this
information via signals would have least overhead, problem is that this
is not POSIX compliant, but may be you could have a try at this?!

Regards,
Daniel


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

Re: [suspend/resume] Re: userspace notification from module

[Daniel Borkmann]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

Daniel Borkmann wrote:
> Bartłomiej Zimoń wrote:
>> What about this discusion:
>> http://lists.freedesktop.org/archives/devkit-devel/2009-December/000617.html
>>
>> I will perform some tests to know what amount of time is usualy needed to disconnect
>> nicely client or something.
> 
> Actually I think this is what signals are there for and bringing this
> information via signals would have least overhead, problem is that this
> is not POSIX compliant, but may be you could have a try at this?!

I'm not quite sure how this is implemented within the kernel, but if you
have lots of processes doing their suspend routines, I think it is not
guaranteed that all of this finishes before doing the suspend, so you
will have some unknown states, processes could stuck at (and later [at
some unintended point of time] resume on).
Or, on the other hand you will have to block the kernel notification
chain until all the procs have signaled that they're done doing their
jobs. Regarding this, the kernel suspend would depend on the correctness
/ termination of userspace routines which is a _very_ bad thing.
You will have to introducte some timeouts... see where this is going? I
think a file interface might be too simple... just some thoughts about this.

Regards,
Daniel


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Sunday 03 January 2010, Bartłomiej Zimoń wrote:
> Dnia 3 stycznia 2010 11:06 	Daniel Borkmann <danborkmann@googlemail.com> napisał(a):
> > Daniel Borkmann wrote:
> > > Bartłomiej Zimoń wrote:
> > >> What about this discusion:
> > >> http://lists.freedesktop.org/archives/devkit-devel/2009-December/000617.html
> > >>
> > >> I will perform some tests to know what amount of time is usualy needed to disconnect
> > >> nicely client or something.
> > > 
> > > Actually I think this is what signals are there for and bringing this
> > > information via signals would have least overhead, problem is that this
> > > is not POSIX compliant, but may be you could have a try at this?!
> > 
> > I'm not quite sure how this is implemented within the kernel, but if you
> > have lots of processes doing their suspend routines, I think it is not
> > guaranteed that all of this finishes before doing the suspend, so you
> > will have some unknown states, processes could stuck at (and later [at
> > some unintended point of time] resume on).
> > Or, on the other hand you will have to block the kernel notification
> > chain until all the procs have signaled that they're done doing their
> > jobs. Regarding this, the kernel suspend would depend on the correctness
> > / termination of userspace routines which is a _very_ bad thing.
> > You will have to introducte some timeouts... see where this is going? I
> > think a file interface might be too simple... just some thoughts about this.
> > 
> 
> mhm, why not to create kernel based pm event messaging for processes?
> How it is implemented on other platforms?
> Because on MacOsX looks like program registers callback for such event.
> 
> I dont know if every pm_notifier blocks suspend until return from callback.
> 
> If we cant do it simple we can do it better.
> Rafael what do You think about it?

I've just said, roughly, in a message I sent to you a while back.

You need a power manager, but not necessarily in the kernel.  The role of the
power manager would be to:
(1) pass suspend requests from different sources in the user space (a GUI for
    one example) to the kernel,
(2) notify processes which registered for that when it's going to pass a
    suspend (or hibernate) request to the kernel,
(3) wait for the notified processes to complete the pre-suspend preparations
    they require.

There are a few more things to consider here.  For example, what if one of the
registered processes becomes unresponsive?  Are we going to suspend anyway
or notify the user and wait for him to resolve the problem?  In the latter
case, what to do if that is, for example, an emergency hibernation started
because we're running out of battery power?  Etc.

Some time ago openSUSE had a daemon called powersaved used for this purpose,
but then it was replaced by pm-utils, apparently because everybody else was
using pm-utils and it just wasn't worth maintaining a different solution for
one distro only.  Maybe it's time to rethink this idea?

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Stefan Seyfried]

Hi,

On Sun, 3 Jan 2010 22:49:11 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Sunday 03 January 2010, Bartłomiej Zimoń wrote:
> > mhm, why not to create kernel based pm event messaging for processes?

I don't think you need the kernel for that.

> > How it is implemented on other platforms?
> > Because on MacOsX looks like program registers callback for such event.
> > 
> > I dont know if every pm_notifier blocks suspend until return from callback.
> > 
> > If we cant do it simple we can do it better.
> > Rafael what do You think about it?

Basically, modern desktops do it exactly this way. They have one "power
manager" (be it gnome-power-manager or KDE's solid) and the
applications are notified by it if a suspend is going to happen.

E.g. the power manager will tell the volume manager (the thing that
mounted the USB stick) "we will suspend, please unmount everything".
Other desktop components will also be notified.

Some processes might also wish to inhibit the suspend (e.g. a CD
burning application while it is burning).

The power manager is the "single point of decision". It might decide to
ignore the CD burning application, if the power is critically low
(better a ruined CD than data loss by hard poweroff), but honor it
otherwise. It might decide to ask the user (and it has an easy way to
do so, since it is running in the users session and thus has access to
the X display), but proceed after a timeout etc...

All this is already possible and does not need any kernel changes at
all.

> I've just said, roughly, in a message I sent to you a while back.
> 
> You need a power manager, but not necessarily in the kernel.  The role of the
> power manager would be to:
> (1) pass suspend requests from different sources in the user space (a GUI for
>     one example) to the kernel,
> (2) notify processes which registered for that when it's going to pass a
>     suspend (or hibernate) request to the kernel,
> (3) wait for the notified processes to complete the pre-suspend preparations
>     they require.

Exactly.

> There are a few more things to consider here.  For example, what if one of the
> registered processes becomes unresponsive?  Are we going to suspend anyway
> or notify the user and wait for him to resolve the problem?  In the latter
> case, what to do if that is, for example, an emergency hibernation started
> because we're running out of battery power?  Etc.
> 
> Some time ago openSUSE had a daemon called powersaved used for this purpose,
> but then it was replaced by pm-utils, apparently because everybody else was
> using pm-utils and it just wasn't worth maintaining a different solution for
> one distro only.  Maybe it's time to rethink this idea?

No. Because what powersaved did was not fundamentally different from
what gnome-power-manager / pm-utils does now. The difference was, that
it was a daemon running as root. Which brought a few problems of its
own:
 * communication with the user was not easily possible
 * it mixed up policy and mechanism (powersaved decided what to do if
   you press the power or sleep button, and you needed root to configure
   that behaviour).

And before somebody comes along with arguments like "but what about my
manually mounted NFS share - who will unmount that?", I can only tell
you: "go away". There is no way to imlement a sane solution that will
take care of all the crazy manual configurations people can dream of,
so you can only tell them "you mounted it manually - so unmount it
manually".

And you can give them pm-utils hooks that give them a mechanism to do
their manual stuff semi-automatically during suspend / resume.

Have fun :-)

	seife
-- 
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out."

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Sunday 03 January 2010, Daniel Borkmann wrote:
...
> Actually I think this is what signals are there for and bringing this
> information via signals would have least overhead, problem is that this
> is not POSIX compliant, but may be you could have a try at this?!

You need information to be passed both ways.

Namely, a power manager (be it a kernel module or a user space process) has
to communicate to the (other) processes that it's about to suspend and it
should wait for each of them to say that it's now OK to suspend.

You could do that with signals, but I guess it's not the easiest way. :-)

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Sunday 03 January 2010, Bartłomiej Zimoń wrote:
> Dnia 3 stycznia 2010 0:29 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > > Thanks for Your answare.
> > > Some points of my idea:
> > > - don't think everyone want to use pm-utils (didn't say it is bad)
> > 
> > That certainly is true, but I think these people won't have a problem setting
> > up their suspend scripts to trigger the notification anyway. :-)
> > 
> 
> But it means almoust always create dbus interface and send message by script.

Of course you don't need to do that.  For example, you can use a UNIX domain
socket for sending the notification from one user space process (a power
manager) to another one (application wanting to be notified) and the
confirmation that's safe to suspend the other way around.  Generally, arbitrary
message-passing interface between two processes would be fine IMO.

...
> > To put it in a different way, you apparently want the kernel to notify the user
> > space of an event originating from the user space and my question is why not
> > to set up the user space to generate the notification without relying on the
> > kernel to do that.
> 
> Because now kernel know better what is going on.

That's because it's just been told by the user space about that.

Basically, you want something like this to happen:

process A ->(suspend) kernel
kernel ->(suspending) process B

where the kernel won't wait for process B to do whatever it has to do before
suspending.  In my opinion it'd be better to do something like this

process A ->(suspending) process B
process B ->(ack) process A
process A ->(suspend) kernel

... 
> > At least, that requires some more discussion, so please tell us why you need
> > the kernel to notify the user space about suspend/hibernation.  IOW, what's the
> > final purpose of this?  [Added some CCs.]
> 
> Yes, it is only first step.
> Have created different point of view, not all linux boxes are desktops/laptops.
> What about embedeed solutions?
> Why app must implement all other to know about resume/suspend?
> Why not open file and know this easily?

And why not to open a socket?

Really, what I think you want is a standard way of notifying applications that
suspend or hibernation is going to happen, but you don't need the kernel to
take part in that directly.

Putting things into the kernel is not the only way to avoid overheads IMO.

... 
> First of all i want to start discussion about this topic and looks like started :)
> 
> So what was in my mind? There are lots of small devices today with linux.
> Lots of them has got unstandardized suspend/resume detection.

OK, so we need a standard here.  I can't agree more.  However, does that mean
the kernel has to be directly involved in that?  If it does, then why exactly?

> It could be too much info exposed from kernel by this module/propose i understand
> for program info about pm_post_resume event could be anought.
> 
> We have now 3types of suspend implementation and 1 kernel API inside.
> App typicaly need just 2-3 event types - suspending, resumming, idle.
> I dont want to slow down kernel suspend, block or something, just perform 
> some actions in apps - typicaly try to reconnect.
> It could be new kernel standard to easy adept some actions.
> 
> Why not pm-utils connect to such module and gather data?
> Then It could work as hal service.
> But hook on kernel like /sbin/init but for suspend and resume, looks like other solution.
> IOW sending event to kernel to perform action and launch userspace once 
> more just before pm-event chain ... check return_val, sounds like other 
> solution/ other kind of module.

Again, you don't need a kernel module for that.  Moreover, using a kernel
module for that is actually inconvenient.

> What about this discusion:
> http://lists.freedesktop.org/archives/devkit-devel/2009-December/000617.html

Well, it would be nice if the desktop people sent CCs of such discussions to
linux-pm.  Talking about the kernel without involving people who actually work
on it is not exactly productive IMO.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Sunday 03 January 2010, Bartłomiej Zimoń wrote:
> Dnia 3 stycznia 2010 22:29 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
...
> 
> It could be even UIO module but there are no pm events reachable there?
> 
> If it is not clean, we must extend pm-utils or write something new
> (with backends dbus, ipc, scripts, ...)
> But You see? We still have no information from kernel about events 
> (especialy resume) or maybe i dont see this ;/

They are available to the process that tells the kernel to suspend.

Namely, to tell the kernel to suspend to RAM, the process (call it a power
manager) needs to (for example) fprintf() "mem" to /sys/power/state.  As soon
as this happens, the kernel will start to freeze processes (except for the
power manager itself), so the power manager knows that everything should be
ready for the suspend before it writes to /sys/power/state.  It doesn't need
the kernel to tell it when the suspend is going to start, because it _knows_
that in advance.

Now, the fprintf() used to trigger the suspend will not return until the resume
is complete.  So, again, when the fprintf() returns, the power manager will know
that the resume has just finished (more precisely, the kernel side of it has
just finished).

There simply is no need for any special communication between the kernel and
the power manager.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 4 stycznia 2010 0:30 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):

> On Sunday 03 January 2010, Bartłomiej Zimoń wrote:
> > Dnia 3 stycznia 2010 22:29 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> ...
> > 
> > It could be even UIO module but there are no pm events reachable there?
> > 
> > If it is not clean, we must extend pm-utils or write something new
> > (with backends dbus, ipc, scripts, ...)
> > But You see? We still have no information from kernel about events 
> > (especialy resume) or maybe i dont see this ;/
> 
> They are available to the process that tells the kernel to suspend.
> 
> Namely, to tell the kernel to suspend to RAM, the process (call it a power
> manager) needs to (for example) fprintf() "mem" to /sys/power/state.  As soon
> as this happens, the kernel will start to freeze processes (except for the
> power manager itself), so the power manager knows that everything should be
> ready for the suspend before it writes to /sys/power/state.  It doesn't need
> the kernel to tell it when the suspend is going to start, because it _knows_
> that in advance.
> 
> Now, the fprintf() used to trigger the suspend will not return until the resume
> is complete.  So, again, when the fprintf() returns, the power manager will know
> that the resume has just finished (more precisely, the kernel side of it has
> just finished).
> 
> There simply is no need for any special communication between the kernel and
> the power manager.
> 

Thx Rafael - now it clear to me.
And what do You think about sending extra signals to processes?

Best regards.
Bartłomiej Zimoń
PLD Linux, Kadu Team, FreeRunner user
http://kadu-im.blogspot.com/

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Monday 04 January 2010, Bartłomiej Zimoń wrote:
> Dnia 4 stycznia 2010 0:30 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> 
> > On Sunday 03 January 2010, Bartłomiej Zimoń wrote:
> > > Dnia 3 stycznia 2010 22:29 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > ...
> > > 
> > > It could be even UIO module but there are no pm events reachable there?
> > > 
> > > If it is not clean, we must extend pm-utils or write something new
> > > (with backends dbus, ipc, scripts, ...)
> > > But You see? We still have no information from kernel about events 
> > > (especialy resume) or maybe i dont see this ;/
> > 
> > They are available to the process that tells the kernel to suspend.
> > 
> > Namely, to tell the kernel to suspend to RAM, the process (call it a power
> > manager) needs to (for example) fprintf() "mem" to /sys/power/state.  As soon
> > as this happens, the kernel will start to freeze processes (except for the
> > power manager itself), so the power manager knows that everything should be
> > ready for the suspend before it writes to /sys/power/state.  It doesn't need
> > the kernel to tell it when the suspend is going to start, because it _knows_
> > that in advance.
> > 
> > Now, the fprintf() used to trigger the suspend will not return until the resume
> > is complete.  So, again, when the fprintf() returns, the power manager will know
> > that the resume has just finished (more precisely, the kernel side of it has
> > just finished).
> > 
> > There simply is no need for any special communication between the kernel and
> > the power manager.
> > 
> 
> Thx Rafael - now it clear to me.
> And what do You think about sending extra signals to processes?

I don't see a problem with this in principle, although I don't think signals
are very suitable for this particular purpose, because you need two-way
communication between the power manager and the processes it's going to
notify (because it has to wait for the processes to finish their preparations
and to tell it that they are ready).

For this purpose it's better to have a file descriptor you can block on (like a
socket or a pipe) while the other side is doing it's job.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 4 stycznia 2010 0:45 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> On Monday 04 January 2010, Bartłomiej Zimoń wrote:
> > Thx Rafael - now it clear to me.
> > And what do You think about sending extra signals to processes?
> 
> I don't see a problem with this in principle, although I don't think signals
> are very suitable for this particular purpose, because you need two-way
> communication between the power manager and the processes it's going to
> notify (because it has to wait for the processes to finish their preparations
> and to tell it that they are ready).
> 
> For this purpose it's better to have a file descriptor you can block on (like a
> socket or a pipe) while the other side is doing it's job.
> 

So let's create one in power/user.c or use /sys/power/state for this :)

Best regards.
Bartłomiej Zimoń
PLD Linux, Kadu Team, FreeRunner user
http://kadu-im.blogspot.com/

Re: [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 4 stycznia 2010 1:51 	Bartłomiej Zimoń <uzi18@o2.pl> napisał(a):
> Dnia 4 stycznia 2010 0:45 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > On Monday 04 January 2010, Bartłomiej Zimoń wrote:
> > > Thx Rafael - now it clear to me.
> > > And what do You think about sending extra signals to processes?
> > 
> > I don't see a problem with this in principle, although I don't think signals
> > are very suitable for this particular purpose, because you need two-way
> > communication between the power manager and the processes it's going to
> > notify (because it has to wait for the processes to finish their preparations
> > and to tell it that they are ready).
> > 
> > For this purpose it's better to have a file descriptor you can block on (like a
> > socket or a pipe) while the other side is doing it's job.
> > 
> 
> So let's create one in power/user.c or use /sys/power/state for this :)

not user.c but: kernel/power/main.c

Best regards.
Bartłomiej Zimoń
PLD Linux, Kadu Team, FreeRunner user
http://kadu-im.blogspot.com/
 

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Monday 04 January 2010, Bartłomiej Zimoń wrote:
> Dnia 4 stycznia 2010 0:45 "Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > On Monday 04 January 2010, Bartłomiej Zimoń wrote:
> > > Thx Rafael - now it clear to me.
> > > And what do You think about sending extra signals to processes?
> > 
> > I don't see a problem with this in principle, although I don't think signals
> > are very suitable for this particular purpose, because you need two-way
> > communication between the power manager and the processes it's going to
> > notify (because it has to wait for the processes to finish their preparations
> > and to tell it that they are ready).
> > 
> > For this purpose it's better to have a file descriptor you can block on (like a
> > socket or a pipe) while the other side is doing it's job.
> > 
> 
> So let's create one in power/user.c or use /sys/power/state for this :)

I don't see a good reason, really.

Rafael

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Anders Eriksson]

rjw@sisk.pl said:
>> > I don't see a problem with this in principle, although I don't think signals
>> > are very suitable for this particular purpose, because you need two-way
>> > communication between the power manager and the processes it's going to
>> > notify (because it has to wait for the processes to finish their preparations
>> > and to tell it that they are ready). 

Wouldn't there need to be dependecy tracking for the userspace processes? A 
process couldn't signal "done" until it know there's no more work to do, which 
requires all other processes to finish up first.

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Tuesday 05 January 2010, Bartłomiej Zimoń wrote:
> Dnia 5 stycznia 2010 10:07 	Anders Eriksson <aeriksson@fastmail.fm> napisał(a):
> > 
> > rjw@sisk.pl said:
> > >> > I don't see a problem with this in principle, although I don't think signals
> > >> > are very suitable for this particular purpose, because you need two-way
> > >> > communication between the power manager and the processes it's going to
> > >> > notify (because it has to wait for the processes to finish their preparations
> > >> > and to tell it that they are ready). 
> > 
> > Wouldn't there need to be dependecy tracking for the userspace processes? A 
> > process couldn't signal "done" until it know there's no more work to do, which 
> > requires all other processes to finish up first.
> > 
> 
> Not all processes will need pm notification.
> 
> Just in case, could this "power manager" be provided as userspace
> driver for kernel (for example compiled as UIO)?

I don't really understand why you want to do that in the kernel.  Is there
any particular reason why this cannot be done in the user space?

Rafael

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 5 stycznia 2010 22:23 	"Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> On Tuesday 05 January 2010, Bartłomiej Zimoń wrote:
> > Dnia 5 stycznia 2010 10:07 	Anders Eriksson <aeriksson@fastmail.fm> napisał(a):
> > > 
> > > rjw@sisk.pl said:
> > > >> > I don't see a problem with this in principle, although I don't think signals
> > > >> > are very suitable for this particular purpose, because you need two-way
> > > >> > communication between the power manager and the processes it's going to
> > > >> > notify (because it has to wait for the processes to finish their preparations
> > > >> > and to tell it that they are ready). 
> > > 
> > > Wouldn't there need to be dependecy tracking for the userspace processes? A 
> > > process couldn't signal "done" until it know there's no more work to do, which 
> > > requires all other processes to finish up first.
> > > 
> > 
> > Not all processes will need pm notification.
> > 
> > Just in case, could this "power manager" be provided as userspace
> > driver for kernel (for example compiled as UIO)?
> 
> I don't really understand why you want to do that in the kernel.  Is there
> any particular reason why this cannot be done in the user space?
> 

Problem is with notification from pm-utils. Dbus for session is only 
user session waid and it is almost imposible to send something 
from systembus or it needs lots of magic.

Even if i'll use pm-utils, must provide way to notify my app.
Looks like no standard way here, that's bad, every app has it's own?

I think this way - if kernel is processes manager and if it sends
"some unknown" signal to every process before kernel suspend so why
not send sepcific and known signal instread.

But now we have DeviceKit-power/upower whatever but it doesn't provide
this feature or something simmilar to relay on.

Best regards.
Bartłomiej Zimoń
PLD Linux, Kadu Team, FreeRunner user
http://kadu-im.blogspot.com/

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Tuesday 05 January 2010, Bartłomiej Zimoń wrote:
> Dnia 5 stycznia 2010 22:23 	"Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > On Tuesday 05 January 2010, Bartłomiej Zimoń wrote:
> > > Dnia 5 stycznia 2010 10:07 	Anders Eriksson <aeriksson@fastmail.fm> napisał(a):
> > > > 
> > > > rjw@sisk.pl said:
> > > > >> > I don't see a problem with this in principle, although I don't think signals
> > > > >> > are very suitable for this particular purpose, because you need two-way
> > > > >> > communication between the power manager and the processes it's going to
> > > > >> > notify (because it has to wait for the processes to finish their preparations
> > > > >> > and to tell it that they are ready). 
> > > > 
> > > > Wouldn't there need to be dependecy tracking for the userspace processes? A 
> > > > process couldn't signal "done" until it know there's no more work to do, which 
> > > > requires all other processes to finish up first.
> > > > 
> > > 
> > > Not all processes will need pm notification.
> > > 
> > > Just in case, could this "power manager" be provided as userspace
> > > driver for kernel (for example compiled as UIO)?
> > 
> > I don't really understand why you want to do that in the kernel.  Is there
> > any particular reason why this cannot be done in the user space?
> > 
> 
> Problem is with notification from pm-utils. Dbus for session is only 
> user session waid and it is almost imposible to send something 
> from systembus or it needs lots of magic.

Don't use dbus, then.

> Even if i'll use pm-utils, must provide way to notify my app.
> Looks like no standard way here, that's bad, every app has it's own?

Isn't there a possibility to introduce a standard way of doing that without
involving the kernel directly?

> I think this way - if kernel is processes manager and if it sends
> "some unknown" signal to every process before kernel suspend so why
> not send sepcific and known signal instread.

We _can_ do that, but what would a process be supposed to do after receiving
such a signal?

> But now we have DeviceKit-power/upower whatever but it doesn't provide
> this feature or something simmilar to relay on.

You're still not saying what you really need.  So, what's the application you
have in mind that needs a notification from the _kernel_ so badly?

Rafael

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Anders Eriksson]

uzi18@o2.pl said:
>  Ummm sorry, it's IM App, to be realible for such app it's needed to close
> conn before suspend, because after resume tcp connections will wait
> (especialy on linux) quiet a lot of time to broke and reconnect.

I don't know the details here, but shouldn't the kernel fix this internally?

If sufficient time have elapsed so the kernel _should_ have sent keep alives 
etc on an othervise idle connection, they shold be sent immediately on resume. 
If the other end has disappeared by then, the resulting port unreachable 
should trigger a local closure of the tcp state which the app will notice 
(EBADF, or similar). Or?

Seems to me the kernel should sit on enough info to handle this nicely. 

Re: [linux-pm] [suspend/resume] Re: userspace notific ation from module

[Bartłomiej Zimoń]

Dnia 6 stycznia 2010 15:35 	Anders Eriksson <aeriksson@fastmail.fm> napisał(a):
> 
> uzi18@o2.pl said:
> >  Ummm sorry, it's IM App, to be realible for such app it's needed to close
> > conn before suspend, because after resume tcp connections will wait
> > (especialy on linux) quiet a lot of time to broke and reconnect.
> 
> I don't know the details here, but shouldn't the kernel fix this internally?
> 
> If sufficient time have elapsed so the kernel _should_ have sent keep alives 
> etc on an othervise idle connection, they shold be sent immediately on resume. 
> If the other end has disappeared by then, the resulting port unreachable 
> should trigger a local closure of the tcp state which the app will notice 
> (EBADF, or similar). Or?
> 
> Seems to me the kernel should sit on enough info to handle this nicely. 
> 

Good bohevior is client to close session before suspend to not loose any message.

Best Regards
Bartlomiej Zimon

Re: [linux-pm] [suspend/resume] Re: userspace notific ation from module

[Roland Dreier]

thanks, applied.

Re: [linux-pm] [suspend/resume] Re: userspace notific ation from module

[Roland Dreier]

 > thanks, applied.

MUA hickup, sorry -- replied to wrong mail.

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Pavel Machek]

On Wed 2010-01-06 15:35:34, Anders Eriksson wrote:
> 
> uzi18@o2.pl said:
> >  Ummm sorry, it's IM App, to be realible for such app it's needed to close
> > conn before suspend, because after resume tcp connections will wait
> > (especialy on linux) quiet a lot of time to broke and reconnect.
> 
> I don't know the details here, but shouldn't the kernel fix this internally?
> 
> If sufficient time have elapsed so the kernel _should_ have sent keep alives 
> etc on an othervise idle connection, they shold be sent immediately on resume. 
> If the other end has disappeared by then, the resulting port unreachable 
> should trigger a local closure of the tcp state which the app will notice 
> (EBADF, or similar). Or?

Patches welcome.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Stefan Seyfried]

On Tue, 05 Jan 2010 10:07:06 +0100
Anders Eriksson <aeriksson@fastmail.fm> wrote:

> 
> rjw@sisk.pl said:
> >> > I don't see a problem with this in principle, although I don't think signals
> >> > are very suitable for this particular purpose, because you need two-way
> >> > communication between the power manager and the processes it's going to
> >> > notify (because it has to wait for the processes to finish their preparations
> >> > and to tell it that they are ready). 
> 
> Wouldn't there need to be dependecy tracking for the userspace processes? A 
> process couldn't signal "done" until it know there's no more work to do, which 
> requires all other processes to finish up first.

No. 99% of the processes don't care about suspend. They don't need
notifications or anything.
The few that do care, register themselves with the power manager. They
get notified before suspend and the power manager might wait until they
tell him that they are ready.
A special case are processes that only want to inhibit suspend - the CD
burning application case - they just tell the power manager "I am
important and you must not suspend now". They do this even if there is
no suspend notification, and once they are done with the critical part
of their work, they remove their "inhibit flag".

This all works pretty well already, and is really not very complicated.
-- 
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out."

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Tuesday 05 January 2010, Bartłomiej Zimoń wrote:
> Dnia 5 stycznia 2010 22:26 	Stefan Seyfried <stefan.seyfried@googlemail.com> napisał(a):
> > On Tue, 05 Jan 2010 10:07:06 +0100
> > Anders Eriksson <aeriksson@fastmail.fm> wrote:
> > 
> > > 
> > > rjw@sisk.pl said:
> > > >> > I don't see a problem with this in principle, although I don't think signals
> > > >> > are very suitable for this particular purpose, because you need two-way
> > > >> > communication between the power manager and the processes it's going to
> > > >> > notify (because it has to wait for the processes to finish their preparations
> > > >> > and to tell it that they are ready). 
> > > 
> > > Wouldn't there need to be dependecy tracking for the userspace processes? A 
> > > process couldn't signal "done" until it know there's no more work to do, which 
> > > requires all other processes to finish up first.
> > 
> > No. 99% of the processes don't care about suspend. They don't need
> > notifications or anything.
> > The few that do care, register themselves with the power manager. They
> > get notified before suspend and the power manager might wait until they
> > tell him that they are ready.
> > A special case are processes that only want to inhibit suspend - the CD
> > burning application case - they just tell the power manager "I am
> > important and you must not suspend now". They do this even if there is
> > no suspend notification, and once they are done with the critical part
> > of their work, they remove their "inhibit flag".
> > 
> > This all works pretty well already, and is really not very complicated.
> 
> So pm could be very simple, and i think we don't need more.
> I want ask You how usualy such user (not root) process notification is done,
> You are talking about pm-utils right?

Well, I think you have some specific issue with pm-utils, but you're not
telling us what it is.  So, why exactly is pm-utils not suitable for your
needs?

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Daniel Borkmann]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

Rafael J. Wysocki wrote:
> On Monday 04 January 2010, Bartłomiej Zimoń wrote:
>> And what do You think about sending extra signals to processes?
> 
> I don't see a problem with this in principle, although I don't think signals
> are very suitable for this particular purpose, because you need two-way
> communication between the power manager and the processes it's going to
> notify (because it has to wait for the processes to finish their preparations
> and to tell it that they are ready).

Again, just to abandon some thoughts... do you really need that "two-way
communication"? I mean if the kernel delivers that specific signal to
the process/task_struct [do_signal():handle_signal()] it has to save the
original execution context that will later on be restored after the
non-default signal handling function returns. This is our ACK /
notification for the successful return of the programs "suspend
handler". The kernel module (if such exists) could be notified about
that for instance by a simple notifier hook within kernelspace. I mean
if I see this right, the "two-way" is just for the ACK isn't it?

Best regards,
Daniel


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Monday 04 January 2010, Daniel Borkmann wrote:
> Rafael J. Wysocki wrote:
> > On Monday 04 January 2010, Bartłomiej Zimoń wrote:
> >> And what do You think about sending extra signals to processes?
> > 
> > I don't see a problem with this in principle, although I don't think signals
> > are very suitable for this particular purpose, because you need two-way
> > communication between the power manager and the processes it's going to
> > notify (because it has to wait for the processes to finish their preparations
> > and to tell it that they are ready).
> 
> Again, just to abandon some thoughts... do you really need that "two-way
> communication"? I mean if the kernel delivers that specific signal to
> the process/task_struct [do_signal():handle_signal()] it has to save the
> original execution context that will later on be restored after the
> non-default signal handling function returns. This is our ACK /
> notification for the successful return of the programs "suspend
> handler". The kernel module (if such exists) could be notified about
> that for instance by a simple notifier hook within kernelspace. I mean
> if I see this right, the "two-way" is just for the ACK isn't it?

_If_ the kernel sends the signals, which is not I think should be done.

Please keep that in the user space.  Really.

I don't see _any_ good reason for putting such things into the kernel.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 3 stycznia 2010 0:29 	"Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > - could be even used partialy by pm-utils
> > - it is perfect just to notify about event
> 
> OK, but why exactly do _you_ need that to happen?
> 
> There's one important drawback of making the kernel generate the notification.
> Namely, even if your userspace task is notified by the kernel of a PM event,
> that doesn't mean it'll have the time to act upon that, because the kernel will
> attempt to freeze it right after the notification has been sent.  This means
> you'd need a way to make the kernel wait for your user space process to finish
> it's job before continuing the suspend/hibernate process.  Otherwise it's not
> going to be very useful.
> 

So if it is bad idea, we could create more advenced implementation.
i.e. IPC or callbacks for this kind of events.

There is vision in my mind to have alternative to pm-utils, 
but even more powerfull and easy to implement in program.
I could try to implement something like that.

Rafael i'm open for Your suggestions.

Best regards.
Bartłomiej Zimoń
PLD Linux, Kadu Team, FreeRunner user
http://kadu-im.blogspot.com/

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Sunday 03 January 2010, Bartłomiej Zimoń wrote:
> Dnia 3 stycznia 2010 0:29 	"Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > > - could be even used partialy by pm-utils
> > > - it is perfect just to notify about event
> > 
> > OK, but why exactly do _you_ need that to happen?
> > 
> > There's one important drawback of making the kernel generate the notification.
> > Namely, even if your userspace task is notified by the kernel of a PM event,
> > that doesn't mean it'll have the time to act upon that, because the kernel will
> > attempt to freeze it right after the notification has been sent.  This means
> > you'd need a way to make the kernel wait for your user space process to finish
> > it's job before continuing the suspend/hibernate process.  Otherwise it's not
> > going to be very useful.
> > 
> 
> So if it is bad idea, we could create more advenced implementation.
> i.e. IPC or callbacks for this kind of events.
> 
> There is vision in my mind to have alternative to pm-utils, 
> but even more powerfull and easy to implement in program.
> I could try to implement something like that.

I guess there's no other way to show people what you're up to, so please go
ahead and let me know if you need help.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

> > Just need as fast as possible solution and on the other hand acceptable for kernel sources.
> > Usually programs needs just to disconnect something or set one flag.
> > Even if program will have no time for this it could be enough just to send this precious info.
> 
> Perhaps I don't understand correctly what you're trying to achieve, but at the
> moment suspend is always started from user space, this way or another, and on

At least zaurus (arm) suspends from kernel on battery critical.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 09 January 2010, Pavel Machek wrote:
> > > Just need as fast as possible solution and on the other hand acceptable for kernel sources.
> > > Usually programs needs just to disconnect something or set one flag.
> > > Even if program will have no time for this it could be enough just to send this precious info.
> > 
> > Perhaps I don't understand correctly what you're trying to achieve, but at the
> > moment suspend is always started from user space, this way or another, and on
> 
> At least zaurus (arm) suspends from kernel on battery critical.

I wasn't aware of this.

That may be a good reason for adding kernel-based suspend notification,
although I'd prefer ARM to notify the user space about the critical battery
status allowing it to decide what to do.

IMhO automatic suspend without something like the Android's wakelocks hurts
more than it helps.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

On Sat 2010-01-09 14:40:46, Rafael J. Wysocki wrote:
> On Saturday 09 January 2010, Pavel Machek wrote:

> > > Perhaps I don't understand correctly what you're trying to achieve, but at the
> > > moment suspend is always started from user space, this way or another, and on
> > 
> > At least zaurus (arm) suspends from kernel on battery critical.
> 
> I wasn't aware of this.
> 
> That may be a good reason for adding kernel-based suspend notification,
> although I'd prefer ARM to notify the user space about the critical battery
> status allowing it to decide what to do.

Hard to do, without breaking compatibility that goes down to 2.4.X.

> IMhO automatic suspend without something like the Android's wakelocks hurts
> more than it helps.

It really makes sense on zaurus. Those machines are simple, no
smartbattery and no embedded controller subsystems. Battery will not
protect itself, and its kernel job. (Should work on init=/bin/bash).

As power-off consumption is same as suspend power consumption (I
beleive zaurus simply does not have true power off), suspend on
critical makes some sense. (Note that it is set lower than on pcs, and
that we declare battery critical sooner than that.)
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Friday 15 January 2010, Pavel Machek wrote:
> On Sat 2010-01-09 14:40:46, Rafael J. Wysocki wrote:
> > On Saturday 09 January 2010, Pavel Machek wrote:
> 
> > > > Perhaps I don't understand correctly what you're trying to achieve, but at the
> > > > moment suspend is always started from user space, this way or another, and on
> > > 
> > > At least zaurus (arm) suspends from kernel on battery critical.
> > 
> > I wasn't aware of this.
> > 
> > That may be a good reason for adding kernel-based suspend notification,
> > although I'd prefer ARM to notify the user space about the critical battery
> > status allowing it to decide what to do.
> 
> Hard to do, without breaking compatibility that goes down to 2.4.X.

Sending a battery-critical notification to the user space is not equivalent to
removing the existing kernel-based mechanism.  They can exist both at the
same time if the notification is sent earlier than the kernel suspends
everything. 

> > IMhO automatic suspend without something like the Android's wakelocks hurts
> > more than it helps.
> 
> It really makes sense on zaurus. Those machines are simple, no
> smartbattery and no embedded controller subsystems. Battery will not
> protect itself, and its kernel job. (Should work on init=/bin/bash).
> 
> As power-off consumption is same as suspend power consumption (I
> beleive zaurus simply does not have true power off), suspend on
> critical makes some sense. (Note that it is set lower than on pcs, and
> that we declare battery critical sooner than that.)

The problem with that is it catches at least some applications unprepared and
notifying them that "we're suspending right now" doesn't really help, because
they won't have any time to react anyway.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Eric Miao]

On Sat, Jan 16, 2010 at 6:14 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday 15 January 2010, Pavel Machek wrote:
>> On Sat 2010-01-09 14:40:46, Rafael J. Wysocki wrote:
>> > On Saturday 09 January 2010, Pavel Machek wrote:
>>
>> > > > Perhaps I don't understand correctly what you're trying to achieve, but at the
>> > > > moment suspend is always started from user space, this way or another, and on
>> > >
>> > > At least zaurus (arm) suspends from kernel on battery critical.
>> >
>> > I wasn't aware of this.
>> >
>> > That may be a good reason for adding kernel-based suspend notification,
>> > although I'd prefer ARM to notify the user space about the critical battery
>> > status allowing it to decide what to do.
>>
>> Hard to do, without breaking compatibility that goes down to 2.4.X.
>
> Sending a battery-critical notification to the user space is not equivalent to
> removing the existing kernel-based mechanism.  They can exist both at the
> same time if the notification is sent earlier than the kernel suspends
> everything.
>

Pavel,

I'd agree with Rafael that we need to move battery management forward to
a more standard way though we can keep the compatibility atm. The code
of sharpsl-pm.c is no way clean and maintainable. Having a battery driver
instead would be a better way to go in a long run.

And the other way we may need to look into what API the current userland
apps on zaurus is depending on this 2.4 compatibility and make changes
slowly to those apps.

>> > IMhO automatic suspend without something like the Android's wakelocks hurts
>> > more than it helps.
>>
>> It really makes sense on zaurus. Those machines are simple, no
>> smartbattery and no embedded controller subsystems. Battery will not
>> protect itself, and its kernel job. (Should work on init=/bin/bash).
>>
>> As power-off consumption is same as suspend power consumption (I
>> beleive zaurus simply does not have true power off), suspend on
>> critical makes some sense. (Note that it is set lower than on pcs, and
>> that we declare battery critical sooner than that.)
>
> The problem with that is it catches at least some applications unprepared and
> notifying them that "we're suspending right now" doesn't really help, because
> they won't have any time to react anyway.
>
> Rafael
>

Re: [suspend/resume] Re: userspace notification from module

[Stanislav Brabec]

Eric Miao wrote:

> And the other way we may need to look into what API the current userland
> apps on zaurus is depending on this 2.4 compatibility and make changes
> slowly to those apps.

I guess that 2.4 compatibility is not an issue. Most modern Zaurus
distributions are even unable to run Sharp ROM compatible binaries.

Distributions either stay on 2.4 kernel or use modern systems based on
modern kernel 2.6 API.

Distributions that decided to migrate to kernel 2.6 are far from
finished state. Any change that allows to use modern applications using
standard kernel API is welcome.


________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx/zaurus

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

On Sat 2010-01-16 18:00:58, Stanislav Brabec wrote:
> Eric Miao wrote:
> 
> > And the other way we may need to look into what API the current userland
> > apps on zaurus is depending on this 2.4 compatibility and make changes
> > slowly to those apps.
> 
> I guess that 2.4 compatibility is not an issue. Most modern Zaurus
> distributions are even unable to run Sharp ROM compatible binaries.
> 
> Distributions either stay on 2.4 kernel or use modern systems based on
> modern kernel 2.6 API.
> 
> Distributions that decided to migrate to kernel 2.6 are far from
> finished state. Any change that allows to use modern applications using
> standard kernel API is welcome.

There is no API involved. It is just ... if you leave zaurus in
 init=/bin/bash mode, it must not kill the battery. Smart and
 currently implemented way to do that is to suspend.

That's counterexample to rjw, but it does not matter -- reasonable
userland should never ever hit that, in a same way PCs should not hit
emergency power cut...
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 16 January 2010, Pavel Machek wrote:
> On Sat 2010-01-16 18:00:58, Stanislav Brabec wrote:
> > Eric Miao wrote:
> > 
> > > And the other way we may need to look into what API the current userland
> > > apps on zaurus is depending on this 2.4 compatibility and make changes
> > > slowly to those apps.
> > 
> > I guess that 2.4 compatibility is not an issue. Most modern Zaurus
> > distributions are even unable to run Sharp ROM compatible binaries.
> > 
> > Distributions either stay on 2.4 kernel or use modern systems based on
> > modern kernel 2.6 API.
> > 
> > Distributions that decided to migrate to kernel 2.6 are far from
> > finished state. Any change that allows to use modern applications using
> > standard kernel API is welcome.
> 
> There is no API involved. It is just ... if you leave zaurus in
>  init=/bin/bash mode, it must not kill the battery. Smart and
>  currently implemented way to do that is to suspend.

IMHO it should just plain shutdown in that case.  Suspending doesn't really
solve the problem, because the battery is going to drain still.  Unless you
mean suspend=hibernate, but I guess you don't.

> That's counterexample to rjw, but it does not matter -- reasonable
> userland should never ever hit that, in a same way PCs should not hit
> emergency power cut...

I don't really understand what you mean.  The user space doesn't know the
battery state if the kernel doesn't tell it, AFAICS, so how exactly can it
predict the critical battery condition without the kernel notifying it?

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

On Sat 2010-01-16 23:05:56, Rafael J. Wysocki wrote:
> On Saturday 16 January 2010, Pavel Machek wrote:
> > On Sat 2010-01-16 18:00:58, Stanislav Brabec wrote:
> > > Eric Miao wrote:
> > > 
> > > > And the other way we may need to look into what API the current userland
> > > > apps on zaurus is depending on this 2.4 compatibility and make changes
> > > > slowly to those apps.
> > > 
> > > I guess that 2.4 compatibility is not an issue. Most modern Zaurus
> > > distributions are even unable to run Sharp ROM compatible binaries.
> > > 
> > > Distributions either stay on 2.4 kernel or use modern systems based on
> > > modern kernel 2.6 API.
> > > 
> > > Distributions that decided to migrate to kernel 2.6 are far from
> > > finished state. Any change that allows to use modern applications using
> > > standard kernel API is welcome.
> > 
> > There is no API involved. It is just ... if you leave zaurus in
> >  init=/bin/bash mode, it must not kill the battery. Smart and
> >  currently implemented way to do that is to suspend.
> 
> IMHO it should just plain shutdown in that case.  Suspending doesn't really
> solve the problem, because the battery is going to drain still.  Unless you
> mean suspend=hibernate, but I guess you don't.

As I explained before, power consumption on suspend and hibernate and
poweroff is equivalent on zaurus (7mA in all the cases -- sorry if I
said uA before). And because it has 1800mAh battery, it means that
even empty battery is going to last for a while. In practice, it works
very well.

(There are other reasons, having to do with internal li-ion resistances
in aged and cold batteries.)

> > That's counterexample to rjw, but it does not matter -- reasonable
> > userland should never ever hit that, in a same way PCs should not hit
> > emergency power cut...
> 
> I don't really understand what you mean.  The user space doesn't know the
> battery state if the kernel doesn't tell it, AFAICS, so how exactly can it
> predict the critical battery condition without the kernel notifying it?

That was not the point I was trying to discuss. Yes, we need
kernel<->user notification of battery critical. 

But on zaurus, correct action is to suspend (not hibernate and not
poweroff) when battery is no longer able to supply enough power to
keep system alive.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

> > IMHO it should just plain shutdown in that case.  Suspending doesn't really
> > solve the problem, because the battery is going to drain still.  Unless you
> > mean suspend=hibernate, but I guess you don't.

BTW I was talking spitz before, but there's collie, too. It was
shipped in configuration where all user data was in RAM and RAMdisk --
no writable flash. On such machine, shutdown is never option. (And
yes, such design was very common in windowsCE days).

Spitz got disk, but inherited that powermanagement design; and it
works very well.

And now, if you want collie for a year (or two), you can have
mine... it still works and battery still lasted for 5 hours 2 years
ago -- not bad for 10 year old battery. You'll note that it is very
different design from PC.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 16 January 2010, Pavel Machek wrote:
> > > IMHO it should just plain shutdown in that case.  Suspending doesn't really
> > > solve the problem, because the battery is going to drain still.  Unless you
> > > mean suspend=hibernate, but I guess you don't.
> 
> BTW I was talking spitz before, but there's collie, too. It was
> shipped in configuration where all user data was in RAM and RAMdisk --
> no writable flash. On such machine, shutdown is never option. (And
> yes, such design was very common in windowsCE days).
> 
> Spitz got disk, but inherited that powermanagement design; and it
> works very well.
> 
> And now, if you want collie for a year (or two), you can have
> mine... it still works and battery still lasted for 5 hours 2 years
> ago -- not bad for 10 year old battery. You'll note that it is very
> different design from PC.

Well, I guess I wouldn't have the time to study it anyway, so thanks. :-)

All in all, it looks like these particular platforms are just specific design
having special requirements.  And even on these platforms sending a battery
critical notification to the user space before the kernel emergency suspends
(or shuts down or whatever) the system seems to be a good idea in general.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 16 January 2010, Pavel Machek wrote:
> On Sat 2010-01-16 23:05:56, Rafael J. Wysocki wrote:
> > On Saturday 16 January 2010, Pavel Machek wrote:
> > > On Sat 2010-01-16 18:00:58, Stanislav Brabec wrote:
> > > > Eric Miao wrote:
> > > > 
> > > > > And the other way we may need to look into what API the current userland
> > > > > apps on zaurus is depending on this 2.4 compatibility and make changes
> > > > > slowly to those apps.
> > > > 
> > > > I guess that 2.4 compatibility is not an issue. Most modern Zaurus
> > > > distributions are even unable to run Sharp ROM compatible binaries.
> > > > 
> > > > Distributions either stay on 2.4 kernel or use modern systems based on
> > > > modern kernel 2.6 API.
> > > > 
> > > > Distributions that decided to migrate to kernel 2.6 are far from
> > > > finished state. Any change that allows to use modern applications using
> > > > standard kernel API is welcome.
> > > 
> > > There is no API involved. It is just ... if you leave zaurus in
> > >  init=/bin/bash mode, it must not kill the battery. Smart and
> > >  currently implemented way to do that is to suspend.
> > 
> > IMHO it should just plain shutdown in that case.  Suspending doesn't really
> > solve the problem, because the battery is going to drain still.  Unless you
> > mean suspend=hibernate, but I guess you don't.
> 
> As I explained before, power consumption on suspend and hibernate and
> poweroff is equivalent on zaurus (7mA in all the cases -- sorry if I
> said uA before). And because it has 1800mAh battery, it means that
> even empty battery is going to last for a while. In practice, it works
> very well.
> 
> (There are other reasons, having to do with internal li-ion resistances
> in aged and cold batteries.)
> 
> > > That's counterexample to rjw, but it does not matter -- reasonable
> > > userland should never ever hit that, in a same way PCs should not hit
> > > emergency power cut...
> > 
> > I don't really understand what you mean.  The user space doesn't know the
> > battery state if the kernel doesn't tell it, AFAICS, so how exactly can it
> > predict the critical battery condition without the kernel notifying it?
> 
> That was not the point I was trying to discuss. Yes, we need
> kernel<->user notification of battery critical. 
> 
> But on zaurus, correct action is to suspend (not hibernate and not
> poweroff) when battery is no longer able to supply enough power to
> keep system alive.

Why not to poweroff (just asking, I don't know that hardware)?

I guess we should at least do our best to keep filesystems in a consistent
state and suspend doesn't really guarantee this if the system remains on
battery power afterwards.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

> > That was not the point I was trying to discuss. Yes, we need
> > kernel<->user notification of battery critical. 
> > 
> > But on zaurus, correct action is to suspend (not hibernate and not
> > poweroff) when battery is no longer able to supply enough power to
> > keep system alive.
> 
> Why not to poweroff (just asking, I don't know that hardware)?

Those machines basically have no poweroff.

> I guess we should at least do our best to keep filesystems in a consistent
> state and suspend doesn't really guarantee this if the system remains on
> battery power afterwards.

I'm not sure if I ever had battery run so low that it could not keep
RAM running. Collie has even separate (small) battery just for
RAM. (And it also has all the filesystems in ramdisk -- you really do
not want to power it off, even if it could.)

AFAICT following message would be nice.

1) battery is critical, userspace please do something

On zaurus and similar, you could add

2) oh and btw we had power failure so we suspended (or maybe -- so
hardware suspended itself -- rmk's examples and old apm systems); we
are now back and running

notification... but... ideally those power failures should never
happen anyway, so... having this notification is in no way neccessary.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Russell King - ARM Linux]

On Sun, Jan 17, 2010 at 02:07:39PM +0100, Pavel Machek wrote:
> AFAICT following message would be nice.
> 
> 1) battery is critical, userspace please do something
> 
> On zaurus and similar, you could add
> 
> 2) oh and btw we had power failure so we suspended (or maybe -- so
> hardware suspended itself -- rmk's examples and old apm systems); we
> are now back and running
> 
> notification... but... ideally those power failures should never
> happen anyway, so... having this notification is in no way neccessary.

There's another consideration here: the more complex the emergency
procedure, the higher the chance of _something_ causing it to fail,
and if it does fail, the result is data loss.

In a properly running system, this isn't something that's going to
get a lot of testing, so there's a higher chance that there will be
bugs, so the simpler the solution, the better.

It's a bit like the kernel shutdown paths - because they don't get a
lot of use, they don't get tested enough, and having discussed it with
Arjan van de Ven, it's a known weakness.  So we know that they're not
that well tested - and the result is eg, 33-rc3 on shutdown results
in an oops for me on x86.

You really don't want to oops or deadlock on "battery critically low".

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

On Sun 2010-01-17 13:26:18, Russell King - ARM Linux wrote:
> On Sun, Jan 17, 2010 at 02:07:39PM +0100, Pavel Machek wrote:
> > AFAICT following message would be nice.
> > 
> > 1) battery is critical, userspace please do something
> > 
> > On zaurus and similar, you could add
> > 
> > 2) oh and btw we had power failure so we suspended (or maybe -- so
> > hardware suspended itself -- rmk's examples and old apm systems); we
> > are now back and running
> > 
> > notification... but... ideally those power failures should never
> > happen anyway, so... having this notification is in no way neccessary.
> 
> There's another consideration here: the more complex the emergency
> procedure, the higher the chance of _something_ causing it to fail,
> and if it does fail, the result is data loss.

That's why I propose to only send notification after we resume from
emergency suspend :-).

> In a properly running system, this isn't something that's going to
> get a lot of testing, so there's a higher chance that there will be
> bugs, so the simpler the solution, the better.

Unfortunately, with old battery in zaurus, I tested it rather a lot. I
have new one now, but I can still use old one for testing.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Russell King - ARM Linux]

On Sat, Jan 16, 2010 at 11:05:56PM +0100, Rafael J. Wysocki wrote:
> On Saturday 16 January 2010, Pavel Machek wrote:
> > On Sat 2010-01-16 18:00:58, Stanislav Brabec wrote:
> > > Eric Miao wrote:
> > > 
> > > > And the other way we may need to look into what API the current userland
> > > > apps on zaurus is depending on this 2.4 compatibility and make changes
> > > > slowly to those apps.
> > > 
> > > I guess that 2.4 compatibility is not an issue. Most modern Zaurus
> > > distributions are even unable to run Sharp ROM compatible binaries.
> > > 
> > > Distributions either stay on 2.4 kernel or use modern systems based on
> > > modern kernel 2.6 API.
> > > 
> > > Distributions that decided to migrate to kernel 2.6 are far from
> > > finished state. Any change that allows to use modern applications using
> > > standard kernel API is welcome.
> > 
> > There is no API involved. It is just ... if you leave zaurus in
> >  init=/bin/bash mode, it must not kill the battery. Smart and
> >  currently implemented way to do that is to suspend.
> 
> IMHO it should just plain shutdown in that case.  Suspending doesn't really
> solve the problem, because the battery is going to drain still.  Unless you
> mean suspend=hibernate, but I guess you don't.

FYI, most handheld/palm-based devices don't have a "power off" mode,
short of opening them up and disconnecting the battery.  The "on/off"
button is really just a "suspend/resume" button.

So, when the battery gets critically low, your only option is to suspend.
If the battery continues to be drained, and it's a properly manufacturered
li-ion battery, it will have a discharge protection circuit inside the
battery which will effectively disconnect the battery until it's recharged.
There is no warning of this event.

The whole idea here is in a low battery sitution, is to place the system
into as low power state as possible to try to retain the users data.

(Some protection circuits monitor each li-ion cell, which means if you
have a single weak cell, it can trip before you get the critical suspend
notification.)

There are also some SoCs which have a battery fault input, which forces a
transition to suspend mode with no software intervention what so ever,
placing the SDRAM into self-refresh (data retention) mode.  Wakeup from
such a mode can only be done by total SoC reset once the fault input has
been removed.

Either way, the issue here is that when you get a critical low battery
notification, there may be seconds of power left if the system is fully
operational, and you must suspend as quickly as possible before the
hardware does something more destructive to your state.

Probably the best way of thinking about this is a layered set of
notifications:

- monitor battery level, cleanly suspend when down to N%
- if ignored, you get a critical low battery notification which forces
  an unclean suspend transition, retaining most device, CPU and RAM state.
- if ignored, a hardware initiated suspend transition occurs, retaining
  just RAM state.
- if that doesn't work, the battery itself cuts power and all state is
  lost.

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

Hi!

> >> > That may be a good reason for adding kernel-based suspend notification,
> >> > although I'd prefer ARM to notify the user space about the critical battery
> >> > status allowing it to decide what to do.
> >>
> >> Hard to do, without breaking compatibility that goes down to 2.4.X.
> >
> > Sending a battery-critical notification to the user space is not equivalent to
> > removing the existing kernel-based mechanism.  They can exist both at the
> > same time if the notification is sent earlier than the kernel suspends
> > everything.
> 
> I'd agree with Rafael that we need to move battery management forward to
> a more standard way though we can keep the compatibility atm. The code
> of sharpsl-pm.c is no way clean and maintainable. Having a battery driver
> instead would be a better way to go in a long run.

Something like below? Yes, I'd like to get something like that pushed.

> And the other way we may need to look into what API the current userland
> apps on zaurus is depending on this 2.4 compatibility and make changes
> slowly to those apps.

Well, emergency suspend/poweroff should still work in init=/bin/bash
mode. And doing emergency poweroff when we are capable of suspend
seems wrong. 

									Pavel

--- ./drivers/power.ofic/Makefile	2009-10-06 13:51:29.000000000 +0200
+++ ./drivers/power/Makefile	2009-10-11 16:12:09.000000000 +0200
@@ -24,6 +24,7 @@
 obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
 obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
 obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
+obj-m	+= spitz_battery.o
 obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
 obj-$(CONFIG_BATTERY_BQ27x00)	+= bq27x00_battery.o
 obj-$(CONFIG_BATTERY_DA9030)	+= da9030_battery.o
diff -ur ./drivers/power.ofic/power_supply_sysfs.c ./drivers/power/power_supply_sysfs.c
--- ./drivers/power.ofic/power_supply_sysfs.c	2009-10-06 13:51:29.000000000 +0200
+++ ./drivers/power/power_supply_sysfs.c	2009-10-15 05:45:46.000000000 +0200
@@ -39,7 +39,8 @@
 
 static ssize_t power_supply_show_property(struct device *dev,
 					  struct device_attribute *attr,
-					  char *buf) {
+					  char *buf)
+{
 	static char *status_text[] = {
 		"Unknown", "Charging", "Discharging", "Not charging", "Full"
 	};
@@ -135,7 +136,8 @@
 
 static ssize_t power_supply_show_static_attrs(struct device *dev,
 					      struct device_attribute *attr,
-					      char *buf) {
+					      char *buf)
+{
 	static char *type_text[] = { "Battery", "UPS", "Mains", "USB" };
 	struct power_supply *psy = dev_get_drvdata(dev);
 
diff -ur ./drivers/power.ofic/spitz_battery.c ./drivers/power/spitz_battery.c
--- ./drivers/power.ofic/spitz_battery.c	2009-10-11 16:14:11.000000000 +0200
+++ ./drivers/power/spitz_battery.c	2009-10-22 07:27:52.000000000 +0200
@@ -0,0 +1,430 @@
+/*
+ * Battery and Power Management code for the Sharp SL-3000c
+ *
+ * Copyright (c) 2009 Pavel Machek <pavel@ucw.cz>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Li-ion batteries are angry beasts, and they like to explode.
+ * If angry lithium comes your way, the hw was misdesigned.
+ *
+ */
+#include <linux/platform_device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/power_supply.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/gpio.h>
+
+#include <asm/mach-types.h>
+#include <mach/spitz.h>
+#include <mach/sharpsl.h>
+#include <mach/sharpsl_pm.h>
+
+#include "../../arch/arm/mach-pxa/sharpsl.h"
+
+extern struct sharpsl_pm_status sharpsl_pm;
+
+
+struct spitz_bat {
+	struct power_supply psy;
+
+	bool (*is_present)(struct spitz_bat *bat);
+};
+
+static struct spitz_bat spitz_bat_main, spitz_ac;
+
+extern int sharpsl_pm_pxa_read_max1111(int channel);
+
+int basic_current = 125; /* miliAmp */
+int battery_resistance = 422; /* miliOhm */
+
+/* 422 seems to be suitable for very old, 1Ah battery.
+   2Ah battery probably has better resistance */
+
+/* Unfortunately, resistance depends on state of charge, current
+ * direction and temperature.
+ *
+ * Ouch, and dependency is actually _not_ too simple. It is lowest
+ * at 3.55V, very slowly rises at 4V (approximately linear dependency),
+ * and quickly rises towards 3.2V (in something exponential-looking).
+ *
+ * It is about same at 25Celsius and 40Celsius, and about 2.5x the value
+ * on 0Celsius, rising _very_ sharply.
+ *
+ * Li-ion should only be charged between 0 and 45 Celsius, and discharged
+ * between -20 and 60 celsius.
+ */
+
+extern int backlight_current;
+
+int battery_current(void)
+{
+	int intensity = sharpsl_pm.machinfo->backlight_get_status ? sharpsl_pm.machinfo->backlight_get_status() : 0;
+
+	return basic_current + backlight_current;
+}
+
+int liion_internal_voltage(int voltage, int current_ma)
+{
+	return voltage + (battery_resistance * current_ma / 1000);
+}
+
+int liion_expected_voltage(int internal_voltage, int current_ma)
+{       
+	return internal_voltage - (battery_resistance * current_ma / 1000);
+}
+
+/* returns mV */
+int liion_voltage(void)
+{
+	/* Thanks to Stanislav B. ADC has 3.3V as reference,
+	   is connected to battery over 47kOhm,
+	   and to ground over 100kOhm. */
+	return (sharpsl_pm.machinfo->read_devdata(SHARPSL_BATT_VOLT) * 147 * 33)/256;
+}
+
+/* See for example http://www.kokam.com/english/biz/rc.html for
+ * voltage/capacity characteristic. I assume it is going to be
+ * reasonably similar to li-ion used in collie.
+ *
+ */
+
+/*
+ { 420, 100 },
+ { 417, 95 }, means it will report 100% between 418 and 420
+ */
+
+struct battery_thresh battery_levels[] = {
+	{ 3980, 100 },
+	{ 3900, 95 },
+	{ 3860, 90 },
+	{ 3800, 85 },
+	{ 3760, 80 },
+	{ 3720, 74 },
+	{ 3680, 69 },
+	{ 3620, 65 },
+	{ 3570, 59 },
+	{ 3560, 55 },
+	{ 3550, 48 },
+	{ 3530, 45 },
+	{ 3510, 39 },
+	{ 3490, 33 },
+	{ 3470, 29 },
+	{ 3450, 23 },
+	{ 3410, 16 },
+	{    0, 0 },
+};
+
+int get_percentage(void)
+{
+	int i = ARRAY_SIZE(battery_levels);
+	struct battery_thresh *thresh;
+	int voltage = liion_internal_voltage(liion_voltage(), battery_current());
+
+	thresh = battery_levels;
+
+	while (i > 0 && (voltage > thresh[i].voltage))
+		i--;
+
+	return thresh[i].percentage;
+}
+
+static int spitz_bat_get_property(struct power_supply *psy,
+			    enum power_supply_property psp,
+			    union power_supply_propval *val)
+{
+	int ret = 0;
+	struct spitz_bat *bat = container_of(psy, struct spitz_bat, psy);
+
+	val->intval = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_HEALTH:
+		/* POWER_SUPPLY_HEALTH_OVERHEAT , POWER_SUPPLY_HEALTH_COLD,
+		   POWER_SUPPLY_HEALTH_OVERVOLTAGE, POWER_SUPPLY_HEALTH_UNSPEC_FAILURE, POWER_SUPPLY_HEALTH_GOOD
+		 */
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
+	  	if (gpio_get_value(SPITZ_GPIO_CHRG_ON) == 0) {
+			if (gpio_get_value(SPITZ_GPIO_JK_B) == 1)
+				val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
+			else
+				val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+		}
+		return 0;
+	case POWER_SUPPLY_PROP_STATUS:
+	  {
+		int status = 0;
+
+	  	if (gpio_get_value(SPITZ_GPIO_CHRG_ON) == 0)
+			printk("Chrg bit on. ");
+	  	if (gpio_get_value(SPITZ_GPIO_JK_B) == 0)
+			printk("Slow charge bit on. ");
+
+		val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+
+		if (!sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_ACIN))
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		else {
+			if (gpio_get_value(SPITZ_GPIO_CHRG_ON) == 0)
+				val->intval = POWER_SUPPLY_STATUS_CHARGING;
+			if (sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_CHRGFULL))
+				val->intval = POWER_SUPPLY_STATUS_FULL;
+		}
+
+		printk("ACIN: %d ", sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_ACIN));
+		printk("Chrgfull: %d  ", sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_CHRGFULL));
+		printk("Fatal: %d  ", sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_FATAL));
+		printk("ACIN_volt: %d\n", sharpsl_pm.machinfo->read_devdata(SHARPSL_ACIN_VOLT));
+
+		return 0;
+	  }
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = liion_voltage()*1000;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		val->intval = 4200000;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		val->intval = 3400000;
+		return 0;
+	case POWER_SUPPLY_PROP_TEMP:
+		mdelay(SHARPSL_CHECK_BATTERY_WAIT_TIME_TEMP);
+		sharpsl_pm.machinfo->measure_temp(1);
+		mdelay(SHARPSL_CHECK_BATTERY_WAIT_TIME_TEMP);
+		val->intval = sharpsl_pm.machinfo->read_devdata(SHARPSL_BATT_TEMP);
+		sharpsl_pm.machinfo->measure_temp(0);
+		/* 121: battery finished charging in 22C room */
+		/* 141: outside at 6C */
+		return 0;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = "spitz-battery";
+		return 0;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		return 0;
+		/* add these */
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		val->intval = 2000000;
+		return 0;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = get_percentage();
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
+		val->intval = liion_internal_voltage(liion_voltage(), battery_current())*1000;
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = battery_current() * 1000;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static int spitz_ac_get_property(struct power_supply *psy,
+			    enum power_supply_property psp,
+			    union power_supply_propval *val)
+{
+	int ret = 0;
+	struct spitz_bat *bat = container_of(psy, struct spitz_bat, psy);
+
+	val->intval = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		/* Thanks to Stanislav B. ADC has 3.3V as reference,
+		   is connected to acin over 2kOhm,
+		   and to ground over 1kOhm. */
+		val->intval = (sharpsl_pm.machinfo->read_devdata(SHARPSL_ACIN_VOLT) * 3000 * 3300)/256;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		val->intval = 5250000;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		val->intval = 4750000;
+		return 0;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = "spitz-power-supply";
+		return 0;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_ACIN);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static void spitz_bat_external_power_changed(struct power_supply *psy)
+{
+}
+
+
+static enum power_supply_property spitz_bat_main_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_AVG,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_CHARGE_TYPE,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static struct spitz_bat spitz_bat_main = {
+	.psy = {
+		.name		= "main-battery",
+		.type		= POWER_SUPPLY_TYPE_BATTERY,
+		.properties	= spitz_bat_main_props,
+		.num_properties	= ARRAY_SIZE(spitz_bat_main_props),
+		.get_property	= spitz_bat_get_property,
+		.external_power_changed = spitz_bat_external_power_changed,
+		.use_for_apm	= 1,
+	},
+};
+
+static enum power_supply_property spitz_ac_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_PRESENT,
+};
+
+static struct spitz_bat spitz_ac = {
+	.psy = {
+		.name		= "ac",
+		.type		= POWER_SUPPLY_TYPE_MAINS,
+		.properties	= spitz_ac_props,
+		.num_properties	= ARRAY_SIZE(spitz_ac_props),
+		.get_property	= spitz_ac_get_property,
+	},
+};
+
+#ifdef CONFIG_PM
+static int spitz_bat_suspend(struct platform_device *dev, pm_message_t state)
+{
+	return 0;
+}
+
+static int spitz_bat_resume(struct platform_device *dev)
+{
+	return 0;
+}
+#else
+#define spitz_bat_suspend NULL
+#define spitz_bat_resume NULL
+#endif
+
+
+static ssize_t spitz_bat_limit_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	sprintf(buf, "Hello :-)");
+	return 9;
+}
+
+static ssize_t spitz_bat_limit_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	if (!strncmp(buf, "fastcharge", 10)) {
+		gpio_set_value(SPITZ_GPIO_JK_B, 1);
+		gpio_set_value(SPITZ_GPIO_CHRG_ON, 0);
+		return 10;
+	}
+	if (!strncmp(buf, "slowcharge", 10)) {
+		gpio_set_value(SPITZ_GPIO_JK_B, 0);
+		gpio_set_value(SPITZ_GPIO_CHRG_ON, 0);
+		return 10;
+	}
+	if (!strncmp(buf, "nonecharge", 10)) {
+		gpio_set_value(SPITZ_GPIO_JK_B, 0);
+		gpio_set_value(SPITZ_GPIO_CHRG_ON, 1);
+		return 10;
+	}
+	return -EINVAL;
+}
+
+
+static DEVICE_ATTR(limit, S_IRUGO | S_IWUSR, spitz_bat_limit_show, spitz_bat_limit_store);
+#if 0
+static struct device_attribute dev_attr_limit = {
+        .attr = { .name = "limit", .mode = 0644 },
+        .show = spitz_bat_limit_show,
+        .store = spitz_bat_limit_store,
+};
+#endif
+
+static int __devinit spitz_bat_probe(struct platform_device *dev)
+{
+	int ret;
+	int i;
+
+	if (!machine_is_spitz())
+		return -ENODEV;
+
+	printk("spitz_bat_probe: register\n");
+	power_supply_register(&dev->dev, &spitz_bat_main.psy);
+	power_supply_register(&dev->dev, &spitz_ac.psy);
+	device_create_file(&dev->dev, &dev_attr_limit);
+
+	return 0;
+}
+
+static int __devexit spitz_bat_remove(struct platform_device *dev)
+{
+	int i;
+
+	device_remove_file(&dev->dev, &dev_attr_limit);
+	power_supply_unregister(&spitz_bat_main.psy);
+	power_supply_unregister(&spitz_ac.psy);
+	return 0;
+}
+
+
+static struct platform_driver spitz_bat_driver = {
+	.driver.name	= "spitz-battery",
+	.driver.owner	= THIS_MODULE,
+	.probe		= spitz_bat_probe,
+	.remove		= __devexit_p(spitz_bat_remove),
+	.suspend	= spitz_bat_suspend,
+	.resume		= spitz_bat_resume,
+};
+
+static int __init spitz_bat_init(void)
+{
+	return platform_driver_register(&spitz_bat_driver);
+}
+
+static void __exit spitz_bat_exit(void)
+{
+	platform_driver_unregister(&spitz_bat_driver);
+}
+
+module_init(spitz_bat_init);
+module_exit(spitz_bat_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Pavel Machek");
+MODULE_DESCRIPTION("Spitz battery driver");
+MODULE_ALIAS("platform:spitz-battery");
diff -ur ./drivers/video/backlight.ofic/corgi_lcd.c ./drivers/video/backlight/corgi_lcd.c
--- ./drivers/video/backlight.ofic/corgi_lcd.c	2009-10-18 18:11:36.000000000 +0200
+++ ./drivers/video/backlight/corgi_lcd.c	2009-10-18 18:37:09.000000000 +0200
@@ -388,17 +388,33 @@
 	.set_mode	= corgi_lcd_set_mode,
 };
 
-static int corgi_bl_get_intensity(struct backlight_device *bd)
+int corgi_bl_get_intensity(struct backlight_device *bd)
 {
 	struct corgi_lcd *lcd = dev_get_drvdata(&bd->dev);
 
 	return lcd->intensity;
 }
+EXPORT_SYMBOL(corgi_bl_get_intensity);
+
+int backlight_current;
+EXPORT_SYMBOL(backlight_current);
 
 static int corgi_bl_set_intensity(struct corgi_lcd *lcd, int intensity)
 {
 	int cont;
 
+	backlight_current = 0;
+	if (intensity > 0)
+		backlight_current = 55;
+	if (intensity > 10)
+		backlight_current = 115;
+	if (intensity > 20)
+		backlight_current = 140;
+	if (intensity > 30)
+		backlight_current = 180;
+	if (intensity > 45)
+		backlight_current = 245;
+
 	if (intensity > 0x10)
 		intensity += 0x10;
 
@@ -433,8 +449,9 @@
 
 	if (corgibl_flags & CORGIBL_SUSPENDED)
 		intensity = 0;
-	if (corgibl_flags & CORGIBL_BATTLOW)
-		intensity &= lcd->limit_mask;
+
+	if ((corgibl_flags & CORGIBL_BATTLOW) && intensity > lcd->limit_mask)
+		intensity = lcd->limit_mask;
 
 	return corgi_bl_set_intensity(lcd, intensity);
 }
diff -ur ./arch/arm.ofic/mach-pxa/corgi_pm.c ./arch/arm/mach-pxa/corgi_pm.c
--- ./arch/arm.ofic/mach-pxa/corgi_pm.c	2009-09-10 00:13:59.000000000 +0200
+++ ./arch/arm/mach-pxa/corgi_pm.c	2009-10-22 19:19:02.000000000 +0200
@@ -35,6 +35,92 @@
 #define SHARPSL_FATAL_ACIN_VOLT        182   /* 3.45V */
 #define SHARPSL_FATAL_NOACIN_VOLT      170   /* 3.40V */
 
+static const struct battery_thresh corgi_battery_levels_acin[] = {
+	{ 213, 100},
+	{ 212,  98},
+	{ 211,  95},
+	{ 210,  93},
+	{ 209,  90},
+	{ 208,  88},
+	{ 207,  85},
+	{ 206,  83},
+	{ 205,  80},
+	{ 204,  78},
+	{ 203,  75},
+	{ 202,  73},
+	{ 201,  70},
+	{ 200,  68},
+	{ 199,  65},
+	{ 198,  63},
+	{ 197,  60},
+	{ 196,  58},
+	{ 195,  55},
+	{ 194,  53},
+	{ 193,  50},
+	{ 192,  48},
+	{ 192,  45},
+	{ 191,  43},
+	{ 191,  40},
+	{ 190,  38},
+	{ 190,  35},
+	{ 189,  33},
+	{ 188,  30},
+	{ 187,  28},
+	{ 186,  25},
+	{ 185,  23},
+	{ 184,  20},
+	{ 183,  18},
+	{ 182,  15},
+	{ 181,  13},
+	{ 180,  10},
+	{ 179,   8},
+	{ 178,   5},
+	{   0,   0},
+};
+
+static const struct battery_thresh  corgi_battery_levels_noac[] = {
+	{ 213, 100},
+	{ 212,  98},
+	{ 211,  95},
+	{ 210,  93},
+	{ 209,  90},
+	{ 208,  88},
+	{ 207,  85},
+	{ 206,  83},
+	{ 205,  80},
+	{ 204,  78},
+	{ 203,  75},
+	{ 202,  73},
+	{ 201,  70},
+	{ 200,  68},
+	{ 199,  65},
+	{ 198,  63},
+	{ 197,  60},
+	{ 196,  58},
+	{ 195,  55},
+	{ 194,  53},
+	{ 193,  50},
+	{ 192,  48},
+	{ 191,  45},
+	{ 190,  43},
+	{ 189,  40},
+	{ 188,  38},
+	{ 187,  35},
+	{ 186,  33},
+	{ 185,  30},
+	{ 184,  28},
+	{ 183,  25},
+	{ 182,  23},
+	{ 181,  20},
+	{ 180,  18},
+	{ 179,  15},
+	{ 178,  13},
+	{ 177,  10},
+	{ 176,   8},
+	{ 175,   5},
+	{   0,   0},
+};
+
 static void corgi_charger_init(void)
 {
 	pxa_gpio_mode(CORGI_GPIO_ADC_TEMP_ON | GPIO_OUT);
@@ -214,8 +300,8 @@
 	.fatal_acin_volt  = SHARPSL_FATAL_ACIN_VOLT,
 	.fatal_noacin_volt= SHARPSL_FATAL_NOACIN_VOLT,
 	.bat_levels       = 40,
-	.bat_levels_noac  = spitz_battery_levels_noac,
-	.bat_levels_acin  = spitz_battery_levels_acin,
+	.bat_levels_noac  = corgi_battery_levels_noac,
+	.bat_levels_acin  = corgi_battery_levels_acin,
 	.status_high_acin = 188,
 	.status_low_acin  = 178,
 	.status_high_noac = 185,
diff -ur ./arch/arm.ofic/mach-pxa/sharpsl.h ./arch/arm/mach-pxa/sharpsl.h
--- ./arch/arm.ofic/mach-pxa/sharpsl.h	2009-09-10 00:13:59.000000000 +0200
+++ ./arch/arm/mach-pxa/sharpsl.h	2009-10-14 12:12:13.000000000 +0200
@@ -42,8 +42,23 @@
 #define MAX1111_BATT_TEMP   2u
 #define MAX1111_ACIN_VOLT   6u
 
-extern struct battery_thresh spitz_battery_levels_acin[];
-extern struct battery_thresh spitz_battery_levels_noac[];
 int sharpsl_pm_pxa_read_max1111(int channel);
 
 
+/*
+ * Constants
+ */
+#define SHARPSL_CHARGE_ON_TIME_INTERVAL        (msecs_to_jiffies(1*60*1000))  /* 1 min */
+#define SHARPSL_CHARGE_FINISH_TIME             (msecs_to_jiffies(10*60*1000)) /* 10 min */
+#define SHARPSL_BATCHK_TIME                    (msecs_to_jiffies(15*1000))    /* 15 sec */
+#define SHARPSL_BATCHK_TIME_SUSPEND            (60*10)                        /* 10 min */
+
+#define SHARPSL_WAIT_CO_TIME                   15  /* 15 sec */
+#define SHARPSL_WAIT_DISCHARGE_ON              100 /* 100 msec */
+#define SHARPSL_CHECK_BATTERY_WAIT_TIME_TEMP   10  /* 10 msec */
+#define SHARPSL_CHECK_BATTERY_WAIT_TIME_VOLT   10  /* 10 msec */
+#define SHARPSL_CHECK_BATTERY_WAIT_TIME_ACIN   10  /* 10 msec */
+#define SHARPSL_CHARGE_WAIT_TIME               15  /* 15 msec */
+#define SHARPSL_CHARGE_CO_CHECK_TIME           5   /* 5 msec */
+#define SHARPSL_CHARGE_RETRY_CNT               1   /* eqv. 10 min */
+
Only in ./arch/arm/mach-pxa: sharpsl.h~
diff -ur ./arch/arm.ofic/mach-pxa/sharpsl_pm.c ./arch/arm/mach-pxa/sharpsl_pm.c
--- ./arch/arm.ofic/mach-pxa/sharpsl_pm.c	2009-10-06 13:48:07.000000000 +0200
+++ ./arch/arm/mach-pxa/sharpsl_pm.c	2009-10-14 12:12:05.000000000 +0200
@@ -32,25 +32,10 @@
 #include <mach/regs-rtc.h>
 #include <mach/sharpsl.h>
 #include <mach/sharpsl_pm.h>
+#include <mach/spitz.h>
 
 #include "sharpsl.h"
 
-/*
- * Constants
- */
-#define SHARPSL_CHARGE_ON_TIME_INTERVAL        (msecs_to_jiffies(1*60*1000))  /* 1 min */
-#define SHARPSL_CHARGE_FINISH_TIME             (msecs_to_jiffies(10*60*1000)) /* 10 min */
-#define SHARPSL_BATCHK_TIME                    (msecs_to_jiffies(15*1000))    /* 15 sec */
-#define SHARPSL_BATCHK_TIME_SUSPEND            (60*10)                        /* 10 min */
-
-#define SHARPSL_WAIT_CO_TIME                   15  /* 15 sec */
-#define SHARPSL_WAIT_DISCHARGE_ON              100 /* 100 msec */
-#define SHARPSL_CHECK_BATTERY_WAIT_TIME_TEMP   10  /* 10 msec */
-#define SHARPSL_CHECK_BATTERY_WAIT_TIME_VOLT   10  /* 10 msec */
-#define SHARPSL_CHECK_BATTERY_WAIT_TIME_ACIN   10  /* 10 msec */
-#define SHARPSL_CHARGE_WAIT_TIME               15  /* 15 msec */
-#define SHARPSL_CHARGE_CO_CHECK_TIME           5   /* 5 msec */
-#define SHARPSL_CHARGE_RETRY_CNT               1   /* eqv. 10 min */
 
 /*
  * Prototypes
@@ -72,112 +57,28 @@
  * Variables
  */
 struct sharpsl_pm_status sharpsl_pm;
+EXPORT_SYMBOL(sharpsl_pm);
 static DECLARE_DELAYED_WORK(toggle_charger, sharpsl_charge_toggle);
 static DECLARE_DELAYED_WORK(sharpsl_bat, sharpsl_battery_thread);
 DEFINE_LED_TRIGGER(sharpsl_charge_led_trigger);
 
 
 
-struct battery_thresh spitz_battery_levels_acin[] = {
-	{ 213, 100},
-	{ 212,  98},
-	{ 211,  95},
-	{ 210,  93},
-	{ 209,  90},
-	{ 208,  88},
-	{ 207,  85},
-	{ 206,  83},
-	{ 205,  80},
-	{ 204,  78},
-	{ 203,  75},
-	{ 202,  73},
-	{ 201,  70},
-	{ 200,  68},
-	{ 199,  65},
-	{ 198,  63},
-	{ 197,  60},
-	{ 196,  58},
-	{ 195,  55},
-	{ 194,  53},
-	{ 193,  50},
-	{ 192,  48},
-	{ 192,  45},
-	{ 191,  43},
-	{ 191,  40},
-	{ 190,  38},
-	{ 190,  35},
-	{ 189,  33},
-	{ 188,  30},
-	{ 187,  28},
-	{ 186,  25},
-	{ 185,  23},
-	{ 184,  20},
-	{ 183,  18},
-	{ 182,  15},
-	{ 181,  13},
-	{ 180,  10},
-	{ 179,   8},
-	{ 178,   5},
-	{   0,   0},
-};
-
-struct battery_thresh  spitz_battery_levels_noac[] = {
-	{ 213, 100},
-	{ 212,  98},
-	{ 211,  95},
-	{ 210,  93},
-	{ 209,  90},
-	{ 208,  88},
-	{ 207,  85},
-	{ 206,  83},
-	{ 205,  80},
-	{ 204,  78},
-	{ 203,  75},
-	{ 202,  73},
-	{ 201,  70},
-	{ 200,  68},
-	{ 199,  65},
-	{ 198,  63},
-	{ 197,  60},
-	{ 196,  58},
-	{ 195,  55},
-	{ 194,  53},
-	{ 193,  50},
-	{ 192,  48},
-	{ 191,  45},
-	{ 190,  43},
-	{ 189,  40},
-	{ 188,  38},
-	{ 187,  35},
-	{ 186,  33},
-	{ 185,  30},
-	{ 184,  28},
-	{ 183,  25},
-	{ 182,  23},
-	{ 181,  20},
-	{ 180,  18},
-	{ 179,  15},
-	{ 178,  13},
-	{ 177,  10},
-	{ 176,   8},
-	{ 175,   5},
-	{   0,   0},
-};
-
 /* MAX1111 Commands */
-#define MAXCTRL_PD0      1u << 0
-#define MAXCTRL_PD1      1u << 1
-#define MAXCTRL_SGL      1u << 2
-#define MAXCTRL_UNI      1u << 3
+#define MAXCTRL_PD0      (1u << 0)
+#define MAXCTRL_PD1      (1u << 1)
+#define MAXCTRL_SGL      (1u << 2)
+#define MAXCTRL_UNI      (1u << 3)
 #define MAXCTRL_SEL_SH   4
-#define MAXCTRL_STR      1u << 7
+#define MAXCTRL_STR      (1u << 7)
 
 /*
  * Read MAX1111 ADC
  */
 int sharpsl_pm_pxa_read_max1111(int channel)
 {
-	if (machine_is_tosa()) // Ugly, better move this function into another module
+	/* Ugly, better move this function into another module */
+	if (machine_is_tosa())
 	    return 0;
 
 #ifdef CONFIG_CORGI_SSP_DEPRECATED
@@ -193,7 +94,7 @@
 #endif
 }
 
-static int get_percentage(int voltage)
+int get_percentage(int voltage)
 {
 	int i = sharpsl_pm.machinfo->bat_levels - 1;
 	int bl_status = sharpsl_pm.machinfo->backlight_get_status ? sharpsl_pm.machinfo->backlight_get_status() : 0;
@@ -209,6 +110,7 @@
 
 	return thresh[i].percentage;
 }
+EXPORT_SYMBOL(get_percentage);
 
 static int get_apm_status(int voltage)
 {
@@ -238,7 +140,7 @@
 
 static void sharpsl_battery_thread(struct work_struct *private_)
 {
-	int voltage, percent, apm_status, i = 0;
+	int voltage, percent, apm_status, i;
 
 	if (!sharpsl_pm.machinfo)
 		return;
@@ -250,15 +152,14 @@
 			&& time_after(jiffies, sharpsl_pm.charge_start_time +  SHARPSL_CHARGE_ON_TIME_INTERVAL))
 		schedule_delayed_work(&toggle_charger, 0);
 
-	while(1) {
+	for (i = 0; i < 5; i++) {
 		voltage = sharpsl_pm.machinfo->read_devdata(SHARPSL_BATT_VOLT);
-
-		if (voltage > 0) break;
-		if (i++ > 5) {
-			voltage = sharpsl_pm.machinfo->bat_levels_noac[0].voltage;
-			dev_warn(sharpsl_pm.dev, "Warning: Cannot read main battery!\n");
+		if (voltage > 0)
 			break;
-		}
+	}
+	if (voltage <= 0) {
+		voltage = sharpsl_pm.machinfo->bat_levels_noac[0].voltage;
+		dev_warn(sharpsl_pm.dev, "Warning: Cannot read main battery!\n");
 	}
 
 	voltage = sharpsl_average_value(voltage);
@@ -266,8 +167,10 @@
 	percent = get_percentage(voltage);
 
 	/* At low battery voltages, the voltage has a tendency to start
-           creeping back up so we try to avoid this here */
-	if ((sharpsl_pm.battstat.ac_status == APM_AC_ONLINE) || (apm_status == APM_BATTERY_STATUS_HIGH) ||  percent <= sharpsl_pm.battstat.mainbat_percent) {
+	   creeping back up so we try to avoid this here */
+	if ((sharpsl_pm.battstat.ac_status == APM_AC_ONLINE)
+	    || (apm_status == APM_BATTERY_STATUS_HIGH)
+	    || percent <= sharpsl_pm.battstat.mainbat_percent) {
 		sharpsl_pm.battstat.mainbat_voltage = voltage;
 		sharpsl_pm.battstat.mainbat_status = apm_status;
 		sharpsl_pm.battstat.mainbat_percent = percent;
@@ -279,8 +182,8 @@
 #ifdef CONFIG_BACKLIGHT_CORGI
 	/* If battery is low. limit backlight intensity to save power. */
 	if ((sharpsl_pm.battstat.ac_status != APM_AC_ONLINE)
-			&& ((sharpsl_pm.battstat.mainbat_status == APM_BATTERY_STATUS_LOW) ||
-			(sharpsl_pm.battstat.mainbat_status == APM_BATTERY_STATUS_CRITICAL))) {
+	    && ((sharpsl_pm.battstat.mainbat_status == APM_BATTERY_STATUS_LOW)
+	    || (sharpsl_pm.battstat.mainbat_status == APM_BATTERY_STATUS_CRITICAL))) {
 		if (!(sharpsl_pm.flags & SHARPSL_BL_LIMIT)) {
 			sharpsl_pm.machinfo->backlight_limit(1);
 			sharpsl_pm.flags |= SHARPSL_BL_LIMIT;
@@ -293,8 +196,8 @@
 
 	/* Suspend if critical battery level */
 	if ((sharpsl_pm.battstat.ac_status != APM_AC_ONLINE)
-			&& (sharpsl_pm.battstat.mainbat_status == APM_BATTERY_STATUS_CRITICAL)
-			&& !(sharpsl_pm.flags & SHARPSL_APM_QUEUED)) {
+	     && (sharpsl_pm.battstat.mainbat_status == APM_BATTERY_STATUS_CRITICAL)
+	     && !(sharpsl_pm.flags & SHARPSL_APM_QUEUED)) {
 		sharpsl_pm.flags |= SHARPSL_APM_QUEUED;
 		dev_err(sharpsl_pm.dev, "Fatal Off\n");
 		apm_queue_event(APM_CRITICAL_SUSPEND);
@@ -339,6 +242,8 @@
 
 static void sharpsl_charge_error(void)
 {
+	dev_warn(sharpsl_pm.dev, "Charger Error\n");
+
 	sharpsl_pm_led(SHARPSL_LED_ERROR);
 	sharpsl_pm.machinfo->charge(0);
 	sharpsl_pm.charge_mode = CHRG_ERROR;
@@ -346,7 +251,7 @@
 
 static void sharpsl_charge_toggle(struct work_struct *private_)
 {
-	dev_dbg(sharpsl_pm.dev, "Toogling Charger at time: %lx\n", jiffies);
+	dev_dbg(sharpsl_pm.dev, "Toggling Charger at time: %lx\n", jiffies);
 
 	if (!sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_ACIN)) {
 		sharpsl_charge_off();
@@ -368,7 +273,7 @@
 {
 	int acin = sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_ACIN);
 
-	dev_dbg(sharpsl_pm.dev, "AC Status: %d\n",acin);
+	dev_dbg(sharpsl_pm.dev, "AC Status: %d\n", acin);
 
 	sharpsl_average_clear();
 	if (acin && (sharpsl_pm.charge_mode != CHRG_ON))
@@ -472,14 +377,14 @@
 	sharpsl_ad[sharpsl_ad_index] = ad;
 	sharpsl_ad_index++;
 	if (sharpsl_ad_index >= SHARPSL_CNV_VALUE_NUM) {
-		for (i=0; i < (SHARPSL_CNV_VALUE_NUM-1); i++)
+		for (i = 0; i < (SHARPSL_CNV_VALUE_NUM-1); i++)
 			sharpsl_ad[i] = sharpsl_ad[i+1];
 		sharpsl_ad_index = SHARPSL_CNV_VALUE_NUM - 1;
 	}
-	for (i=0; i < sharpsl_ad_index; i++)
+	for (i = 0; i < sharpsl_ad_index; i++)
 		ad_val += sharpsl_ad[i];
 
-	return (ad_val / sharpsl_ad_index);
+	return ad_val / sharpsl_ad_index;
 }
 
 /*
@@ -492,8 +397,8 @@
 
 	/* Find MAX val */
 	temp = val[0];
-	j=0;
-	for (i=1; i<5; i++) {
+	j = 0;
+	for (i = 1; i < 5; i++) {
 		if (temp < val[i]) {
 			temp = val[i];
 			j = i;
@@ -502,21 +407,21 @@
 
 	/* Find MIN val */
 	temp = val[4];
-	k=4;
-	for (i=3; i>=0; i--) {
+	k = 4;
+	for (i = 3; i >= 0; i--) {
 		if (temp > val[i]) {
 			temp = val[i];
 			k = i;
 		}
 	}
 
-	for (i=0; i<5; i++)
-		if (i != j && i != k )
+	for (i = 0; i < 5; i++)
+		if (i != j && i != k)
 			sum += val[i];
 
 	dev_dbg(sharpsl_pm.dev, "Average: %d from values: %d, %d, %d, %d, %d\n", sum/3, val[0], val[1], val[2], val[3], val[4]);
 
-	return (sum/3);
+	return sum/3;
 }
 
 static int sharpsl_check_battery_temp(void)
@@ -524,7 +429,7 @@
 	int val, i, buff[5];
 
 	/* Check battery temperature */
-	for (i=0; i<5; i++) {
+	for (i = 0; i < 5; i++) {
 		mdelay(SHARPSL_CHECK_BATTERY_WAIT_TIME_TEMP);
 		sharpsl_pm.machinfo->measure_temp(1);
 		mdelay(SHARPSL_CHECK_BATTERY_WAIT_TIME_TEMP);
@@ -535,8 +440,10 @@
 	val = get_select_val(buff);
 
 	dev_dbg(sharpsl_pm.dev, "Temperature: %d\n", val);
-	if (val > sharpsl_pm.machinfo->charge_on_temp) {
-		printk(KERN_WARNING "Not charging: temperature out of limits.\n");
+	/* FIXME: this should catch battery read errors, but we should
+	   probably avoid charging in <0C temperatures, too. */
+	if ((val < 0) || (val > sharpsl_pm.machinfo->charge_on_temp)) {
+		dev_warn(sharpsl_pm.dev, "Not charging: temperature %d out of limits.\n", val);
 		return -1;
 	}
 
@@ -557,7 +464,7 @@
 		sharpsl_pm.machinfo->discharge1(1);
 
 	/* Check battery voltage */
-	for (i=0; i<5; i++) {
+	for (i = 0; i < 5; i++) {
 		buff[i] = sharpsl_pm.machinfo->read_devdata(SHARPSL_BATT_VOLT);
 		mdelay(SHARPSL_CHECK_BATTERY_WAIT_TIME_VOLT);
 	}
@@ -581,16 +488,16 @@
 {
 	int temp, i, buff[5];
 
-	for (i=0; i<5; i++) {
+	for (i = 0; i < 5; i++) {
 		buff[i] = sharpsl_pm.machinfo->read_devdata(SHARPSL_ACIN_VOLT);
 		mdelay(SHARPSL_CHECK_BATTERY_WAIT_TIME_ACIN);
 	}
 
 	temp = get_select_val(buff);
-	dev_dbg(sharpsl_pm.dev, "AC Voltage: %d\n",temp);
+	dev_dbg(sharpsl_pm.dev, "AC Voltage: %d\n", temp);
 
 	if ((temp > sharpsl_pm.machinfo->charge_acin_high) || (temp < sharpsl_pm.machinfo->charge_acin_low)) {
-		dev_err(sharpsl_pm.dev, "Error: AC check failed.\n");
+		dev_err(sharpsl_pm.dev, "Error: AC check failed: voltage %d.\n", temp);
 		return -1;
 	}
 
@@ -624,9 +531,9 @@
 
 static void corgi_goto_sleep(unsigned long alarm_time, unsigned int alarm_enable, suspend_state_t state)
 {
-	dev_dbg(sharpsl_pm.dev, "Time is: %08x\n",RCNR);
+	dev_dbg(sharpsl_pm.dev, "Time is: %08x\n", RCNR);
 
-	dev_dbg(sharpsl_pm.dev, "Offline Charge Activate = %d\n",sharpsl_pm.flags & SHARPSL_DO_OFFLINE_CHRG);
+	dev_dbg(sharpsl_pm.dev, "Offline Charge Activate = %d\n", sharpsl_pm.flags & SHARPSL_DO_OFFLINE_CHRG);
 	/* not charging and AC-IN! */
 
 	if ((sharpsl_pm.flags & SHARPSL_DO_OFFLINE_CHRG) && (sharpsl_pm.machinfo->read_devdata(SHARPSL_STATUS_ACIN))) {
@@ -644,12 +551,12 @@
 	if ((sharpsl_pm.charge_mode == CHRG_ON) && ((alarm_enable && ((alarm_time - RCNR) > (SHARPSL_BATCHK_TIME_SUSPEND + 30))) || !alarm_enable)) {
 		RTSR &= RTSR_ALE;
 		RTAR = RCNR + SHARPSL_BATCHK_TIME_SUSPEND;
-		dev_dbg(sharpsl_pm.dev, "Charging alarm at: %08x\n",RTAR);
+		dev_dbg(sharpsl_pm.dev, "Charging alarm at: %08x\n", RTAR);
 		sharpsl_pm.flags |= SHARPSL_ALARM_ACTIVE;
 	} else if (alarm_enable) {
 		RTSR &= RTSR_ALE;
 		RTAR = alarm_time;
-		dev_dbg(sharpsl_pm.dev, "User alarm at: %08x\n",RTAR);
+		dev_dbg(sharpsl_pm.dev, "User alarm at: %08x\n", RTAR);
 	} else {
 		dev_dbg(sharpsl_pm.dev, "No alarms set.\n");
 	}
@@ -658,19 +565,20 @@
 
 	sharpsl_pm.machinfo->postsuspend();
 
-	dev_dbg(sharpsl_pm.dev, "Corgi woken up from suspend: %08x\n",PEDR);
+
+	dev_dbg(sharpsl_pm.dev, "Corgi woken up from suspend: %08x\n", PEDR);
 }
 
 static int corgi_enter_suspend(unsigned long alarm_time, unsigned int alarm_enable, suspend_state_t state)
 {
-	if (!sharpsl_pm.machinfo->should_wakeup(!(sharpsl_pm.flags & SHARPSL_ALARM_ACTIVE) && alarm_enable) )
-	{
+  return 0;
+	if (!sharpsl_pm.machinfo->should_wakeup(!(sharpsl_pm.flags & SHARPSL_ALARM_ACTIVE) && alarm_enable)) {
 		if (!(sharpsl_pm.flags & SHARPSL_ALARM_ACTIVE)) {
 			dev_dbg(sharpsl_pm.dev, "No user triggered wakeup events and not charging. Strange. Suspend.\n");
 			corgi_goto_sleep(alarm_time, alarm_enable, state);
 			return 1;
 		}
-		if(sharpsl_off_charge_battery()) {
+		if (sharpsl_off_charge_battery()) {
 			dev_dbg(sharpsl_pm.dev, "Charging. Suspend...\n");
 			corgi_goto_sleep(alarm_time, alarm_enable, state);
 			return 1;
@@ -697,7 +605,7 @@
 
 	corgi_goto_sleep(alarm_time, alarm_status, state);
 
-	while (corgi_enter_suspend(alarm_time,alarm_status,state))
+	while (corgi_enter_suspend(alarm_time, alarm_status, state))
 		{}
 
 	if (sharpsl_pm.machinfo->earlyresume)
@@ -732,7 +640,7 @@
 		sharpsl_pm.machinfo->discharge1(1);
 
 	/* Check battery : check inserting battery ? */
-	for (i=0; i<5; i++) {
+	for (i = 0; i < 5; i++) {
 		buff[i] = sharpsl_pm.machinfo->read_devdata(SHARPSL_BATT_VOLT);
 		mdelay(SHARPSL_CHECK_BATTERY_WAIT_TIME_VOLT);
 	}
@@ -812,7 +720,7 @@
 		mdelay(SHARPSL_CHARGE_CO_CHECK_TIME);
 
 		time = RCNR;
-		while(1) {
+		while (1) {
 			/* Check if any wakeup event had occurred */
 			if (sharpsl_pm.machinfo->charger_wakeup() != 0)
 				return 0;
@@ -835,9 +743,9 @@
 	mdelay(SHARPSL_CHARGE_CO_CHECK_TIME);
 
 	time = RCNR;
-	while(1) {
+	while (1) {
 		/* Check if any wakeup event had occurred */
-		if (sharpsl_pm.machinfo->charger_wakeup() != 0)
+		if (sharpsl_pm.machinfo->charger_wakeup())
 			return 0;
 		/* Check for timeout */
 		if ((RCNR-time) > SHARPSL_WAIT_CO_TIME) {
@@ -864,12 +772,12 @@
 
 static ssize_t battery_percentage_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n",sharpsl_pm.battstat.mainbat_percent);
+	return sprintf(buf, "%d\n", sharpsl_pm.battstat.mainbat_percent);
 }
 
 static ssize_t battery_voltage_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n",sharpsl_pm.battstat.mainbat_voltage);
+	return sprintf(buf, "%d\n", sharpsl_pm.battstat.mainbat_voltage);
 }
 
 static DEVICE_ATTR(battery_percentage, 0444, battery_percentage_show, NULL);
@@ -943,7 +851,6 @@
 		}
 	}
 
-	if (sharpsl_pm.machinfo->batfull_irq)
-	{
+	if (sharpsl_pm.machinfo->batfull_irq) {
 		/* Register interrupt handler. */
 		if (request_irq(IRQ_GPIO(sharpsl_pm.machinfo->gpio_batfull), sharpsl_chrg_full_isr, IRQF_DISABLED | IRQF_TRIGGER_RISING, "CO", sharpsl_chrg_full_isr)) {
diff -ur ./arch/arm.ofic/mach-pxa/spitz.c ./arch/arm/mach-pxa/spitz.c
--- ./arch/arm.ofic/mach-pxa/spitz.c	2009-10-06 13:48:07.000000000 +0200
+++ ./arch/arm/mach-pxa/spitz.c	2009-10-14 11:01:36.000000000 +0200
@@ -686,12 +733,19 @@
 	.dev.platform_data = &sharpsl_rom_data,
 };
 
+static struct platform_device spitz_battery_device = {
+	.name	= "spitz-battery",
+	.id	= -1,
+};
+
+
 static struct platform_device *devices[] __initdata = {
 	&spitzscoop_device,
 	&spitzkbd_device,
 	&spitzled_device,
 	&sharpsl_nand_device,
 	&sharpsl_rom_device,
+	&spitz_battery_device,
 };
 
 static void spitz_poweroff(void)
--- ./arch/arm.ofic/mach-pxa/spitz_pm.c	2009-09-10 00:13:59.000000000 +0200
+++ ./arch/arm/mach-pxa/spitz_pm.c	2009-10-19 07:28:42.000000000 +0200
@@ -37,6 +37,93 @@
 
 static int spitz_last_ac_status;
 
+static const struct battery_thresh spitz_battery_levels_acin[] = {
+	{ 213, 100},
+	{ 212,  98},
+	{ 211,  95},
+	{ 210,  93},
+	{ 209,  90},
+	{ 208,  88},
+	{ 207,  85},
+	{ 206,  83},
+	{ 205,  80},
+	{ 204,  78},
+	{ 203,  75},
+	{ 202,  73},
+	{ 201,  70},
+	{ 200,  68},
+	{ 199,  65},
+	{ 198,  63},
+	{ 197,  60},
+	{ 196,  58},
+	{ 195,  55},
+	{ 194,  53},
+	{ 193,  50},
+	{ 192,  48},
+	{ 192,  45},
+	{ 191,  43},
+	{ 191,  40},
+	{ 190,  38},
+	{ 190,  35},
+	{ 189,  33},
+	{ 188,  30},
+	{ 187,  28},
+	{ 186,  25},
+	{ 185,  23},
+	{ 184,  20},
+	{ 183,  18},
+	{ 182,  15},
+	{ 181,  13},
+	{ 180,  10},
+	{ 179,   8},
+	{ 178,   5},
+	{   0,   0},
+};
+
+static const struct battery_thresh  spitz_battery_levels_noac[] = {
+	{ 213, 100},
+	{ 212,  98},
+	{ 211,  95},
+	{ 210,  93},
+	{ 209,  90},
+	{ 208,  88},
+	{ 207,  85},
+	{ 206,  83},
+	{ 205,  80},
+	{ 204,  78},
+	{ 203,  75},
+	{ 202,  73},
+	{ 201,  70},
+	{ 200,  68},
+	{ 199,  65},
+	{ 198,  63},
+	{ 197,  60},
+	{ 196,  58},
+	{ 195,  55},
+	{ 194,  53},
+	{ 193,  50},
+	{ 192,  48},
+	{ 191,  45},
+	{ 190,  43},
+	{ 189,  40},
+	{ 188,  38},
+	{ 187,  35},
+	{ 186,  33},
+	{ 185,  30},
+	{ 184,  28},
+	{ 183,  25},
+	{ 182,  23},
+	{ 181,  20},
+	{ 180,  18},
+	{ 179,  15},
+	{ 178,  13},
+	{ 177,  10},
+	{ 176,   8},
+	{ 175,   5},
+	{   0,   0},
+};
+
+
 static void spitz_charger_init(void)
 {
 	pxa_gpio_mode(SPITZ_GPIO_KEY_INT | GPIO_IN);
@@ -103,7 +190,7 @@
 	PFER = GPIO_bit(SPITZ_GPIO_KEY_INT) | GPIO_bit(SPITZ_GPIO_RESET);
 	PWER = GPIO_bit(SPITZ_GPIO_KEY_INT) | GPIO_bit(SPITZ_GPIO_RESET) | PWER_RTC;
 	PKWR = GPIO_bit(SPITZ_GPIO_SYNC) | GPIO_bit(SPITZ_GPIO_KEY_INT) | GPIO_bit(SPITZ_GPIO_RESET);
-	PKSR = 0xffffffff; // clear
+	PKSR = 0xffffffff; /* clear */
 
 	/* nRESET_OUT Disable */
 	PSLR |= PSLR_SL_ROD;
@@ -149,7 +236,7 @@
 	if (resume_on_alarm && (PEDR & PWER_RTC))
 		is_resume |= PWER_RTC;
 
-	dev_dbg(sharpsl_pm.dev, "is_resume: %x\n",is_resume);
+	dev_dbg(sharpsl_pm.dev, "is_resume: %x\n", is_resume);
 	return is_resume;
 }
 
@@ -160,15 +247,15 @@
 
 unsigned long spitzpm_read_devdata(int type)
 {
-	switch(type) {
+	switch (type) {
 	case SHARPSL_STATUS_ACIN:
 		return (((~GPLR(SPITZ_GPIO_AC_IN)) & GPIO_bit(SPITZ_GPIO_AC_IN)) != 0);
 	case SHARPSL_STATUS_LOCK:
-		return READ_GPIO_BIT(sharpsl_pm.machinfo->gpio_batlock);
+		return !!READ_GPIO_BIT(sharpsl_pm.machinfo->gpio_batlock);
 	case SHARPSL_STATUS_CHRGFULL:
-		return READ_GPIO_BIT(sharpsl_pm.machinfo->gpio_batfull);
+		return !!READ_GPIO_BIT(sharpsl_pm.machinfo->gpio_batfull);
 	case SHARPSL_STATUS_FATAL:
-		return READ_GPIO_BIT(sharpsl_pm.machinfo->gpio_fatal);
+		return !!READ_GPIO_BIT(sharpsl_pm.machinfo->gpio_fatal);
 	case SHARPSL_ACIN_VOLT:
 		return sharpsl_pm_pxa_read_max1111(MAX1111_ACIN_VOLT);
 	case SHARPSL_BATT_TEMP:
@@ -179,6 +266,11 @@
 	}
 }
 
+int backlight_get_status(void)
+{
+	return 0;
+}
+
 struct sharpsl_charger_machinfo spitz_pm_machinfo = {
 	.init             = spitz_charger_init,
 	.exit             = NULL,
@@ -199,8 +291,9 @@
 #if defined(CONFIG_LCD_CORGI)
 	.backlight_limit = corgi_lcd_limit_intensity,
 #elif defined(CONFIG_BACKLIGHT_CORGI)
-        .backlight_limit  = corgibl_limit_intensity,
+	.backlight_limit  = corgibl_limit_intensity,
 #endif
+//	.backlight_get_status = backlight_get_status,
 	.charge_on_volt	  = SHARPSL_CHARGE_ON_VOLT,
 	.charge_on_temp	  = SHARPSL_CHARGE_ON_TEMP,
 	.charge_acin_high = SHARPSL_CHARGE_ON_ACIN_HIGH,
@@ -241,7 +334,7 @@
 
 static void spitzpm_exit(void)
 {
- 	platform_device_unregister(spitzpm_device);
+	platform_device_unregister(spitzpm_device);
 }
 
 module_init(spitzpm_init);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

Hi!

> > > I wasn't aware of this.
> > > 
> > > That may be a good reason for adding kernel-based suspend notification,
> > > although I'd prefer ARM to notify the user space about the critical battery
> > > status allowing it to decide what to do.
> > 
> > Hard to do, without breaking compatibility that goes down to 2.4.X.
> 
> Sending a battery-critical notification to the user space is not equivalent to
> removing the existing kernel-based mechanism.  They can exist both at the
> same time if the notification is sent earlier than the kernel suspends
> everything. 

Yes, and obviously sending notification early is ok with me.

> > It really makes sense on zaurus. Those machines are simple, no
> > smartbattery and no embedded controller subsystems. Battery will not
> > protect itself, and its kernel job. (Should work on init=/bin/bash).
> > 
> > As power-off consumption is same as suspend power consumption (I
> > beleive zaurus simply does not have true power off), suspend on
> > critical makes some sense. (Note that it is set lower than on pcs, and
> > that we declare battery critical sooner than that.)
> 
> The problem with that is it catches at least some applications unprepared and
> notifying them that "we're suspending right now" doesn't really help, because
> they won't have any time to react anyway.

Agreed, but so what? On PC, machine would power off at that
point. That would surprise the apps, too.

Basically new enough userland should not make battery run low enough
for either emergency power off or emergency suspend.

IOW "nothing to see here, problem does not exist".
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 16 January 2010, Pavel Machek wrote:
> Hi!
> 
> > > > I wasn't aware of this.
> > > > 
> > > > That may be a good reason for adding kernel-based suspend notification,
> > > > although I'd prefer ARM to notify the user space about the critical battery
> > > > status allowing it to decide what to do.
> > > 
> > > Hard to do, without breaking compatibility that goes down to 2.4.X.
> > 
> > Sending a battery-critical notification to the user space is not equivalent to
> > removing the existing kernel-based mechanism.  They can exist both at the
> > same time if the notification is sent earlier than the kernel suspends
> > everything. 
> 
> Yes, and obviously sending notification early is ok with me.
> 
> > > It really makes sense on zaurus. Those machines are simple, no
> > > smartbattery and no embedded controller subsystems. Battery will not
> > > protect itself, and its kernel job. (Should work on init=/bin/bash).
> > > 
> > > As power-off consumption is same as suspend power consumption (I
> > > beleive zaurus simply does not have true power off), suspend on
> > > critical makes some sense. (Note that it is set lower than on pcs, and
> > > that we declare battery critical sooner than that.)
> > 
> > The problem with that is it catches at least some applications unprepared and
> > notifying them that "we're suspending right now" doesn't really help, because
> > they won't have any time to react anyway.
> 
> Agreed, but so what? On PC, machine would power off at that
> point. That would surprise the apps, too.
> 
> Basically new enough userland should not make battery run low enough
> for either emergency power off or emergency suspend.

I wonder how it is supposed to achieve that without knowing the current battery
status.  Do you mean it should poll the battery driver?

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Pavel Machek]

Hi!

> > Agreed, but so what? On PC, machine would power off at that
> > point. That would surprise the apps, too.
> > 
> > Basically new enough userland should not make battery run low enough
> > for either emergency power off or emergency suspend.
> 
> I wonder how it is supposed to achieve that without knowing the current battery
> status.  Do you mean it should poll the battery driver?

That's besides point, is it?

Usual kernel<->user interfaces should apply here. Zaurus provides
/proc/apm emulation, and I'm writing drivers/power/ driver. Not sure
it has event interface...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Saturday 16 January 2010, Pavel Machek wrote:
> Hi!
> 
> > > Agreed, but so what? On PC, machine would power off at that
> > > point. That would surprise the apps, too.
> > > 
> > > Basically new enough userland should not make battery run low enough
> > > for either emergency power off or emergency suspend.
> > 
> > I wonder how it is supposed to achieve that without knowing the current battery
> > status.  Do you mean it should poll the battery driver?
> 
> That's besides point, is it?

Well, mentioned that first. :-)

> Usual kernel<->user interfaces should apply here.

And we're talking about the design of these, aren't we?  The notification about
the critical battery status sent to the user space is a part of the kernel-user
space interface too, after all.

Rafael

Re: [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 2 stycznia 2010 19:26 	Bartłomiej Zimoń <uzi18@o2.pl> napisał(a):
> Dnia 2 stycznia 2010 16:56 	Daniel Borkmann <danborkmann@googlemail.com> napisał(a):
> > Hi Andy,
> > 
> > 2010/1/2 Andy Walls <awalls@radix.net>:
> > > Why not:
> > >
> > > a. write a module that implements a device node that supports poll(),
> > > and
> > >
> > > b. have a user space process select() on the fd for read or exception
> > > notification
> > >
> > > ?
> > 
> > This is, of course, another possible solution that is more "cleaner"
> > than the one with the signals.
> > Then, your userspace program would have another thread polling for the
> > device node. Question is which timeout would be appropriate to be "CPU
> > friendly" and to keep notification latency short?
> > 
> 
> Just need as fast as possible solution and on the other hand acceptable for kernel sources.
> Usually programs needs just to disconnect something or set one flag.
> Even if program will have no time for this it could be enough just to send this precious info.
> 

first draft: http://starowa.one.pl/~uzi/pld/pm-notify.c

# tail -f /var/log/kernel |grep pm-notify
Jan  2 21:37:19 topijna kernel: pm-notify: suspend prepare
Jan  2 21:38:18 topijna kernel: pm-notify: suspend prepare
Jan  2 21:39:05 topijna kernel: pm-notify: suspend prepare
Jan  2 21:43:38 topijna kernel: pm-notify: suspend prepare
Jan  2 21:51:31 topijna kernel: pm-notify: suspend prepare
Jan  2 21:53:59 topijna kernel: pm-notify: hibernation prepare

But dont know why have loop here:

$ hexdump /dev/test
0000000 0033 0000 0033 0000 0033 0000 0033 0000
*
31424480 0033 0000 0033 0000 0031 0000 0031 0000
31424490 0031 0000 0031 0000 0031 0000 0031 0000
*
$

Best regards.
Barłomiej Zimoń
PLD Linux, Kadu Team

[suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 2 stycznia 2010 15:04 	Daniel Borkmann <danborkmann@googlemail.com> napisał(a):
> 
> I guess netlink could be the "cleaner" solution, but with more overhead?!
> 

Could be, but remember we have small amount of time from pm notify to freeze processes.

Best Regards
Bartłomiej Zimoń
PLD Linux, Kadu Team

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Bartłomiej Zimoń]

Dnia 6 stycznia 2010 0:05 	"Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> Well, I think you have some specific issue with pm-utils, but you're not
> telling us what it is.  So, why exactly is pm-utils not suitable for your
> needs?
> 

Rafael to be honest it's more dbus specific. I'm talking about 
dbus as it is standard for system<->apps communication.

But please understand if we'll create kernel device with simple pm notification
It could be hooked by udev and next ... by DeviceKit-power/upower and after that
we have real standard. But to have this we must export "power" device - something 
like my proof-of-concept module. This could be realy good start.

Best regards.
Bartłomiej Zimoń
PLD Linux, Kadu Team, FreeRunner user
http://kadu-im.blogspot.com/

Re: [linux-pm] [suspend/resume] Re: userspace notification from module

[Rafael J. Wysocki]

On Wednesday 06 January 2010, Bartłomiej Zimoń wrote:
> Dnia 6 stycznia 2010 0:05 	"Rafael J. Wysocki" <rjw@sisk.pl> napisał(a):
> > Well, I think you have some specific issue with pm-utils, but you're not
> > telling us what it is.  So, why exactly is pm-utils not suitable for your
> > needs?
> > 
> 
> Rafael to be honest it's more dbus specific. I'm talking about 
> dbus as it is standard for system<->apps communication.

That was my impression. :-)

Well, I don't like dbus too, but ...

> But please understand if we'll create kernel device with simple pm notification
> It could be hooked by udev and next ... by DeviceKit-power/upower and after that
> we have real standard. But to have this we must export "power" device - something 
> like my proof-of-concept module. This could be realy good start.

... that doesn't make me think it should be sneakily replaced with "magic"
kernel modules or similar stuff.

As I already said many times in this thread before, please look for a solution
in the user space.

Rafael