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

David Brownell
2008-07-27T06:50:42Z

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