Opened 7 years ago

Last modified 6 years ago

#6010 assigned defect

Export EC wakeup event mask to userspace

Reported by: cjb Owned by: dsaxena
Priority: high Milestone: 8.2.0 (was Update.2)
Component: kernel Version:
Keywords: Cc: pgf, dilinger
Blocked By: Blocking: #5974
Deployments affected: Action Needed:
Verified: no

Description

We currently have the "suspend" vs. "sleep" distinction made in userspace, which results in many temporary wakeups in order to see whether we're supposed to be woken up fully by an external event.

Instead, the kernel should expose a method for setting the EC wakeup mask to userspace, and OHM should set it appropriately before each type of suspend.

Change History (18)

comment:1 Changed 7 years ago by cjb

  • Blocking 5974 added

(In #5974) Let's have this be fixed by #6010, in Update.2. The Update.1 answer is that you should not press the power button while recording.

comment:2 Changed 6 years ago by cjb

  • Owner changed from dilinger to dsaxena

Andres, maybe I should reassign this one to deepak?

Deepak, we have an EC mask that controls which events will wake the machine up -- currently we allow (almost) all of them and then test what woke us up (via /sys/power/wakeup-source in olpc-pm.c) in userspace, but we should push the test down to the EC and have the EC decide based on our setting the mask.

So, we could use a sysfs/root-only way of setting the wakeup mask.

comment:3 Changed 6 years ago by dsaxena

I'm thinking something along the lines of having one file per event type that userspace can write to with 1|0 and the kernel will manage the mask. My understanding of sysfs usage is that writing binary data such as bit masks is frowned upon.

comment:4 follow-up: Changed 6 years ago by cjb

Seems a little obtuse, since it sounds like userspace (OHM) will end up storing a mask and then synthesizing it into write-one-file-at-a-time, but I'll take what I can get. :)

comment:5 in reply to: ↑ 4 Changed 6 years ago by dsaxena

Replying to cjb:

Seems a little obtuse, since it sounds like userspace (OHM) will end up storing a mask and then synthesizing it into write-one-file-at-a-time, but I'll take what I can get. :)

I agree, and my first thought was to go with that option too, but there are advantages to having it be one per file, such as being able to do quick tests via the command line. I also talked to Greg upstream and this is definitely the preferred method:

<dsaxena>       is my understanding correct in that binary data (a bitmask in my case) exported/imported via sysfs is frowned upon?
<gregkh>        dsaxena: why would you want to export binary data that way?
<gregkh>        dsaxena: what would userspace use it for?
<dsaxena>       well, let me explain the usage...
<gregkh>        that
<gregkh>        's a great introduction :)
<dsaxena>       on OLPC we can tell the system controller what events should trigger us to wake up out of suspend
<dsaxena>       this is done via setting a bit mask, with one bit per event
<dsaxena>       we want a way for OHM to tell the EC driver which events we want to ignore and which to actually wake up from
<dsaxena>       my first thought was have one file in sysfs per event
<dsaxena>       write 1|0 to enable|disable
<gregkh>        ok, that sounds fine.
<dsaxena>       but having a single file which just exporst the mask to userspace just seems nicer from userspace programmers point of view
<gregkh>        from a bash programmers point of view?
<gregkh>        and what defines those bits?
<dsaxena>       the hw spec
<gregkh>        if they are one per file, it's much easier to figure out without having to read a hw spec.
<dsaxena>       true true
<gregkh>        and you have a chance that they can be used by other arches.
<gregkh>        so i'd say one value per file, like the rest of sysfs :)
<dsaxena>       ok :)

comment:6 Changed 6 years ago by cjb

Works for me, then -- go ahead. Thanks!

Btw, the mask interface is defined in software that we control in the EC, via the http://wiki.laptop.org/go/Ec_specification, rather than in HW. (And it's mutable if we need it to be.)

comment:7 follow-up: Changed 6 years ago by dsaxena

  • Status changed from new to assigned

comment:8 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by pgf

  • Cc pgf added

so if i understand this correctly, userspace will have to read and/or write (for example) 8 separate files in order to set and/or test one byte's worth of data? am i alone in thinking this is a little nutty?

sorry -- this is largely rhetorical, but prompted out of dismay. :-/

(and the "you have a chance they can be used by other arches" comment is useless. this isn't even arch-specific -- it's _platform_ specific.)

comment:9 Changed 6 years ago by cjb

so if i understand this correctly, userspace will have to read and/or write (for example) 8 separate files in order to set and/or test one byte's worth of data? am i alone in thinking this is a little nutty?

It's awkward, but not unmanagable. We're probably only going to write four of the bits, with which four depending on what the default is..

(and the "you have a chance they can be used by other arches" comment is useless. this isn't even arch-specific -- it's _platform_ specific.)

Well, there may be other devices with configurable wakeup events in the future, and their wakeup events may bear some similarity to our own, and having files provides for the possibility of that becoming a reusable interface.

Not sure why I'm arguing so much, though; my initial reaction was your own. :)

comment:10 in reply to: ↑ 8 Changed 6 years ago by dsaxena

Replying to pgf:

so if i understand this correctly, userspace will have to read and/or write (for example) 8 separate files in order to set and/or test one byte's worth of data? am i alone in thinking this is a little nutty?

No, not alone and in looking through some sysfs files on my system, it doesn't look like
the "standard" is being followed.

