Re: [PATCH] RTC classdev: Add sysfs support for wakeup alarm (r/w)

[David Brownell <david-b@pacbell.net>]

Hi Paul,

On Monday 18 December 2006 3:58 pm, Paul Sokolovsky wrote:
> Monday, December 18, 2006, 6:28:58 AM, you wrote:
> > On Sunday 17 December 2006 11:30 am, Paul Sokolovsky wrote:
> 
> >>     Small battery-powered systems, like PDAs, need a way to be
> >> suspended most of the time and woken up just from time to time to
> >> process pending tasks. 
> 
> > Sounds like you're thinking of this from a userspace perspective...
> 
> > Could you share some examples of such "pending tasks"?
> 
>   Well, the actual usecase, which triggered me to hack that, was a
> need to write a "burn out" test script for suspend/resume for a
> battery-powered ARM device (PDA), which would do suspend/resume cycle
> thousands of times. And wakeup alarm is obvious, if not only, source of
> automated resume events.

I like how you think ... SCRIPTED TESTING for suspend/resume!!
Can you clone yourself?  Soon, and massively?  :)

Though I confess I've used that example myself.  I think if you scan LKML
you'll find an "rtcwake" program (userspace) I've used on several different
ARMs (not PXA though), and even x86 PCs (using a new RTC driver, but getting
the usual headaches from ACPI S3 resume failures on most systems).


>   Of course, I started by trying existing solutions - e.g. there's an
> "atd" implementation which uses /dev/rtc, but I found it having awful
> latency (>2s), then I tried to write simple C app to set alarm via
> ioctl(), just to find alarm IRQs are shutdown on its exit.
> 
>   But anyway, I'm that kind of guy who think that debugging and
> diagnostics are important things for *production* system,

You'd find violent agreement from anyone who's spent time trying
to support all this fancy technology.  Heck, even toasters have
problems sometimes.


> >> Obvious way to achieve this is to use timer, or 
> >> alarm, wakeup. Unfortunately, this matter is bit confusing in Linux.
> >> There's only one "good" "supported" way to set alarm - via ioctl() on
> >> an RTC device fd. Unfortunately, this alarm is not persistent - as soon
> >> as fd is closed, alarm id discharged.
> 
> > I don't think that's true in general.  Most RTCs don't even care
> > whether userspace did an open() or close().  I see the S3C one does,
> > and that explicitly leaves the alarm active. 
> 
> > But I see that only the SA1100/PXA and SH RTCs turn off all IRQs
> > after RTC_WKALM_* requests ... that's a distinct minority.
> 
>   Oh my! I couldn't even think this can be idiosyncrasy of specific
> implementation. Oh, what a world... ;-)

Yeah, just think how bad it was _before_ the RTC class framework
existed.   I think I counted at least half a dozen implementations,
with minimal API overlap.

One thing we're missing now is RTC conformance tests.  They'd have
turned up some of these issues pretty quickly, if thery were at all
good.

 
> > So judging implementations as votes ... only two implementations
> > that implement the RTC_WKALM_* call follow that rule, and most
> > don't.  However, a few implementations ignore rtc_wkalrm.enabled,
> > or otherwise mistreat that flag (e.g. rtc-ds1553 doesn't disable
> > AIE when enabled==0), so it's clear there are some issues there.
> 
> > My vote would be that closing the FD should not turn off the alarm.
> > It's supposed to be a one-shot deal anyway.
> 
>   I would agree with such behavior. But what's clear that the
> behavior, whatever it is, should be consistent across implementations,
> or its just a mess ;-(.

Yep.  I've been known to submit patches to improve that, even ones
to the RTC framework.  :)

 
> > And also, that someone audits the drivers/rtc code to make sure that
> > alarm-capable drivers handle the rtc_wkalrm.enabled flag correctly;
> > your patch sort of presumes that will happen, anyway.
> 
>   Yes, I mentioned, that for PXA/SA, my patch becomes actually useful
> only after applying your patch (plus, with fixed TODO: here's what
> I applied to handhelds.org tree:
> http://handhelds.org/cgi-bin/cvsweb.cgi/linux/kernel26/drivers/rtc/rtc-sa1100.c.diff?r1=1.5&r2=1.6&f=h
> ).
> 
>   That of course doesn't mean sysfs alarm support patch depends on
> rtc-sa1100.c patch in any way (it's just PXA/SA won't actually wake up,
> but sysfs patch for showing/storing alarm properties obviously doesn't
> depend on any specific implementation).

Hmm, you're in a position to test, so I may send you an update to try.

That patch you applied looks right to me -- why don't you forward it
to Alessandro as a bugfix for 2.6.20-rc2, and save me the effort?

 
> >> Implement "alarm" attribute group for RTC classdevs. At this time,
> >> add "since_epoch", "wakeup_enabled", and "pending" attributes. First
> >> two support both read and write.
> 
> > I think you shouldn't add this group unless the RTC has methods
> > to read and write the alarm; there are RTCs that don't have that
> > feature.
> 
> > Also, I'd rather see a much simpler interface.  Like a single
> > "alarm" attribute.  It would display as the empty string unless
> > it was enabled, in which case the alarm time wouhd show.  To
> > disable it, write an empty string; to enable an alarm, just write
> > a valid time (in the future).  The other parameters aren't needed;
> > "wakeup" is PM infrastructure (/sys/devices/.../power/wakeup),
> > since it's easy to have an alarm that's not wakeup-capable.
> 
>   Yes, both of these are, or may be, true. That was really a draft,
> initial version. I probably don't have knowledge/resources to make it
> "right", but it would be nice if it prompted someone with more
> experience/resources to tweak in such support, as well as the problems
> outlined above...

You have enough knowledge to implement those suggestions though,
that seems clear to me.  :)

- Dave

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