Messages in this thread | | | From | David Brownell <> | Subject | Re: [PATCH] rtc-dev: stop periodic interrupts on device release | Date | Sat, 26 Jul 2008 22:03:36 -0700 |
| |
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
| |