I don't imagine userspace needing to deal with the all the bits at once. I would think we would react to the current state of the system and set/unset only the needed bits, but I don't know OHM internals...

From a testing perspective, it makes it easy for me as a developer to do "echo 1 > /sys/power/wakeup_events/foo" and do a quick suspend/resume cycle.

(and the "you have a chance they can be used by other arches" comment is useless. this isn't even arch-specific -- it's _platform_ specific.)

True, but we can maybe agree on some generic infrastructure (/sys/power/wakeup_events) that can be used across arches.

comment:11 follow-up: Changed 6 years ago by mstone

For what it's worth, as a userspace developer, I often care about writing programs which make delicate changes atomically with respect to other userspace processes in order to avoid racing with those processes according the vagaries of the scheduler. In general, my ability to accomplish this has strong implications for the security, correctness, and failure-tolerance of my software. (Look at all the pain surrounding creation of tmp-files, the introduction of the f* file manipulation variants, or the ppoll() and pselect() extensions for easy examples.) Consequently, and speaking generally, I would be happiest if an underlying atomic interface were provided when possible and if a "high-level shell" interface were made available separately. Any reason it's inappropriate for the kernel to provide both? (Or to build the high-level one in userspace atop the atomic kernel one?)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by dsaxena

  • Cc dilinger added

Replying to mstone:

Consequently, and speaking generally, I would be happiest if an underlying atomic interface were provided when possible and if a "high-level shell" interface were made available separately. Any reason it's inappropriate for the kernel to provide both? (Or to build the high-level one in userspace atop the atomic kernel one?)

A sysfs one-file-per-bit interface guarantee an atomic update of the mask via a read/write lock. Yes, there exists the possibility of two applications writing to the same bit back to back, but at the system level, we should not have multiple entities managing these bits. I would like write code that has a chance of being accepted upstream w/o a major rewrite and if we're going with sysfs, that may mean doing things in what seems a somewhat roundabout manner.

I've gone ahead and implemented the sysfs interface as this as it was fairly trivial; however, I'm wondering if in the long-run it may be better to not use sysfs and instead provide a full EC driver of some sort that exposes the EC interface to user space to do with it as it pleases. There seem to be multiple cases where we need to poke at the EC from OHM and adding a new sysfs file every time we need to poke at something is not very efficient. This could be wrapped in a library that could be called by OHM or utilities for use on the shell. That's a topic that should be discussed outside this bug.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 6 years ago by pgf

Replying to dsaxena:

A sysfs one-file-per-bit interface guarantee an atomic update of the mask via a read/write lock.

but how would you guarantee an atomic update of several bits at once? but perhaps that's not necessary in this case.

I've gone ahead and implemented the sysfs interface as this as it was fairly trivial; however, I'm wondering if in the long-run it may be better to not use sysfs and instead provide a full EC driver of some sort that exposes the EC interface to user space to do with it as it pleases.

that needs to be balanced against possible future platform needs. as has been said, sysfs does make future compatibility more likely, though at some cost.

comment:14 in reply to: ↑ 13 Changed 6 years ago by dsaxena

Replying to pgf:

Replying to dsaxena:

A sysfs one-file-per-bit interface guarantee an atomic update of the mask via a read/write lock.

but how would you guarantee an atomic update of several bits at once? but perhaps that's not necessary in this case.

I don't think it's completely needed. We have only one entity (OHM) that is going to be managing these bits.

I've gone ahead and implemented the sysfs interface as this as it was fairly trivial; however, I'm wondering if in the long-run it may be better to not use sysfs and instead provide a full EC driver of some sort that exposes the EC interface to user space to do with it as it pleases.

that needs to be balanced against possible future platform needs. as has been said, sysfs does make future compatibility more likely, though at some cost.

Agreed. There are also other issues with this as there maybe EC commands that we don't want userspace mucking with.

comment:15 follow-up: Changed 6 years ago by dsaxena

Chris,

You'll find a kernel image and initrd with initial code @:

http://dev.laptop.org/~dsaxena/vmlinuz-2.6.25.6.img

http://dev.laptop.org/~dsaxena/olpcrd-2.6.25.6.img

You can grab /lib/modules/2.6.25.6 out of the cpio image.

Look under /sys/power/wakeup_events for the filenames. I'll probably rename
wlan_event to just wlan before final commit.

comment:16 in reply to: ↑ 15 Changed 6 years ago by dsaxena

Replying to dsaxena:

Chris,

You'll find a kernel image and initrd with initial code @:

Note that due to #7275, you will need to run an older joyride due to mouse issues. I'm running 1935.

comment:17 Changed 6 years ago by dsaxena

Just making a note that looking at sysfs on my laptop and and the kernel sources, it appears that there is a generic "wakeup" attribute that is supported by drivers/base/power/sysfs.c to enable/disable wakeup events. I am not sure if this will work for us as the wakeup events we are poking at are not at the device register level, but at the platform chipset level; however, this is something to investigate a little before submitting upstream. One possibility is to have a concept of an EC class/bus that is exported via sysfs, with one device per control bit...to me this seems like overkill.

comment:18 Changed 6 years ago by dsaxena

  • Summary changed from The EC SCI mask should be set in "sleep" mode. to Export EC wakeup event mask to userspace
Note: See TracTickets for help on using tickets.