Atsushi Nemoto <[email protected]> wrote:
>
> Import genrtc's RTC UIE emulation (CONFIG_GEN_RTC_X) to rtc-dev driver
> with slight adjustments. This makes UIE-less chips/drivers work
> better with programs doing read/poll on /dev/rtc, such as hwclock.
>
> Signed-off-by: Atsushi Nemoto <[email protected]>
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 65d090d..29ca46c 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -73,6 +73,12 @@ config RTC_INTF_DEV
> This driver can also be built as a module. If so, the module
> will be called rtc-dev.
>
> +config RTC_INTF_DEV_X
> + bool "Extended RTC operation"
> + depends on RTC_INTF_DEV
> + help
> + Provides an emulation for RTC_UIE.
> +
That help is somewhat terse. A user might have trouble working out what it
does, and whether it's something they want to use.
> comment "RTC drivers"
> depends on RTC_CLASS
>
> diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
> index b1e3e61..0bbd181 100644
> --- a/drivers/rtc/rtc-dev.c
> +++ b/drivers/rtc/rtc-dev.c
> @@ -48,6 +48,93 @@ static int rtc_dev_open(struct inode *in
> return err;
> }
>
> +#ifdef CONFIG_RTC_INTF_DEV_X
> +/*
> + * Routine to poll RTC seconds field for change as often as possible,
> + * after first RTC_UIE use timer to reduce polling
> + */
> +static void rtc_uie_task(void *data)
> +{
> + struct rtc_device *rtc = data;
> + struct rtc_time tm;
> + unsigned int tmp = rtc_read_time(&rtc->class_dev, &tm) ? 0 : tm.tm_sec;
If rtc_read_time() fails we proceed as if it returned 0.
Are you sure that's correct?
(In practice, it cannot fail. Or, if it does, it'll fail 100% of the time.
But still...)
> + int num = 0;
> +
> + spin_lock_irq(&rtc->irq_lock);
> + if (rtc->stop_rtc_timers) {
> + rtc->stask_active = 0;
> + spin_unlock_irq(&rtc->irq_lock);
> + return;
> + }
> +
> + if (rtc->oldsecs != tmp) {
> + num = (tmp + 60 - rtc->oldsecs) % 60;
> + rtc->oldsecs = tmp;
> +
> + rtc->timer_task.expires = jiffies + HZ - (HZ/10);
> + rtc->ttask_active = 1;
> + rtc->stask_active = 0;
> + add_timer(&rtc->timer_task);
> + } else if (schedule_work(&rtc->uie_task) == 0)
> + rtc->stask_active = 0;
> + spin_unlock_irq(&rtc->irq_lock);
> + if (num)
> + rtc_update_irq(&rtc->class_dev, num, RTC_UF | RTC_IRQF);
> +}
> +
> +static void rtc_uie_timer(unsigned long data)
> +{
> + struct rtc_device *rtc = (struct rtc_device *)data;
> + unsigned long flags;
> + spin_lock_irqsave(&rtc->irq_lock, flags);
> + rtc->ttask_active = 0;
> + rtc->stask_active = 1;
> + if ((schedule_work(&rtc->uie_task) == 0))
> + rtc->stask_active = 0;
> + spin_unlock_irqrestore(&rtc->irq_lock, flags);
> +}
I see a schedule_work(), but I don't see a flush_scheduled_work(). Is
there anything preventing the scheduled work from still being pending after a
close() or an rmmod?
> +static void clear_uie(struct rtc_device *rtc)
> +{
> + spin_lock_irq(&rtc->irq_lock);
> + rtc->stop_rtc_timers = 1;
> + if (rtc->ttask_active) {
> + spin_unlock_irq(&rtc->irq_lock);
> + del_timer_sync(&rtc->timer_task);
> + spin_lock(&rtc->irq_lock);
Presumably that should be spin_lock_irq().
> + rtc->ttask_active = 0;
> + }
> + while (rtc->stask_active) {
> + spin_unlock_irq(&rtc->irq_lock);
> + schedule();
> + spin_lock_irq(&rtc->irq_lock);
> + }
That's a busywait. Please let's find a better way of doing this.
That way might be a flush_scheduled_work(). I don't know, because it's not
immediately clear what the responsibilities of this function are. Please
add some comments to the code which explain things like this.
> + rtc->irq_active = 0;
> + spin_unlock_irq(&rtc->irq_lock);
> +}
> +
> +static void set_uie(struct rtc_device *rtc)
> +{
> + int start = 0;
> + spin_lock_irq(&rtc->irq_lock);
> + if (!rtc->irq_active)
> + start = rtc->irq_active = 1;
> + rtc->irq_data = 0;
> + spin_unlock_irq(&rtc->irq_lock);
> + if (start) {
> + struct rtc_time tm;
> + rtc->stop_rtc_timers = 0;
> + INIT_WORK(&rtc->uie_task, rtc_uie_task, rtc);
> + rtc->oldsecs = rtc_read_time(&rtc->class_dev, &tm) ?
> + 0 : tm.tm_sec;
> + setup_timer(&rtc->timer_task, rtc_uie_timer,
> + (unsigned long)rtc);
> + rtc->stask_active = 1;
> + if (schedule_work(&rtc->uie_task) == 0)
> + rtc->stask_active = 0;
> + }
> +}
Sometimes you have a blank line after the the declarations of the local
variables, sometimes not. I prefer it to be there, personally.
The above code is slightly racy,
> +#ifdef CONFIG_RTC_INTF_DEV_X
> + struct work_struct uie_task;
> + struct timer_list timer_task;
> + unsigned int oldsecs;
> + unsigned int irq_active :1;
> + unsigned int stop_rtc_timers :1; /* don't requeue tasks */
> + unsigned int stask_active :1; /* schedule_work */
> + unsigned int ttask_active :1; /* timer_task */
> +#endif
because all these bitfields will occupy the same machine word. We must
provide locking for that word, because the compiler won't do it.
Generally, rtc->irq_lock does provide that locking. But not in the above
case - it's conceivable that the rtc_uie_task() callback will be executing
while this CPU is modifying rtc->stask_active.
A suitable fix would be to extend the rtc->irq_lock coverage here. Plus,
of course, adding a comment above those four fields explaining what their
locking protocol is.
Also,
unsigned int ttask_active:1;
is more conventional whitespace usage.
"timer_task" is a rather misleading name for a timer. It implies that
it's, umm, a task. "uie_timer", perhaps?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
