rtc-dev: stop periodic interrupts on device release
PATCH 1/1 rtc-dev: stop periodic interrupts on device release
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 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.
--- drivers/rtc/rtc-dev.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/rtc/rtc-dev.c +2 -0
@@ -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 +rtc_irq_set_state (rtc,NULL ,0 ); +if (rtc->ops->release) rtc->ops->release (rtc->dev.parent);
Signature · 5 lines
1.5.6 -- Tomáš Janoušek, a.k.a. Liskni_si, http://work.lisk.in/
Quoted 7 lines · On Sat, 26 Jul 2008 16:46:17 +0100 Tomas Janousek <[email protected]> wrote:
Solves http://bugzilla.kernel.org/show_bug.cgi?id=11127
The old
rtc.cdriver did it, some drivers (like rtc-sh) do it in their release function too, but rtc-cmos does not -- because it provides theirq_set_stateop -- so the rtc framework itself should care about it. This patch makes it do so.
Quoted 5 lines
I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their ioctl op, exporting the
irq_set_stateop at the same time. The logic inrtc_irq_set_stateshould 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.
I'm not sure this is appropriate. sometimes the PIE is used to control external hardware and it doesn't make sense to have an application that's always open to handle that.
Any app should be responsible to release what it has allocated, if appropriate, and not rely on someone else to do on his behalf.
Signature · 6 lines
Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it
Hello,
Quoted 6 lines · Alessandro Zummo
I'm not sure this is appropriate. sometimes the PIE is used to control external hardware and it doesn't make sense to have an application that's always open to handle that.
Any app should be responsible to release what it has allocated, if appropriate, and not rely on someone else to do on his behalf.
mplayer and aireplay-ng have never done so. And what about crashes? Am I supposed to create a small "rtcpieoff" applications and make it into all distributions so that everyone can clean up the mess?
Additionally, it has been a regression against the old rtc and drivers like rtc-sh do so even today.
Signature · 1 line
Tomáš Janoušek, a.k.a. Liskni_si, http://work.lisk.in/
Quoted 9 lines · On Sat, 26 Jul 2008 20:06:10 +0200 Tomáš Janoušek <[email protected]> wrote:
Quoted 6 lines · Alessandro Zummo
I'm not sure this is appropriate. sometimes the PIE is used to control external hardware and it doesn't make sense to have an application that's always open to handle that.
Any app should be responsible to release what it has allocated, if appropriate, and not rely on someone else to do on his behalf.
mplayer and aireplay-ng have never done so. And what about crashes? Am I
that's not an excuse, they have been written on a false assumption. Unless that behaviour is documented somewhere.
Quoted 2 lines
supposed to create a small "rtcpieoff" applications and make it into all distributions so that everyone can clean up the mess?
just send patches to mplayer and aireplay-ng and it will make in the distros.
Quoted 2 lines
Additionally, it has been a regression against the old rtc and drivers like rtc-sh do so even today.
Specific drivers might choice to do it on close. I believe rtc-cmos might be one of the few that might have to do it in order to cope with badly written programs. It's up to David to choice if it's appropriate too add it to rtc-cmos.
Signature · 6 lines
Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it
Quoted 6 lines · Alessandro Zummo
Quoted 5 lines · On Sat, 26 Jul 2008 20:06:10 +0200 Tomáš Janoušek <[email protected]> wrote:
Quoted 3 lines · On Sat, Jul 26, 2008 at 07:55:21PM +0200 Alessandro Zummo wrote:
I'm not sure this is appropriate. sometimes the PIE is used to control external hardware and it doesn't make sense to have an application that's always open to handle that.
I'd stay that having an application running is the standard way to handle such stuff!
In fact, how could the 1/(2^N) Hz periodic interrupt ever control external hardware *by itself*? It would need some control path that's not part of the programming interface, like routing some signal from the RTC to that hardware. That's PWM functionality, and isn't available from all timers/clocks/rtcs.
The only control path in scope of the RTC programming interface is client/application code responding to a (periodic) IRQ by issuing some type of command.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Any app should be responsible to release what it has allocated, if appropriate, and not rely on someone else to do on his behalf.
But exiting a task is responsible for freeing all of its kernel resources ... closing file descriptors, releasing memory, and so forth. Programs rely on that, as in this case.
Quoted 4 lines
Quoted 1 line
mplayer and aireplay-ng have never done so. And what about crashes? Am I
that's not an excuse, they have been written on a false assumption. Unless that behaviour is documented somewhere.
We can't rely on documentation for interfaces (like the legacy RTC interface which the RTC framework is emulating) that never really had complete documentation!!
In this case, we need to follow interface behavior, as observed, when it's not a bug. Like shutting down periodic IRQs on close()...
Quoted 4 lines
Quoted 2 lines
supposed to create a small "rtcpieoff" applications and make it into all distributions so that everyone can clean up the mess?
just send patches to mplayer and aireplay-ng and it will make in the distros.
Except ... that makes it even more clear that you're suggesting changes to a well established userspace interface. Which is not something we like doing to the Linux userspace community.
Quoted 7 lines
Quoted 2 lines
Additionally, it has been a regression against the old rtc and drivers like rtc-sh do so even today.
Specific drivers might choice to do it on close. I believe rtc-cmos might be one of the few that might have to do it in order to cope with badly written programs. It's up to David to choice if it's appropriate too add it to rtc-cmos.
I want the framework to handle it, since that's how the RTC programming interface has traditionally behaved and since there should be as few driver-specific behaviors as practical.
Remember, those programs should not be growing needless hardware dependencies. In particular "only works on x86" would be really nasty.
If it needs to be changed on x86 so that most Linux programs behave correctly again, then it needs to be changed for all RTC drivers. Which means updating the framework is the most effective way to solve the problem.
- Dave
Quoted 6 lines · Tomas Janousek
Solves http://bugzilla.kernel.org/show_bug.cgi?id=11127
The old
rtc.cdriver did it, some drivers (like rtc-sh) do it in their release function too, but rtc-cmos does not -- because it provides theirq_set_stateop -- 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.
Quoted 5 lines
I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their ioctl op, exporting the
irq_set_stateop at the same time. The logic inrtc_irq_set_stateshould 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.
Quoted 3 lines
Signed-off-by Tomas Janousek <[email protected]>Cc Alessandro Zummo <[email protected]>Cc David Brownell <[email protected]>
... who likes it when bugfixes just take one line of code, even if they consume many pages of discussion. ;)
drivers/rtc/rtc-dev.c static int rtc_dev_release(struct inode *inode, struct file *file) · line 410
Quoted 12 lines
--- drivers/rtc/rtc-dev.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)drivers/rtc/rtc-dev.c +0
-0@@ -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...
Quoted 12 lines
drivers/rtc/rtc-dev.c +2
-0+rtc_irq_set_state (rtc,NULL ,0 ); +if (rtc->ops->release) rtc->ops->release (rtc->dev.parent);Signature · 5 lines
1.5.6 -- Tomáš Janoušek, a.k.a. Liskni_si, http://work.lisk.in/
Quoted 7 lines · David Brownell
Quoted 5 lines · Tomas Janousek
I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their ioctl op, exporting the
irq_set_stateop at the same time. The logic inrtc_irq_set_stateshould 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.
Quoted 5 lines
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 toirq_set_state().
the rtc-bfin.c patches are in some queue somewhere to fix this ;)
Quoted 4 lines
- 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.
Quoted 9 lines
That suggests there's quite a mess yet to be fixed. This patch will ensure that periodic IRQs get properly shut off by
close()orexit()of a task that started them. Thoserelease()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 ? -mike
Quoted 8 lines · Mike Frysinger
Quoted 7 lines · David Brownell
Quoted 5 lines · Tomas Janousek
I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their ioctl op, exporting the
irq_set_stateop at the same time. The logic inrtc_irq_set_stateshould 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.
Quoted 7 lines
Quoted 5 lines
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 toirq_set_state().the
rtc-bfin.cpatches 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. :)
Quoted 8 lines
Quoted 4 lines
- 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.cturns off all irqs and frees it in therelease()function (since the irq is requested in theopen()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.
Quoted 5 lines
Quoted 5 lines
That suggests there's quite a mess yet to be fixed. This patch will ensure that periodic IRQs get properly shut off by
close()orexit()of a task that started them. Thoserelease()methods shouldn't then be second-guessing things.
Quoted 7 lines
Quoted 4 lines
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
Tomáš Janoušek
Acked-by David BrownellHello,
thanks for your insight about other RTC drivers. I have updated the description somewhat.
Quoted 3 lines · David Brownell
Hmm, I'd think that something like an
rtc_dev_ioctl(PIE_OFF)would be preferable here ... so that it's not justUIE_EMULlogic which turns off the one-per-second update IRQs.
I think it'd be more consistent if the framework only called the rtc api functions. Like: if the driver doesn't export an op for it and handles it in the ioctl op, it itself should be responsible to clear the irq in its release op. (I know there's no op for UIE, so we'd better add it instead of calling ioctl in the framework's release function.) More explanation in the updated patch desc.
---
From: Tomas Janousek <[email protected]>
Date: Sat, 26 Jul 2008 16:23:36 +0100
Subject: [PATCH] rtc-dev: stop periodic interrupts on device release
Solves http://bugzilla.kernel.org/show_bug.cgi?id=11127
The old rtc.c driver did it and some drivers (like rtc-sh) do it in their release function, though they should not -- because they should provide the irq_set_state op and the rtc framework itself should care about it. This patch makes it do so.
I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their ioctl op (instead of having the framework call the 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.
The correct way, in my opinion, should be this:
1) The driver provides the irq_set_state op and does not care closing the interrupts in its release op.
2) If the driver does not provide the op and handles PIE in the ioctl op, it's reponsible for closing them in its release op.
3) Something similar for other IRQs, like UIE -- if there's no in-kernel API like irq_set_state, handle it in ioctl and release ops. The framework will be responsible either for everything or for nothing.
--- drivers/rtc/rtc-dev.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/rtc/rtc-dev.c +2 -0
@@ -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 +rtc_irq_set_state (rtc,NULL ,0 ); +if (rtc->ops->release) rtc->ops->release (rtc->dev.parent);
Signature · 5 lines
1.5.6 -- Tomáš Janoušek, a.k.a. Liskni_si, http://work.lisk.in/
Alessandro Zummo
Acked-byQuoted 27 lines · On Mon, 28 Jul 2008 22:41:36 +0200 Tomáš Janoušek <[email protected]> wrote:
From: Tomas Janousek <[email protected]>
Date: Sat, 26 Jul 2008 16:23:36 +0100
Subject: [PATCH] rtc-dev: stop periodic interrupts on device releaseSolves http://bugzilla.kernel.org/show_bug.cgi?id=11127
The old
rtc.cdriver did it and some drivers (like rtc-sh) do it in their release function, though they should not -- because they should provide theirq_set_stateop and the rtc framework itself should care about it. This patch makes it do so.I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their ioctl op (instead of having the framework call the op), exporting the
irq_set_stateop at the same time. The logic inrtc_irq_set_stateshould make sure it doesn't matter and the driver should not need to care stopping periodic interrupts in its release routine any more.The correct way, in my opinion, should be this:
1) The driver provides theirq_set_stateop and does not care closing the interrupts in its release op.
2) If the driver does not provide the op and handles PIE in the ioctl op, it's reponsible for closing them in its release op.
3) Something similar for other IRQs, like UIE -- if there's no in-kernel API likeirq_set_state, handle it in ioctl and release ops. The framework will be responsible either for everything or for nothing.
Quoted 2 lines
Signed-off-by Tomas Janousek <[email protected]>Acked-by David Brownell <[email protected]>
ok, that's fair.
Signature · 6 lines
Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it
Quoted 2 lines · Tomáš Janoušek
Quoted 1 line · David Brownell
Hmm, I'd think that something like an
rtc_dev_ioctl(PIE_OFF)would be
Typo, by the way ... I meant UIE_OFF.
Quoted 5 lines
Quoted 2 lines
preferable here ... so that it's not just
UIE_EMULlogic which turns off the one-per-second update IRQs.I think it'd be more consistent if the framework only called the rtc api functions.
When they exist, sure. But they currently don't, and I see no value in adding them just to avoid a simple rtc_dev_ioctl(UIE_OFF) call. It'd be different if any in-kernel code used update IRQs ... NTP sync?
(Regardless, that's a separate bug and appropriate for a different patch.)
Quoted 4 lines
Like: if the driver doesn't export an op for it and handles it in the ioctl op, it itself should be responsible to clear the irq in its release op. (I know there's no op for UIE, so we'd better add it instead of calling ioctl in the framework's release function.)
I still think the *existence* of a release() op is a problem. It's requiring the drivers to maintain history they should never need.
Surely you agree that having the framework shut down only *emulated* update IRQs, not "real" ones, is inconsistent? And hence undesirable?
- Dave
Hello,
Quoted 2 lines · David Brownell
Surely you agree that having the framework shut down only *emulated* update IRQs, not "real" ones, is inconsistent? And hence undesirable?
The idea was that if the "real" ones get turned on using some ioctl magic the framework has no exact control over, they shouldn't be shut down by it. But yeah, your point of view looks fine as well.
So I guess I'll post the current patch to Andrew and then, someone (not me, for time and competence reasons, sorry) can prepare a patch removing the release op and changing the calls in framework's release to call rtc_dev_ioctl.
Is this ok?
Signature · 1 line
Tomáš Janoušek, a.k.a. Liskni_si, http://work.lisk.in/
Quoted 9 lines · Tomáš Janoušek
Hello,
Quoted 2 lines · David Brownell
Surely you agree that having the framework shut down only *emulated* update IRQs, not "real" ones, is inconsistent? And hence undesirable?
The idea was that if the "real" ones get turned on using some ioctl magic the framework has no exact control over, they shouldn't be shut down by it. But yeah, your point of view looks fine as well.
The /dev/rtcN support *is* part of the framework. ;)
Quoted 6 lines
So I guess I'll post the current patch to Andrew and then, someone (not me, for time and competence reasons, sorry) can prepare a patch removing the release op and changing the calls in framework's release to call
rtc_dev_ioctl.Is this ok?
Yep.
- Dave