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

David Brownell
2008-07-26T20:53:45Z

On Saturday 26 July 2008, Tomas Janousek wrote:
> Solves http://bugzilla.kernel.org/show_bug.cgi?id=11127
>
> The old rtc.c driver did it, some drivers (like rtc-sh) do it in their release
> function too, but rtc-cmos does not -- because it provides the irq_set_state
> op -- so the rtc framework itself should care about it. This patch makes it do
> so.

I'd say it differently: hardly any RTC drivers do this correctly.

Maybe only rtc-sh, which tracks whether user or kernel code turned
on the periodic IRQ.

> 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().

- rtc-{cmos,s3c,sh,vr41xx} support the more correct irq_set_state()
requests, which are available for in-kernel use not just through
ioctl(PIE_ON/PIE_OFF) calls to /dev files.

- 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.

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.

> Signed-off-by: Tomas Janousek <tomi@nomi.cz>
> Cc: Alessandro Zummo <alessandro.zummo@towertech.it>
> Cc: David Brownell <david-b@pacbell.net>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

... who likes it when bugfixes just take one line of code,
even if they consume many pages of discussion. ;)

> ---
> drivers/rtc/rtc-dev.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
> index 90dfa0d..6fafa62 100644
> --- a/drivers/rtc/rtc-dev.c
> +++ b/drivers/rtc/rtc-dev.c
> @@ -408,6 +408,8 @@ static int rtc_dev_release(struct inode *inode, struct file *file)
> #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
> clear_uie(rtc);
> #endif

Hmm, I'd think that something like an rtc_dev_ioctl(PIE_OFF) would be
preferable here ... so that it's not just UIE_EMUL logic which turns
off the one-per-second update IRQs.

In fact, with a change like that I suspect the release() issues could
best be dealt with by just removing that method and its implementations...

> + rtc_irq_set_state(rtc, NULL, 0);
> +
> if (rtc->ops->release)
> rtc->ops->release(rtc->dev.parent);
>
> --
> 1.5.6
>
>
> --
> Tomáš Janoušek, a.k.a. Liskni_si, http://work.lisk.in/
>