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

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

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