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.
Signed-off-by: Tomas Janousek <[email protected]>
Cc: Alessandro Zummo <[email protected]>
Cc: David Brownell <[email protected]>
---
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
+ 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/
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.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.
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.
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
Hello,
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.
>
> 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.
--
Tom?? Janou?ek, a.k.a. Liskni_si, http://work.lisk.in/
On Sat, 26 Jul 2008 20:06:10 +0200
Tomáš Janoušek <[email protected]> wrote:
> 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.
> >
> > 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.
> 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.
> 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.
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
On Saturday 26 July 2008, Alessandro Zummo wrote:
> On Sat, 26 Jul 2008 20:06:10 +0200 Tomáš Janoušek <[email protected]> wrote:
> > 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.
> > >
> > > 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.
> > 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()...
> > 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.
> > 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
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 <[email protected]>
> Cc: Alessandro Zummo <[email protected]>
> Cc: David Brownell <[email protected]>
Acked-by: 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 | 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/
>
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 ;)
> - 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.
> 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 ?
-mike
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
Hello,
thanks for your insight about other RTC drivers. I have updated the
description somewhat.
On Sat, Jul 26, 2008 at 01:50:55PM -0700, David Brownell wrote:
> 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.
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.
Signed-off-by: Tomas Janousek <[email protected]>
Acked-by: David Brownell <[email protected]>
---
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
+ 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/
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 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.
>
> Signed-off-by: Tomas Janousek <[email protected]>
> Acked-by: David Brownell <[email protected]>
ok, that's fair.
Acked-by: Alessandro Zummo <[email protected]>
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
On Monday 28 July 2008, Tomáš Janoušek wrote:
> On Sat, Jul 26, 2008 at 01:50:55PM -0700, David Brownell wrote:
> > Hmm, I'd think that something like an rtc_dev_ioctl(PIE_OFF) would be
Typo, by the way ... I meant UIE_OFF.
> > preferable here ... so that it's not just UIE_EMUL logic 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.)
> 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,
On Mon, Jul 28, 2008 at 03:05:36PM -0700, David Brownell wrote:
> 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?
--
Tom?? Janou?ek, a.k.a. Liskni_si, http://work.lisk.in/
On Monday 28 July 2008, Tomáš Janoušek wrote:
> Hello,
>
> On Mon, Jul 28, 2008 at 03:05:36PM -0700, David Brownell wrote:
> > 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. ;)
> 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