mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] RTC: rtc-dev UIE emulation
@ 2006-04-28 16:16 Atsushi Nemoto
  2006-04-29  6:23 ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Atsushi Nemoto @ 2006-04-28 16:16 UTC (permalink / raw)
  To: Alessandro Zummo; +Cc: linux-kernel, akpm

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 <anemo@mba.ocn.ne.jp>

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.
+
 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;
+	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);
+}
+
+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);
+		rtc->ttask_active = 0;
+	}
+	while (rtc->stask_active) {
+		spin_unlock_irq(&rtc->irq_lock);
+		schedule();
+		spin_lock_irq(&rtc->irq_lock);
+	}
+	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;
+	}
+}
+#endif /* CONFIG_RTC_INTF_DEV_X */
 
 static ssize_t
 rtc_dev_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
@@ -227,6 +314,15 @@ static int rtc_dev_ioctl(struct inode *i
 			return -EFAULT;
 		break;
 
+#ifdef CONFIG_RTC_INTF_DEV_X
+	case RTC_UIE_OFF:
+		clear_uie(rtc);
+		return 0;
+
+	case RTC_UIE_ON:
+	        set_uie(rtc);
+		return 0;
+#endif
 	default:
 		err = -EINVAL;
 		break;
@@ -239,6 +335,9 @@ static int rtc_dev_release(struct inode 
 {
 	struct rtc_device *rtc = to_rtc_device(file->private_data);
 
+#ifdef CONFIG_RTC_INTF_DEV_X
+	clear_uie(rtc);
+#endif
 	if (rtc->ops->release)
 		rtc->ops->release(rtc->class_dev.dev);
 
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index ab61cd1..29199c3 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -155,6 +155,15 @@ struct rtc_device
 	struct rtc_task *irq_task;
 	spinlock_t irq_task_lock;
 	int irq_freq;
+#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
 };
 #define to_rtc_device(d) container_of(d, struct rtc_device, class_dev)
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RTC: rtc-dev UIE emulation
  2006-04-28 16:16 [PATCH] RTC: rtc-dev UIE emulation Atsushi Nemoto
@ 2006-04-29  6:23 ` Andrew Morton
       [not found]   ` <20060429093108.77ced705@inspiron>
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-04-29  6:23 UTC (permalink / raw)
  To: Atsushi Nemoto; +Cc: a.zummo, linux-kernel

Atsushi Nemoto <anemo@mba.ocn.ne.jp> 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 <anemo@mba.ocn.ne.jp>
> 
> 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?


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RTC: rtc-dev UIE emulation
       [not found]   ` <20060429093108.77ced705@inspiron>
@ 2006-04-29 15:10     ` Atsushi Nemoto
  2006-05-01 14:32       ` Atsushi Nemoto
  0 siblings, 1 reply; 6+ messages in thread
From: Atsushi Nemoto @ 2006-04-29 15:10 UTC (permalink / raw)
  To: akpm; +Cc: a.zummo, linux-kernel

Thanks for your detailed review, Andrew.  Basically I just merged
genrtc's stuff, but obviously it seems there are lots of things to
fix/refine/improve.  I'll try to do but it will take some times.

Alessandro, following comments are against for this patch, right?

On Sat, 29 Apr 2006 09:31:08 +0200, Alessandro Zummo <alessandro.zummo@towertech.it> wrote:
>   this patch will conflict with rtc drivers that have proper UIE
>  support, please remove it from the tree.
> 
>   A generic UIE emulation should at least check if the ioctl
>  has not been already handled by the underlaying rtc driver.
> 
>   That means that every driver should be modified to return
>  -ENOIOCTLCMD if it gets passed an unknown IOCTL and that
>  this patch should check for this code before trying to emulate.

Since rtc_dev_ioctl() calls underlaying rtc driver's ioctl() first and
checks -EINVAL, so I think it will not conflict.  Is it wrong?

---
Atsushi Nemoto

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RTC: rtc-dev UIE emulation
  2006-04-29 15:10     ` Atsushi Nemoto
@ 2006-05-01 14:32       ` Atsushi Nemoto
  2006-05-02 20:15         ` Alessandro Zummo
  0 siblings, 1 reply; 6+ messages in thread
From: Atsushi Nemoto @ 2006-05-01 14:32 UTC (permalink / raw)
  To: akpm; +Cc: a.zummo, linux-kernel

On Sun, 30 Apr 2006 00:10:03 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> Thanks for your detailed review, Andrew.  Basically I just merged
> genrtc's stuff, but obviously it seems there are lots of things to
> fix/refine/improve.  I'll try to do but it will take some times.

Here is an updated patch.  I think this one reflects all suggestions
by Andrew.


Import genrtc's RTC UIE emulation (CONFIG_GEN_RTC_X) to rtc-dev driver
with slight adjustments/refinements.  This makes UIE-less rtc drivers
work better with programs doing read/poll on /dev/rtc, such as
hwclock.  This emulation should not harm rtc drivers with UIE support,
since rtc_dev_ioctl() calls underlaying rtc driver's ioctl() first.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 65d090d..2664dd6 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -73,6 +73,14 @@ 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_UIE_EMUL
+	bool "RTC UIE emulation on dev interface"
+	depends on RTC_INTF_DEV
+	help
+	  Provides an emulation for RTC_UIE if the underlaying rtc chip
+	  driver did not provides RTC_UIE ioctls.  RTC_UIE is required
+	  by some programs, such as hwclock.
+
 comment "RTC drivers"
 	depends on RTC_CLASS
 
diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index b1e3e61..d87348c 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_UIE_EMUL
+/*
+ * 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;
+	int num = 0;
+	int err;
+
+	err = rtc_read_time(&rtc->class_dev, &tm);
+	spin_lock_irq(&rtc->irq_lock);
+	if (rtc->stop_uie_polling || err) {
+		rtc->uie_task_active = 0;
+	} else if (rtc->oldsecs != tm.tm_sec) {
+		num = (tm.tm_sec + 60 - rtc->oldsecs) % 60;
+		rtc->oldsecs = tm.tm_sec;
+		rtc->uie_timer.expires = jiffies + HZ - (HZ/10);
+		rtc->uie_timer_active = 1;
+		rtc->uie_task_active = 0;
+		add_timer(&rtc->uie_timer);
+	} else if (schedule_work(&rtc->uie_task) == 0) {
+		rtc->uie_task_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->uie_timer_active = 0;
+	rtc->uie_task_active = 1;
+	if ((schedule_work(&rtc->uie_task) == 0))
+		rtc->uie_task_active = 0;
+	spin_unlock_irqrestore(&rtc->irq_lock, flags);
+}
+
+static void clear_uie(struct rtc_device *rtc)
+{
+	spin_lock_irq(&rtc->irq_lock);
+	if (rtc->irq_active) {
+		rtc->stop_uie_polling = 1;
+		if (rtc->uie_timer_active) {
+			spin_unlock_irq(&rtc->irq_lock);
+			del_timer_sync(&rtc->uie_timer);
+			spin_lock_irq(&rtc->irq_lock);
+			rtc->uie_timer_active = 0;
+		}
+		if (rtc->uie_task_active) {
+			spin_unlock_irq(&rtc->irq_lock);
+			flush_scheduled_work();
+			spin_lock_irq(&rtc->irq_lock);
+		}
+		rtc->irq_active = 0;
+	}
+	spin_unlock_irq(&rtc->irq_lock);
+}
+
+static int set_uie(struct rtc_device *rtc)
+{
+	struct rtc_time tm;
+	int err;
+
+	err = rtc_read_time(&rtc->class_dev, &tm);
+	if (err)
+		return err;
+	spin_lock_irq(&rtc->irq_lock);
+	if (!rtc->irq_active) {
+		rtc->irq_active = 1;
+		rtc->stop_uie_polling = 0;
+		rtc->oldsecs = tm.tm_sec;
+		rtc->uie_task_active = 1;
+		if (schedule_work(&rtc->uie_task) == 0)
+			rtc->uie_task_active = 0;
+	}
+	rtc->irq_data = 0;
+	spin_unlock_irq(&rtc->irq_lock);
+	return 0;
+}
+#endif /* CONFIG_RTC_INTF_DEV_UIE_EMUL */
 
 static ssize_t
 rtc_dev_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
@@ -227,6 +314,14 @@ static int rtc_dev_ioctl(struct inode *i
 			return -EFAULT;
 		break;
 
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	case RTC_UIE_OFF:
+		clear_uie(rtc);
+		return 0;
+
+	case RTC_UIE_ON:
+		return set_uie(rtc);
+#endif
 	default:
 		err = -EINVAL;
 		break;
@@ -239,6 +334,9 @@ static int rtc_dev_release(struct inode 
 {
 	struct rtc_device *rtc = to_rtc_device(file->private_data);
 
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	clear_uie(rtc);
+#endif
 	if (rtc->ops->release)
 		rtc->ops->release(rtc->class_dev.dev);
 
@@ -279,6 +377,14 @@ static int rtc_dev_add_device(struct cla
 	mutex_init(&rtc->char_lock);
 	spin_lock_init(&rtc->irq_lock);
 	init_waitqueue_head(&rtc->irq_queue);
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	INIT_WORK(&rtc->uie_task, rtc_uie_task, rtc);
+	setup_timer(&rtc->uie_timer, rtc_uie_timer, (unsigned long)rtc);
+	rtc->irq_active = 0;
+	rtc->stop_uie_polling = 0;
+	rtc->uie_task_active = 0;
+	rtc->uie_timer_active = 0;
+#endif
 
 	cdev_init(&rtc->char_dev, &rtc_dev_fops);
 	rtc->char_dev.owner = rtc->owner;
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index ab61cd1..4331076 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -155,6 +155,16 @@ struct rtc_device
 	struct rtc_task *irq_task;
 	spinlock_t irq_task_lock;
 	int irq_freq;
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	struct work_struct uie_task;
+	struct timer_list uie_timer;
+	/* Those fields are protected by rtc->irq_lock */
+	unsigned int oldsecs;
+	unsigned int irq_active:1;
+	unsigned int stop_uie_polling:1;
+	unsigned int uie_task_active:1;
+	unsigned int uie_timer_active:1;
+#endif
 };
 #define to_rtc_device(d) container_of(d, struct rtc_device, class_dev)
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RTC: rtc-dev UIE emulation
  2006-05-01 14:32       ` Atsushi Nemoto
@ 2006-05-02 20:15         ` Alessandro Zummo
  2006-05-03 16:43           ` Atsushi Nemoto
  0 siblings, 1 reply; 6+ messages in thread
From: Alessandro Zummo @ 2006-05-02 20:15 UTC (permalink / raw)
  To: Atsushi Nemoto; +Cc: akpm, a.zummo, linux-kernel

On Mon, 01 May 2006 23:32:42 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:

> Here is an updated patch.  I think this one reflects all suggestions
> by Andrew.

 seems ok to me, just a few comments:


> +	  driver did not provides RTC_UIE ioctls.  RTC_UIE is required
> +	  by some programs, such as hwclock.

 please fix the double spacing and s/provides/provide/ 

 hwclock will be fixed to not rely on uie anymore anyway.


> +#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
> +	INIT_WORK(&rtc->uie_task, rtc_uie_task, rtc);
> +	setup_timer(&rtc->uie_timer, rtc_uie_timer, (unsigned long)rtc);
> +	rtc->irq_active = 0;
> +	rtc->stop_uie_polling = 0;
> +	rtc->uie_task_active = 0;
> +	rtc->uie_timer_active = 0;
> +#endif

 the rtc struct is allocated via kzalloc, so 
 you don't need to zero it.



-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RTC: rtc-dev UIE emulation
  2006-05-02 20:15         ` Alessandro Zummo
@ 2006-05-03 16:43           ` Atsushi Nemoto
  0 siblings, 0 replies; 6+ messages in thread
From: Atsushi Nemoto @ 2006-05-03 16:43 UTC (permalink / raw)
  To: alessandro.zummo; +Cc: akpm, a.zummo, linux-kernel

On Tue, 2 May 2006 22:15:35 +0200, Alessandro Zummo <alessandro.zummo@towertech.it> wrote:
> > Here is an updated patch.  I think this one reflects all suggestions
> > by Andrew.
> 
>  seems ok to me, just a few comments:

Thank you for comments.  Here is an updated patch.  Kconfig help-text
fixes and initialization cleanup.


Import genrtc's RTC UIE emulation (CONFIG_GEN_RTC_X) to rtc-dev driver
with slight adjustments/refinements.  This makes UIE-less rtc drivers
work better with programs doing read/poll on /dev/rtc.  This emulation
should not harm rtc drivers with UIE support, since rtc_dev_ioctl()
calls underlaying rtc driver's ioctl() first.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 65d090d..6d3ecf2 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -73,6 +73,13 @@ 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_UIE_EMUL
+	bool "RTC UIE emulation on dev interface"
+	depends on RTC_INTF_DEV
+	help
+	  Provides an emulation for RTC_UIE if the underlaying rtc chip
+	  driver did not provide RTC_UIE ioctls.
+
 comment "RTC drivers"
 	depends on RTC_CLASS
 
diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index 6c9ad92..12893b7 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_UIE_EMUL
+/*
+ * 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;
+	int num = 0;
+	int err;
+
+	err = rtc_read_time(&rtc->class_dev, &tm);
+	spin_lock_irq(&rtc->irq_lock);
+	if (rtc->stop_uie_polling || err) {
+		rtc->uie_task_active = 0;
+	} else if (rtc->oldsecs != tm.tm_sec) {
+		num = (tm.tm_sec + 60 - rtc->oldsecs) % 60;
+		rtc->oldsecs = tm.tm_sec;
+		rtc->uie_timer.expires = jiffies + HZ - (HZ/10);
+		rtc->uie_timer_active = 1;
+		rtc->uie_task_active = 0;
+		add_timer(&rtc->uie_timer);
+	} else if (schedule_work(&rtc->uie_task) == 0) {
+		rtc->uie_task_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->uie_timer_active = 0;
+	rtc->uie_task_active = 1;
+	if ((schedule_work(&rtc->uie_task) == 0))
+		rtc->uie_task_active = 0;
+	spin_unlock_irqrestore(&rtc->irq_lock, flags);
+}
+
+static void clear_uie(struct rtc_device *rtc)
+{
+	spin_lock_irq(&rtc->irq_lock);
+	if (rtc->irq_active) {
+		rtc->stop_uie_polling = 1;
+		if (rtc->uie_timer_active) {
+			spin_unlock_irq(&rtc->irq_lock);
+			del_timer_sync(&rtc->uie_timer);
+			spin_lock_irq(&rtc->irq_lock);
+			rtc->uie_timer_active = 0;
+		}
+		if (rtc->uie_task_active) {
+			spin_unlock_irq(&rtc->irq_lock);
+			flush_scheduled_work();
+			spin_lock_irq(&rtc->irq_lock);
+		}
+		rtc->irq_active = 0;
+	}
+	spin_unlock_irq(&rtc->irq_lock);
+}
+
+static int set_uie(struct rtc_device *rtc)
+{
+	struct rtc_time tm;
+	int err;
+
+	err = rtc_read_time(&rtc->class_dev, &tm);
+	if (err)
+		return err;
+	spin_lock_irq(&rtc->irq_lock);
+	if (!rtc->irq_active) {
+		rtc->irq_active = 1;
+		rtc->stop_uie_polling = 0;
+		rtc->oldsecs = tm.tm_sec;
+		rtc->uie_task_active = 1;
+		if (schedule_work(&rtc->uie_task) == 0)
+			rtc->uie_task_active = 0;
+	}
+	rtc->irq_data = 0;
+	spin_unlock_irq(&rtc->irq_lock);
+	return 0;
+}
+#endif /* CONFIG_RTC_INTF_DEV_UIE_EMUL */
 
 static ssize_t
 rtc_dev_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
@@ -232,6 +319,14 @@ static int rtc_dev_ioctl(struct inode *i
 			return -EFAULT;
 		break;
 
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	case RTC_UIE_OFF:
+		clear_uie(rtc);
+		return 0;
+
+	case RTC_UIE_ON:
+		return set_uie(rtc);
+#endif
 	default:
 		err = -EINVAL;
 		break;
@@ -244,6 +339,9 @@ static int rtc_dev_release(struct inode 
 {
 	struct rtc_device *rtc = to_rtc_device(file->private_data);
 
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	clear_uie(rtc);
+#endif
 	if (rtc->ops->release)
 		rtc->ops->release(rtc->class_dev.dev);
 
@@ -284,6 +382,10 @@ static int rtc_dev_add_device(struct cla
 	mutex_init(&rtc->char_lock);
 	spin_lock_init(&rtc->irq_lock);
 	init_waitqueue_head(&rtc->irq_queue);
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	INIT_WORK(&rtc->uie_task, rtc_uie_task, rtc);
+	setup_timer(&rtc->uie_timer, rtc_uie_timer, (unsigned long)rtc);
+#endif
 
 	cdev_init(&rtc->char_dev, &rtc_dev_fops);
 	rtc->char_dev.owner = rtc->owner;
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index ab61cd1..4331076 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -155,6 +155,16 @@ struct rtc_device
 	struct rtc_task *irq_task;
 	spinlock_t irq_task_lock;
 	int irq_freq;
+#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
+	struct work_struct uie_task;
+	struct timer_list uie_timer;
+	/* Those fields are protected by rtc->irq_lock */
+	unsigned int oldsecs;
+	unsigned int irq_active:1;
+	unsigned int stop_uie_polling:1;
+	unsigned int uie_task_active:1;
+	unsigned int uie_timer_active:1;
+#endif
 };
 #define to_rtc_device(d) container_of(d, struct rtc_device, class_dev)
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2006-05-03 16:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-04-28 16:16 [PATCH] RTC: rtc-dev UIE emulation Atsushi Nemoto
2006-04-29  6:23 ` Andrew Morton
     [not found]   ` <20060429093108.77ced705@inspiron>
2006-04-29 15:10     ` Atsushi Nemoto
2006-05-01 14:32       ` Atsushi Nemoto
2006-05-02 20:15         ` Alessandro Zummo
2006-05-03 16:43           ` Atsushi Nemoto

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®