Re: [PATCH] rtc-dev: stop periodic interrupts on device release

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

On Saturday 26 July 2008, Mike Frysinger wrote:
> On Sat, Jul 26, 2008 at 4:50 PM, David Brownell wrote:
> > On Saturday 26 July 2008, Tomas Janousek wrote:
> >> I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their
> >> ioctl op, exporting the irq_set_state op at the same time. The logic in
> >> rtc_irq_set_state should make sure it doesn't matter and the driver should not
> >> need to care stopping periodic interrupts in its release routine any more.
> >> I did not look at other drivers though.
> >
> > A quick grep shows that out of 42 (wow!) current RTC drivers:
> >
> >  - rtc-{bfin,sa1100,sh,test} support ioctl(PIE_ON/PIE_OFF), at least
> >   before some recent patches fixing that glitch (not in my tree) by
> >   switching to irq_set_state().
> 
> the rtc-bfin.c patches are in some queue somewhere to fix this ;)

Andrew's queue I hope!  Though right now I expect he's taking a
deep breath after merging over seven hundred patches for RC1 and
so he may not be quite current on the other stuff.  :)


> >  - Of those:  rtc-{bfin,s3c,sa1100,sh,vr41xx} all have release()
> >   methods ... though it looks to me like most of those wrongly
> >   disable *all* IRQs, even ones in use by something other than
> >   the /dev client closing that FD.
> 
> rtc-bfin.c turns off all irqs and frees it in the release() function
> (since the irq is requested in the open() function).  i guess that
> isnt supposed to happen huh.

I generally expect IRQs to be requested in probe() and freed in
remove(), so it's just a bit odd ... the main thing is that kernel
interfaces to alarm and periodic IRQs (drivers/rtc/interface.c) will
misbehave if IRQs only work when the RTC is driven from userspace.

So will wake alarms triggered through sysfs, though that driver
may not support that yet.


> > That suggests there's quite a mess yet to be fixed.  This patch
> > will ensure that periodic IRQs get properly shut off by close()
> > or exit() of a task that started them.  Those release() methods
> > shouldn't then be second-guessing things.
> >
> > And then there are the other two types of IRQ.  Update IRQs can
> > only be enabled through ioctl(UIE_ON), so they're fair game to
> > turn off when closing /dev files.  Alarms seem to be a special
> > case -- best not touched when closing files.
> 
> specific drivers shouldnt worry about this then right ?
> handle it in rtc-dev ? 

That's what I believe, yes.  That approach has a nice benefit
of letting all the RTC release() infrastructure vanish (I think,
based on a quick scan of the methods) ... and even shrinks
the rtc-dev.c code a bit.  In my book, it's particularly good
to remove code when it makes things behave more consistently.

- Dave