This patch fixes the following issues with the RTC API:
1 - The alarm_irq_enable() and the update_irq_enable() callbacks were
not beeing called anywhere in the code. They are necessary if you want
to write a driver that does not use the kernel pre-existent timer
implemented alarm and update interrupts.
2 - In the current configuration, an update interrupt, for example,
would be a trigger for an alarm interrupt as well, even though the mode
variable stated it was an update interrupt. To avoid that, mode is
beeing checked inside rtc_handle_legacy_irq(). That used to be the
previous user space behaviour.
3 - The irq_set_freq() and rtc_irq_set_state() functions are entry
points to the RTC API, and were not calling the irq_set_state() and
irq_set_freq() callbacks. These are also necessary overrides to provide
a proper independent RTC implementation.
4 - The rtc-test kernel RTC driver has been updated to work properly
with the new kernel RTC timer infrastructure. This driver has been used
together with the rtctest.c source code present in the file
Documentation/rtc.txt to test the user space behaviour of all the
modifications in this patch.
Signed-off-by: Marcelo Roberto Jimenez <[email protected]>
---
drivers/rtc/interface.c | 63 +++++++++++++++++++++++++++++---------
drivers/rtc/rtc-dev.c | 12 ++++----
drivers/rtc/rtc-test.c | 77 ++++++++++++++++++++++++++++++++++++++++------
3 files changed, 121 insertions(+), 31 deletions(-)
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 90384b9..6cb4b65 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -194,6 +194,11 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
if (err)
return err;
+ if (rtc->ops->alarm_irq_enable) {
+ err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled);
+ goto out;
+ }
+
if (rtc->aie_timer.enabled != enabled) {
if (enabled) {
rtc->aie_timer.enabled = 1;
@@ -211,6 +216,7 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
else
err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled);
+out:
mutex_unlock(&rtc->ops_lock);
return err;
}
@@ -222,6 +228,11 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled)
if (err)
return err;
+ if (rtc->ops->update_irq_enable) {
+ err = rtc->ops->update_irq_enable(rtc->dev.parent, enabled);
+ goto out;
+ }
+
/* make sure we're changing state */
if (rtc->uie_rtctimer.enabled == enabled)
goto out;
@@ -263,19 +274,23 @@ static void rtc_handle_legacy_irq(struct rtc_device *rtc, int num, int mode)
{
unsigned long flags;
- /* mark one irq of the appropriate mode */
- spin_lock_irqsave(&rtc->irq_lock, flags);
- rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode);
- spin_unlock_irqrestore(&rtc->irq_lock, flags);
-
- /* call the task func */
- spin_lock_irqsave(&rtc->irq_task_lock, flags);
- if (rtc->irq_task)
- rtc->irq_task->func(rtc->irq_task->private_data);
- spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
-
- wake_up_interruptible(&rtc->irq_queue);
- kill_fasync(&rtc->async_queue, SIGIO, POLL_IN);
+ if (((mode & RTC_UF) && rtc->uie_rtctimer.enabled) ||
+ ((mode & RTC_AF) && rtc->aie_timer.enabled) ||
+ ((mode & RTC_PF) && rtc->pie_enabled)) {
+ /* mark one irq of the appropriate mode */
+ spin_lock_irqsave(&rtc->irq_lock, flags);
+ rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode);
+ spin_unlock_irqrestore(&rtc->irq_lock, flags);
+
+ /* call the task func */
+ spin_lock_irqsave(&rtc->irq_task_lock, flags);
+ if (rtc->irq_task)
+ rtc->irq_task->func(rtc->irq_task->private_data);
+ spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
+
+ wake_up_interruptible(&rtc->irq_queue);
+ kill_fasync(&rtc->async_queue, SIGIO, POLL_IN);
+ }
}
@@ -423,9 +438,18 @@ EXPORT_SYMBOL_GPL(rtc_irq_unregister);
*/
int rtc_irq_set_state(struct rtc_device *rtc, struct rtc_task *task, int enabled)
{
- int err = 0;
unsigned long flags;
+ int err = mutex_lock_interruptible(&rtc->ops_lock);
+ if (err)
+ return err;
+ if (rtc->ops->irq_set_state) {
+ err = rtc->ops->irq_set_state(rtc->dev.parent, enabled);
+ mutex_unlock(&rtc->ops_lock);
+ return err;
+ }
+ mutex_unlock(&rtc->ops_lock);
+
spin_lock_irqsave(&rtc->irq_task_lock, flags);
if (rtc->irq_task != NULL && task == NULL)
err = -EBUSY;
@@ -457,9 +481,18 @@ EXPORT_SYMBOL_GPL(rtc_irq_set_state);
*/
int rtc_irq_set_freq(struct rtc_device *rtc, struct rtc_task *task, int freq)
{
- int err = 0;
unsigned long flags;
+ int err = mutex_lock_interruptible(&rtc->ops_lock);
+ if (err)
+ return err;
+ if (rtc->ops->irq_set_freq) {
+ err = rtc->ops->irq_set_freq(rtc->dev.parent, freq);
+ mutex_unlock(&rtc->ops_lock);
+ return err;
+ }
+ mutex_unlock(&rtc->ops_lock);
+
spin_lock_irqsave(&rtc->irq_task_lock, flags);
if (rtc->irq_task != NULL && task == NULL)
err = -EBUSY;
diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index 212b16e..d901160 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -261,12 +261,12 @@ static long rtc_dev_ioctl(struct file *file,
return rtc_set_time(rtc, &tm);
case RTC_PIE_ON:
- err = rtc_irq_set_state(rtc, NULL, 1);
- break;
+ mutex_unlock(&rtc->ops_lock);
+ return rtc_irq_set_state(rtc, NULL, 1);
case RTC_PIE_OFF:
- err = rtc_irq_set_state(rtc, NULL, 0);
- break;
+ mutex_unlock(&rtc->ops_lock);
+ return rtc_irq_set_state(rtc, NULL, 0);
case RTC_AIE_ON:
mutex_unlock(&rtc->ops_lock);
@@ -285,8 +285,8 @@ static long rtc_dev_ioctl(struct file *file,
return rtc_update_irq_enable(rtc, 0);
case RTC_IRQP_SET:
- err = rtc_irq_set_freq(rtc, NULL, arg);
- break;
+ mutex_unlock(&rtc->ops_lock);
+ return rtc_irq_set_freq(rtc, NULL, arg);
case RTC_IRQP_READ:
err = put_user(rtc->irq_freq, (unsigned long __user *)uarg);
diff --git a/drivers/rtc/rtc-test.c b/drivers/rtc/rtc-test.c
index 51725f7..7cbe3e3 100644
--- a/drivers/rtc/rtc-test.c
+++ b/drivers/rtc/rtc-test.c
@@ -13,6 +13,8 @@
#include <linux/rtc.h>
#include <linux/platform_device.h>
+static const unsigned long RTC_DEFAULT_PERIODIC_FREQ = 1024;
+
static struct platform_device *test0 = NULL, *test1 = NULL;
static int test_rtc_read_alarm(struct device *dev,
@@ -31,12 +33,54 @@ static int test_rtc_read_time(struct device *dev,
struct rtc_time *tm)
{
rtc_time_to_tm(get_seconds(), tm);
+
return 0;
}
static int test_rtc_set_mmss(struct device *dev, unsigned long secs)
{
dev_info(dev, "%s, secs = %lu\n", __func__, secs);
+
+ return 0;
+}
+
+static int test_rtc_irq_set_state(struct device *dev, int enabled)
+{
+ struct platform_device *plat_dev = to_platform_device(dev);
+ struct rtc_device *rtc = platform_get_drvdata(plat_dev);
+
+ rtc->pie_enabled = enabled;
+
+ return 0;
+}
+
+static int test_rtc_irq_set_freq(struct device *dev, int freq)
+{
+ struct platform_device *plat_dev = to_platform_device(dev);
+ struct rtc_device *rtc = platform_get_drvdata(plat_dev);
+
+ rtc->irq_freq = freq;
+
+ return 0;
+}
+
+static int test_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+ struct platform_device *plat_dev = to_platform_device(dev);
+ struct rtc_device *rtc = platform_get_drvdata(plat_dev);
+
+ rtc->aie_timer.enabled = enabled;
+
+ return 0;
+}
+
+static int test_rtc_update_irq_enable(struct device *dev, unsigned int enabled)
+{
+ struct platform_device *plat_dev = to_platform_device(dev);
+ struct rtc_device *rtc = platform_get_drvdata(plat_dev);
+
+ rtc->uie_rtctimer.enabled = enabled;
+
return 0;
}
@@ -50,12 +94,11 @@ static int test_rtc_proc(struct device *dev, struct seq_file *seq)
return 0;
}
+/* As this is a test device, I am leaving the ioctl() code here to make it
+ * simpler for those want to test. The code currently does nothing. */
static int test_rtc_ioctl(struct device *dev, unsigned int cmd,
unsigned long arg)
{
- /* We do support interrupts, they're generated
- * using the sysfs interface.
- */
switch (cmd) {
case RTC_PIE_ON:
case RTC_PIE_OFF:
@@ -63,7 +106,9 @@ static int test_rtc_ioctl(struct device *dev, unsigned int cmd,
case RTC_UIE_OFF:
case RTC_AIE_ON:
case RTC_AIE_OFF:
- return 0;
+ /* Return zero in case you want to process the ioctl() */
+ /*return 0;*/
+ return -ENOIOCTLCMD;
default:
return -ENOIOCTLCMD;
@@ -77,16 +122,22 @@ static const struct rtc_class_ops test_rtc_ops = {
.set_alarm = test_rtc_set_alarm,
.set_mmss = test_rtc_set_mmss,
.ioctl = test_rtc_ioctl,
+ .irq_set_state = test_rtc_irq_set_state,
+ .irq_set_freq = test_rtc_irq_set_freq,
+ .alarm_irq_enable = test_rtc_alarm_irq_enable,
+ .update_irq_enable = test_rtc_update_irq_enable,
};
static ssize_t test_irq_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+ struct device_attribute *attr, char *buf)
{
return sprintf(buf, "%d\n", 42);
}
+
+/* We support interrupts generated using the sysfs interface. */
static ssize_t test_irq_store(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
+ struct device_attribute *attr,
+ const char *buf, size_t count)
{
int retval;
struct platform_device *plat_dev = to_platform_device(dev);
@@ -94,11 +145,11 @@ static ssize_t test_irq_store(struct device *dev,
retval = count;
if (strncmp(buf, "tick", 4) == 0)
- rtc_update_irq(rtc, 1, RTC_PF | RTC_IRQF);
+ rtc_pie_update_irq(&rtc->pie_timer);
else if (strncmp(buf, "alarm", 5) == 0)
- rtc_update_irq(rtc, 1, RTC_AF | RTC_IRQF);
+ rtc_aie_update_irq(rtc);
else if (strncmp(buf, "update", 6) == 0)
- rtc_update_irq(rtc, 1, RTC_UF | RTC_IRQF);
+ rtc_uie_update_irq(rtc);
else
retval = -EINVAL;
@@ -120,6 +171,12 @@ static int test_probe(struct platform_device *plat_dev)
if (err)
goto err;
+ /* If we don't set rtc->irq_freq here, a spurious periodic interrup
+ * can cause a division by zero in the kernel. And with this driver
+ * we certainly can generate spurious interrupts through the sys
+ * interface. */
+ rtc->irq_freq = RTC_DEFAULT_PERIODIC_FREQ;
+
platform_set_drvdata(plat_dev, rtc);
return 0;
--
1.7.3.4
On Tue, 2011-02-01 at 14:30 -0200, Marcelo Roberto Jimenez wrote:
> This patch fixes the following issues with the RTC API:
>
> 1 - The alarm_irq_enable() and the update_irq_enable() callbacks were
> not beeing called anywhere in the code. They are necessary if you want
> to write a driver that does not use the kernel pre-existent timer
> implemented alarm and update interrupts.
I'm not sure I totally follow this one. alarm_irq_enable is still being
called in that function.
There are some callbacks that are no longer being made because the
functionality is now virtualized internally to the kernel. Could you
expand on the rational for this a bit more? Possibly showing a userland
example why the virtualized implementation isn't sufficient?
I will admit that I probably should clean out the callback hooks and
remove them from the drivers, as that probably adds to some of the
confusion here.
> 2 - In the current configuration, an update interrupt, for example,
> would be a trigger for an alarm interrupt as well, even though the mode
> variable stated it was an update interrupt. To avoid that, mode is
> beeing checked inside rtc_handle_legacy_irq(). That used to be the
> previous user space behaviour.
Right, we internally use recurring alarm interrupts to emulate the
update interrupt mode. Although I believe I provided the right behavior
to userland, so I'm not sure I see the issue or how your changes correct
things.
> 3 - The irq_set_freq() and rtc_irq_set_state() functions are entry
> points to the RTC API, and were not calling the irq_set_state() and
> irq_set_freq() callbacks. These are also necessary overrides to provide
> a proper independent RTC implementation.
Again, the freq is handled via a hrtimer now, so we shouldn't be calling
out to the hardware specific rtc driver anymore.
> 4 - The rtc-test kernel RTC driver has been updated to work properly
> with the new kernel RTC timer infrastructure. This driver has been used
> together with the rtctest.c source code present in the file
> Documentation/rtc.txt to test the user space behaviour of all the
> modifications in this patch.
So forgive me, as I might just be missing what you're describing. It
might be easier if you break fixes up into multiple patches?
More comments in the code below.
> Signed-off-by: Marcelo Roberto Jimenez <[email protected]>
> ---
> drivers/rtc/interface.c | 63 +++++++++++++++++++++++++++++---------
> drivers/rtc/rtc-dev.c | 12 ++++----
> drivers/rtc/rtc-test.c | 77 ++++++++++++++++++++++++++++++++++++++++------
> 3 files changed, 121 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 90384b9..6cb4b65 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -194,6 +194,11 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
> if (err)
> return err;
>
> + if (rtc->ops->alarm_irq_enable) {
> + err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled);
> + goto out;
> + }
> +
> if (rtc->aie_timer.enabled != enabled) {
> if (enabled) {
> rtc->aie_timer.enabled = 1;
> @@ -211,6 +216,7 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
> else
> err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled);
>
> +out:
> mutex_unlock(&rtc->ops_lock);
> return err;
So first, you're not checking if rtc->ops is valid before dereferencing.
Second, I'm not sure I see why you would call it and then avoid the new
virtualized rtc layer? Esp since we do end up calling the same
alarm_irq_enable function later (see the top of your second chunk).
> }
> @@ -222,6 +228,11 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled)
> if (err)
> return err;
>
> + if (rtc->ops->update_irq_enable) {
> + err = rtc->ops->update_irq_enable(rtc->dev.parent, enabled);
> + goto out;
> + }
> +
> /* make sure we're changing state */
> if (rtc->uie_rtctimer.enabled == enabled)
> goto out;
Same issue above here.
> @@ -263,19 +274,23 @@ static void rtc_handle_legacy_irq(struct rtc_device *rtc, int num, int mode)
> {
> unsigned long flags;
>
> - /* mark one irq of the appropriate mode */
> - spin_lock_irqsave(&rtc->irq_lock, flags);
> - rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode);
> - spin_unlock_irqrestore(&rtc->irq_lock, flags);
> -
> - /* call the task func */
> - spin_lock_irqsave(&rtc->irq_task_lock, flags);
> - if (rtc->irq_task)
> - rtc->irq_task->func(rtc->irq_task->private_data);
> - spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
> -
> - wake_up_interruptible(&rtc->irq_queue);
> - kill_fasync(&rtc->async_queue, SIGIO, POLL_IN);
> + if (((mode & RTC_UF) && rtc->uie_rtctimer.enabled) ||
> + ((mode & RTC_AF) && rtc->aie_timer.enabled) ||
> + ((mode & RTC_PF) && rtc->pie_enabled)) {
> + /* mark one irq of the appropriate mode */
> + spin_lock_irqsave(&rtc->irq_lock, flags);
> + rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode);
> + spin_unlock_irqrestore(&rtc->irq_lock, flags);
> +
> + /* call the task func */
> + spin_lock_irqsave(&rtc->irq_task_lock, flags);
> + if (rtc->irq_task)
> + rtc->irq_task->func(rtc->irq_task->private_data);
> + spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
> +
> + wake_up_interruptible(&rtc->irq_queue);
> + kill_fasync(&rtc->async_queue, SIGIO, POLL_IN);
> + }
> }
I don't quite understand what this is doing.
> @@ -423,9 +438,18 @@ EXPORT_SYMBOL_GPL(rtc_irq_unregister);
> */
> int rtc_irq_set_state(struct rtc_device *rtc, struct rtc_task *task, int enabled)
> {
> - int err = 0;
> unsigned long flags;
>
> + int err = mutex_lock_interruptible(&rtc->ops_lock);
> + if (err)
> + return err;
> + if (rtc->ops->irq_set_state) {
> + err = rtc->ops->irq_set_state(rtc->dev.parent, enabled);
> + mutex_unlock(&rtc->ops_lock);
> + return err;
> + }
> + mutex_unlock(&rtc->ops_lock);
> +
> spin_lock_irqsave(&rtc->irq_task_lock, flags);
> if (rtc->irq_task != NULL && task == NULL)
> err = -EBUSY;
Again, not checking rtc->ops and irq_set_state shouldn't be called
anymore, as the hrtimer handles periodic mode irq emulation now.
> @@ -457,9 +481,18 @@ EXPORT_SYMBOL_GPL(rtc_irq_set_state);
> */
> int rtc_irq_set_freq(struct rtc_device *rtc, struct rtc_task *task, int freq)
> {
> - int err = 0;
> unsigned long flags;
>
> + int err = mutex_lock_interruptible(&rtc->ops_lock);
> + if (err)
> + return err;
> + if (rtc->ops->irq_set_freq) {
> + err = rtc->ops->irq_set_freq(rtc->dev.parent, freq);
> + mutex_unlock(&rtc->ops_lock);
> + return err;
> + }
> + mutex_unlock(&rtc->ops_lock);
> +
Same as the last comment here. irq_set_freq is handled by generic
emulation code now.
> diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
> index 212b16e..d901160 100644
> --- a/drivers/rtc/rtc-dev.c
> +++ b/drivers/rtc/rtc-dev.c
> @@ -261,12 +261,12 @@ static long rtc_dev_ioctl(struct file *file,
> return rtc_set_time(rtc, &tm);
>
> case RTC_PIE_ON:
> - err = rtc_irq_set_state(rtc, NULL, 1);
> - break;
> + mutex_unlock(&rtc->ops_lock);
> + return rtc_irq_set_state(rtc, NULL, 1);
>
> case RTC_PIE_OFF:
> - err = rtc_irq_set_state(rtc, NULL, 0);
> - break;
> + mutex_unlock(&rtc->ops_lock);
> + return rtc_irq_set_state(rtc, NULL, 0);
>
> case RTC_AIE_ON:
> mutex_unlock(&rtc->ops_lock);
> @@ -285,8 +285,8 @@ static long rtc_dev_ioctl(struct file *file,
> return rtc_update_irq_enable(rtc, 0);
>
> case RTC_IRQP_SET:
> - err = rtc_irq_set_freq(rtc, NULL, arg);
> - break;
> + mutex_unlock(&rtc->ops_lock);
> + return rtc_irq_set_freq(rtc, NULL, arg);
>
> case RTC_IRQP_READ:
Why the unlock/return change here?
If I'm being totally daft here, my apologies and please let me know. The
rework I did was fairly large, and its quite possible there are issues
lurking. But I'm not connecting your patch to specific breakage yet.
thanks
-john
Hi John,
Thank you for the quick answer.
My original motivation was to rewrite the strongarm rtc, which
recently got broken due to the last changes. In the process, I tested
with the rtc-test kernel RTC driver to make it work as it did before,
but I may have got some semantics wrong.
On Wed, Feb 2, 2011 at 01:00, John Stultz <[email protected]> wrote:
> On Tue, 2011-02-01 at 14:30 -0200, Marcelo Roberto Jimenez wrote:
>> This patch fixes the following issues with the RTC API:
>>
>> 1 - The alarm_irq_enable() and the update_irq_enable() callbacks were
>> not beeing called anywhere in the code. They are necessary if you want
>> to write a driver that does not use the kernel pre-existent timer
>> implemented alarm and update interrupts.
>
> I'm not sure I totally follow this one. alarm_irq_enable is still being
> called in that function.
>
> There are some callbacks that are no longer being made because the
> functionality is now virtualized internally to the kernel. Could you
> expand on the rational for this a bit more? Possibly showing a userland
> example why the virtualized implementation isn't sufficient?
>
> I will admit that I probably should clean out the callback hooks and
> remove them from the drivers, as that probably adds to some of the
> confusion here.
You are right, alarm_irq_enable() is already being called, but after
the timer enqueue/remove code. My original intention was to change the
order but I missed the removal of the old code, sorry for that.
I understood that this callback was there in case one wants to
implement an alternative to the current kernel RTC implementation, but
maybe I got that wrong.
The reason I added it before was that the user space behavior of the
rtc-test kernel RTC driver had changed, but maybe its previous
behavior is is no longer acceptable. Alarm interrupts were hand
generated by the user by echoing to the sys interface, but it does not
make sense to even have a timer in that case. That way,
alarm_irq_enable() would bypass the kernel implementation.
Also, the way it is today, the programmer implementing the driver
_must_ define alarm_irq_enable(), even if this function does nothing,
otherwise you get EINVAL.
Maybe I got alarm_irq_enable() completely wrong and it should just be removed.
>> 2 - In the current configuration, an update interrupt, for example,
>> would be a trigger for an alarm interrupt as well, even though the mode
>> variable stated it was an update interrupt. To avoid that, mode is
>> beeing checked inside rtc_handle_legacy_irq(). That used to be the
>> previous user space behaviour.
>
> Right, we internally use recurring alarm interrupts to emulate the
> update interrupt mode. Although I believe I provided the right behavior
> to userland, so I'm not sure I see the issue or how your changes correct
> things.
Again, what the rtc-test kernel RTC and the strongarm RTC user space
behavior have changed. Alarm interrupts and update interrupts were
generated by a different interrupts in the strongarm driver, and the
rtc-test driver also behaved similarly, i.e., an update interrupt did
not trigger an alarm interrupt. Currently, rtc_handle_legacy_irq()
centralizes the irq processing, and by not checking the generated
interrupt, it allows the new behavior, which seemed broken to me.
>> 3 - The irq_set_freq() and rtc_irq_set_state() functions are entry
>> points to the RTC API, and were not calling the irq_set_state() and
>> irq_set_freq() callbacks. These are also necessary overrides to provide
>> a proper independent RTC implementation.
>
> Again, the freq is handled via a hrtimer now, so we shouldn't be calling
> out to the hardware specific rtc driver anymore.
My reasoning here goes along the same lines as in the
alarm_irq_enable() case, so I believe those callbacks should also be
removed, right?
For rtc_irq_set_freq(), I believe that the current code allows a user
space program to call the RTC_IRQP_SET ioclt() with freq == 0 and have
a division by zero in the kernel code. I'll send a separate patch for
that.
>> 4 - The rtc-test kernel RTC driver has been updated to work properly
>> with the new kernel RTC timer infrastructure. This driver has been used
>> together with the rtctest.c source code present in the file
>> Documentation/rtc.txt to test the user space behaviour of all the
>> modifications in this patch.
>
> So forgive me, as I might just be missing what you're describing. It
> might be easier if you break fixes up into multiple patches?
The original purpose of the patch was to bring back the previous
rtc-test driver behaviour, I think that anything less would be to
commit broken work in progress code.
Anyway, this rtc-test driver has been handy before for debugging
purposes, I guess we should decide if it still makes sense to have it
around. Currently, it is broken.
> More comments in the code below.
>
> ...
>
> So first, you're not checking if rtc->ops is valid before dereferencing.
> Second, I'm not sure I see why you would call it and then avoid the new
> virtualized rtc layer? Esp since we do end up calling the same
> alarm_irq_enable function later (see the top of your second chunk).
>
> ...
>
> Same issue above here.
Ok, I have answered that on my first paragraph.
>> @@ -263,19 +274,23 @@ static void rtc_handle_legacy_irq(struct rtc_device *rtc, int num, int mode)
>> ?{
>> ? ? ? unsigned long flags;
>>
>> - ? ? /* mark one irq of the appropriate mode */
>> - ? ? spin_lock_irqsave(&rtc->irq_lock, flags);
>> - ? ? rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode);
>> - ? ? spin_unlock_irqrestore(&rtc->irq_lock, flags);
>> -
>> - ? ? /* call the task func */
>> - ? ? spin_lock_irqsave(&rtc->irq_task_lock, flags);
>> - ? ? if (rtc->irq_task)
>> - ? ? ? ? ? ? rtc->irq_task->func(rtc->irq_task->private_data);
>> - ? ? spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
>> -
>> - ? ? wake_up_interruptible(&rtc->irq_queue);
>> - ? ? kill_fasync(&rtc->async_queue, SIGIO, POLL_IN);
>> + ? ? if (((mode & RTC_UF) && rtc->uie_rtctimer.enabled) ||
>> + ? ? ? ? ((mode & RTC_AF) && rtc->aie_timer.enabled) ||
>> + ? ? ? ? ((mode & RTC_PF) && rtc->pie_enabled)) {
>> + ? ? ? ? ? ? /* mark one irq of the appropriate mode */
>> + ? ? ? ? ? ? spin_lock_irqsave(&rtc->irq_lock, flags);
>> + ? ? ? ? ? ? rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode);
>> + ? ? ? ? ? ? spin_unlock_irqrestore(&rtc->irq_lock, flags);
>> +
>> + ? ? ? ? ? ? /* call the task func */
>> + ? ? ? ? ? ? spin_lock_irqsave(&rtc->irq_task_lock, flags);
>> + ? ? ? ? ? ? if (rtc->irq_task)
>> + ? ? ? ? ? ? ? ? ? ? rtc->irq_task->func(rtc->irq_task->private_data);
>> + ? ? ? ? ? ? spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
>> +
>> + ? ? ? ? ? ? wake_up_interruptible(&rtc->irq_queue);
>> + ? ? ? ? ? ? kill_fasync(&rtc->async_queue, SIGIO, POLL_IN);
>> + ? ? }
>> ?}
>
> I don't quite understand what this is doing.
This is preventing interpreting an alarm interrupt as an update interrupt.
> ...
>
> Again, not checking rtc->ops and irq_set_state shouldn't be called
> anymore, as the hrtimer handles periodic mode irq emulation now.
>
> ...
>
> Same as the last comment here. irq_set_freq is handled by generic
> emulation code now.
Ok, addressed before too.
>
>
>> diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
>> index 212b16e..d901160 100644
>> --- a/drivers/rtc/rtc-dev.c
>> +++ b/drivers/rtc/rtc-dev.c
>> @@ -261,12 +261,12 @@ static long rtc_dev_ioctl(struct file *file,
>> ? ? ? ? ? ? ? return rtc_set_time(rtc, &tm);
>>
>> ? ? ? case RTC_PIE_ON:
>> - ? ? ? ? ? ? err = rtc_irq_set_state(rtc, NULL, 1);
>> - ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock);
>> + ? ? ? ? ? ? return rtc_irq_set_state(rtc, NULL, 1);
>>
>> ? ? ? case RTC_PIE_OFF:
>> - ? ? ? ? ? ? err = rtc_irq_set_state(rtc, NULL, 0);
>> - ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock);
>> + ? ? ? ? ? ? return rtc_irq_set_state(rtc, NULL, 0);
>>
>> ? ? ? case RTC_AIE_ON:
>> ? ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock);
>> @@ -285,8 +285,8 @@ static long rtc_dev_ioctl(struct file *file,
>> ? ? ? ? ? ? ? return rtc_update_irq_enable(rtc, 0);
>>
>> ? ? ? case RTC_IRQP_SET:
>> - ? ? ? ? ? ? err = rtc_irq_set_freq(rtc, NULL, arg);
>> - ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock);
>> + ? ? ? ? ? ? return rtc_irq_set_freq(rtc, NULL, arg);
>>
>> ? ? ? case RTC_IRQP_READ:
>
> Why the unlock/return change here?
Because the mutex had to be taken inside rtc_irq_set_state() and
rtc_irq_set_freq() in order to perform the callbacks. And being entry
points, the mutex must no be taken. Breaking instead of returning
would release the mutex twice.
> If I'm being totally daft here, my apologies and please let me know. The
> rework I did was fairly large, and its quite possible there are issues
> lurking. But I'm not connecting your patch to specific breakage yet.
>
> thanks
> -john
Thanks for your comments, I will leave the rtc-driver aside, as its
old behaviour probably no longer makes sense and concentrate on the
sa1100 driver.
Regards,
Marcelo.
On Wed, 2011-02-02 at 15:58 -0200, Marcelo Roberto Jimenez wrote:
> The reason I added it before was that the user space behavior of the
> rtc-test kernel RTC driver had changed, but maybe its previous
> behavior is is no longer acceptable. Alarm interrupts were hand
> generated by the user by echoing to the sys interface, but it does not
> make sense to even have a timer in that case. That way,
> alarm_irq_enable() would bypass the kernel implementation.
>
> Also, the way it is today, the programmer implementing the driver
> _must_ define alarm_irq_enable(), even if this function does nothing,
> otherwise you get EINVAL.
>
> Maybe I got alarm_irq_enable() completely wrong and it should just be removed.
No, I think you are just bringing up some subtleties of the interface,
and clearly if the rtc-test driver is broken, that's my fault.
I've started playing with the rtc-test driver, and I am seeing some
issues there, so I'll try to see if I can't fix those quickly here.
> >> 2 - In the current configuration, an update interrupt, for example,
> >> would be a trigger for an alarm interrupt as well, even though the mode
> >> variable stated it was an update interrupt. To avoid that, mode is
> >> beeing checked inside rtc_handle_legacy_irq(). That used to be the
> >> previous user space behaviour.
> >
> > Right, we internally use recurring alarm interrupts to emulate the
> > update interrupt mode. Although I believe I provided the right behavior
> > to userland, so I'm not sure I see the issue or how your changes correct
> > things.
>
> Again, what the rtc-test kernel RTC and the strongarm RTC user space
> behavior have changed. Alarm interrupts and update interrupts were
> generated by a different interrupts in the strongarm driver, and the
> rtc-test driver also behaved similarly, i.e., an update interrupt did
> not trigger an alarm interrupt. Currently, rtc_handle_legacy_irq()
> centralizes the irq processing, and by not checking the generated
> interrupt, it allows the new behavior, which seemed broken to me.
So
> >> 3 - The irq_set_freq() and rtc_irq_set_state() functions are entry
> >> points to the RTC API, and were not calling the irq_set_state() and
> >> irq_set_freq() callbacks. These are also necessary overrides to provide
> >> a proper independent RTC implementation.
> >
> > Again, the freq is handled via a hrtimer now, so we shouldn't be calling
> > out to the hardware specific rtc driver anymore.
>
> My reasoning here goes along the same lines as in the
> alarm_irq_enable() case, so I believe those callbacks should also be
> removed, right?
>
> For rtc_irq_set_freq(), I believe that the current code allows a user
> space program to call the RTC_IRQP_SET ioclt() with freq == 0 and have
> a division by zero in the kernel code. I'll send a separate patch for
> that.
Yes. Very good catch here. Thanks for sending that in! I'll be sending
it upstream through Thomas shortly here.
> >> 4 - The rtc-test kernel RTC driver has been updated to work properly
> >> with the new kernel RTC timer infrastructure. This driver has been used
> >> together with the rtctest.c source code present in the file
> >> Documentation/rtc.txt to test the user space behaviour of all the
> >> modifications in this patch.
> >
> > So forgive me, as I might just be missing what you're describing. It
> > might be easier if you break fixes up into multiple patches?
>
> The original purpose of the patch was to bring back the previous
> rtc-test driver behaviour, I think that anything less would be to
> commit broken work in progress code.
Very much agreed.
So thanks for your more detailed explanation. Taking a look at the
rtc-test driver, I can see there's some issues in my emulation code
surrounding the split between the rtc driver ioctl and the emulated
processing (basically if the driver registers an ioctl handler, it
short-circuits the emulation code causing problems).
There's some ugliness here, as basically either the RTC driver can try
to handle all or most of the functionality through the ioctl or
implement operations and let the generic code handle the rest.
So for instance, the driver can handle RTC_AIE_ON directly in the ioctl
operation, and then not implement the alarm_irq_enable operation.
So there's a number of ways you could implement the same functionality
in the driver. I missed this inconsistency here. So I'll have to clean
that up somewhat. :(
Thanks again for pointing this issue out! I'll be sending you patches
for testing soon.
thanks
-john
On Wed, 2011-02-02 at 14:24 -0800, John Stultz wrote:
> On Wed, 2011-02-02 at 15:58 -0200, Marcelo Roberto Jimenez wrote:
> > Again, what the rtc-test kernel RTC and the strongarm RTC user space
> > behavior have changed. Alarm interrupts and update interrupts were
> > generated by a different interrupts in the strongarm driver, and the
> > rtc-test driver also behaved similarly, i.e., an update interrupt did
> > not trigger an alarm interrupt. Currently, rtc_handle_legacy_irq()
> > centralizes the irq processing, and by not checking the generated
> > interrupt, it allows the new behavior, which seemed broken to me.
>
> So
Sorry. I didn't finish my thought here. (I *did not* mean "So?" :)
So... yes, we should make sure its not broken. Could you explain some
more details about the different interrupts fro the driver? If the two
interrupt types come from different sources, is there a problem actually
using the alarm irq to emulate the update irq? In other words, can we
just skip the update irq management code? Or is there a draw back to
that?
thanks
-john
Hi John,
On Wed, Feb 2, 2011 at 20:28, john stultz <[email protected]> wrote:
> On Wed, 2011-02-02 at 14:24 -0800, John Stultz wrote:
>> On Wed, 2011-02-02 at 15:58 -0200, Marcelo Roberto Jimenez wrote:
>> > Again, what the rtc-test kernel RTC and the strongarm RTC user space
>> > behavior have changed. Alarm interrupts and update interrupts were
>> > generated by a different interrupts in the strongarm driver, and the
>> > rtc-test driver also behaved similarly, i.e., an update interrupt did
>> > not trigger an alarm interrupt. Currently, rtc_handle_legacy_irq()
>> > centralizes the irq processing, and by not checking the generated
>> > interrupt, it allows the new behavior, which seemed broken to me.
>>
>> So
>
> Sorry. I didn't finish my thought here. (I *did not* mean "So?" :)
>
> So... yes, we should make sure its not broken. Could you explain some
> more details about the different interrupts fro the driver? ?If the two
> interrupt types come from different sources, is there a problem actually
> using the alarm irq to emulate the update irq? In other words, can we
> just skip the update irq management code? Or is there a draw back to
> that?
In a real timer device, I don't see a problem, as time passes the same
for everything, including update and alarm interrupts. This is an
issue only in the rtc-test driver, which is just a debug device. Since
everything is now emulated in timers, the word IRQ does not make sense
anymore, the right thing to do would be a global rename to remove
references to that string in the RTC framework. The rtc-test driver is
based on the concept of an interrupt, which is no longer the case.
I think your implementation is fine.
> thanks
> -john
Regards,
Marcelo.