[PATCH] hpet: factor timer allocate from open

[PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

The current implementation of the /dev/hpet driver couples opening the
device with allocating one of the (scarce) timers (aka comparators).
This is a limitation in that the main counter may be valuable to
applications seeking a high-resolution timer who have no use for the
interrupt generating functionality of the comparators.

This patch alters the open semantics so that when the device is
opened, no timer is allocated. Operations that depend on a timer being
in context implicitly attempt allocating a timer, to maintain backward
compatibility. There is also an IOCTL (HPET_ALLOC_TIMER _IO) added so
that the allocation may be done explicitly. (I prefer the explicit
open then allocate pattern but don't know how practical it would be to
require all existing code to be changed.)

I also have changes for adding IOCTLs to get the main counter
value--which is more straightforward than mmaping and reading the
registers directly, and possibly necessary if mmap is ifdef'd
away--but have separated that logically into another patch. I'll wait
to post that until any issues with this are hashed out.

Signed-off-by: Magnus Lynch <maglyx@gmail.com>
---
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e481c59..8991227 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -244,16 +244,40 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)

 static int hpet_open(struct inode *inode, struct file *file)
 {
-       struct hpet_dev *devp;
       struct hpets *hpetp;
-       int i;

       if (file->f_mode & FMODE_WRITE)
               return -EINVAL;

+       hpetp = hpets;
+       /* starting with timer-neutral instance */
+       file->private_data = &hpetp->hp_dev[hpetp->hp_ntimer];
+
+       return 0;
+}
+
+static int hpet_alloc_timer(struct file *file)
+{
+       struct hpet_dev *devp;
+       struct hpets *hpetp;
+       int i;
+
+       /* once acquired, will remain */
+       devp = file->private_data;
+       if (devp->hd_timer)
+               return 0;
+
       lock_kernel();
       spin_lock_irq(&hpet_lock);

+       /* check for race acquiring */
+       devp = file->private_data;
+       if (devp->hd_timer) {
+               spin_unlock_irq(&hpet_lock);
+               unlock_kernel();
+               return 0;
+       }
+
       for (devp = NULL, hpetp = hpets; hpetp && !devp; hpetp = hpetp->hp_next)
               for (i = 0; i < hpetp->hp_ntimer; i++)
                       if (hpetp->hp_dev[i].hd_flags & HPET_OPEN)
@@ -384,6 +408,10 @@ static int hpet_fasync(int fd, struct file *file, int on)
 {
       struct hpet_dev *devp;

+       int r = hpet_alloc_timer(file);
+       if (r < 0)
+               return r;
+
       devp = file->private_data;

       if (fasync_helper(fd, file, on, &devp->hd_async_queue) >= 0)
@@ -401,6 +429,9 @@ static int hpet_release(struct inode *inode,
struct file *file)
       devp = file->private_data;
       timer = devp->hd_timer;

+       if (!timer)
+               goto out;
+
       spin_lock_irq(&hpet_lock);

       writeq((readq(&timer->hpet_config) & ~Tn_INT_ENB_CNF_MASK),
@@ -425,7 +456,7 @@ static int hpet_release(struct inode *inode,
struct file *file)

       if (irq)
               free_irq(irq, devp);
-
+out:
       file->private_data = NULL;
       return 0;
 }
@@ -438,6 +469,17 @@ hpet_ioctl(struct inode *inode, struct file
*file, unsigned int cmd,
 {
       struct hpet_dev *devp;

+       switch (cmd) {
+       case HPET_INFO:
+               break;
+       default:
+               {
+                       int r = hpet_alloc_timer(file);
+                       if (r < 0)
+                               return r;
+               }
+       }
+
       devp = file->private_data;
       return hpet_ioctl_common(devp, cmd, arg, 0);
 }
@@ -570,6 +612,9 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd,
unsigned long arg, int kernel)
               break;
       case HPET_IE_ON:
               return hpet_ioctl_ieon(devp);
+       case HPET_ALLOC_TIMER:
+               /* nothing to do */
+               return 0;
       default:
               return -EINVAL;
       }
@@ -598,10 +643,16 @@ hpet_ioctl_common(struct hpet_dev *devp, int
cmd, unsigned long arg, int kernel)
                                       hpet_time_div(hpetp, devp->hd_ireqfreq);
                       else
                               info.hi_ireqfreq = 0;
-                       info.hi_flags =
-                           readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
+                       if (timer) {
+                               info.hi_flags =
+                                       readq(&timer->hpet_config) &
+                                       Tn_PER_INT_CAP_MASK;
+                               info.hi_timer = devp - hpetp->hp_dev;
+                       } else {
+                               info.hi_flags = 0;
+                               info.hi_timer = -1;
+                       }
                       info.hi_hpet = hpetp->hp_which;
-                       info.hi_timer = devp - hpetp->hp_dev;
                       if (kernel)
                               memcpy((void *)arg, &info, sizeof(info));
                       else
@@ -794,7 +845,11 @@ int hpet_alloc(struct hpet_data *hdp)
               return 0;
       }

-       siz = sizeof(struct hpets) + ((hdp->hd_nirqs - 1) *
+       /*
+        * last hpet_dev will have null timer pointer, gives timer-neutral
+        * representation of block
+        */
+       siz = sizeof(struct hpets) + ((hdp->hd_nirqs) *
                                     sizeof(struct hpet_dev));

       hpetp = kzalloc(siz, GFP_KERNEL);
@@ -860,13 +915,16 @@ int hpet_alloc(struct hpet_data *hdp)
               writeq(mcfg, &hpet->hpet_config);
       }

-       for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++, devp++) {
+       for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer + 1;
+            i++, devp++) {
               struct hpet_timer __iomem *timer;

-               timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
-
               devp->hd_hpets = hpetp;
               devp->hd_hpet = hpet;
+               if (i == hpetp->hp_ntimer)
+                       continue;
+
+               timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
               devp->hd_timer = timer;

               /*
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 219ca4f..d690c0f 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -125,6 +125,7 @@ struct hpet_info {
 #define        HPET_EPI        _IO('h', 0x04)  /* enable periodic */
 #define        HPET_DPI        _IO('h', 0x05)  /* disable periodic */
 #define        HPET_IRQFREQ    _IOW('h', 0x6, unsigned long)   /*
IRQFREQ usec */
+#define        HPET_ALLOC_TIMER _IO('h', 0x7)

 #define MAX_HPET_TBS   8               /* maximum hpet timer blocks */

Re: [PATCH] hpet: factor timer allocate from open

[Clemens Ladisch]

Magnus Lynch wrote:
> The current implementation of the /dev/hpet driver couples opening the
> device with allocating one of the (scarce) timers (aka comparators).
> This is a limitation in that the main counter may be valuable to
> applications seeking a high-resolution timer who have no use for the
> interrupt generating functionality of the comparators.
> 
> This patch alters the open semantics so that when the device is
> opened, no timer is allocated. Operations that depend on a timer being
> in context implicitly attempt allocating a timer, to maintain backward
> compatibility.

Many thanks; I had planned to do something like this, but never found
the time.

> I also have changes for adding IOCTLs to get the main counter
> value--which is more straightforward than mmaping and reading the
> registers directly, and possibly necessary if mmap is ifdef'd
> away--but have separated that logically into another patch.

This driver was written before the high-resolution timer API was
available, so the only actual use case, besides backwards compatibility,
is the ability to read the timer register directly without a syscall.

A ioctl to read the main counter would just duplicate clock_gettime(),
but I cannot see any benefit over that.

> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c

Your mailer killed tabs and wrapped lines.

> @@ -438,6 +469,17 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>  {
>        struct hpet_dev *devp;
> 
> +       switch (cmd) {
> +       case HPET_INFO:
> +               break;
> +       default:
> +               {
> +                       int r = hpet_alloc_timer(file);

Applications might want to use HPET_INFO to find out which timer they
got, so I think the driver cannot avoid allocating a timer in this case.


Regards,
Clemens

Re: [PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

Clemens Ladisch wrote:
> Many thanks; I had planned to do something like this, but never found
> the time.

You're welcome. It's not completely coincidental: I read an old discussion
you had with someone else trying to solve this problem and this is the method
you recommended.

> This driver was written before the high-resolution timer API was
> available, so the only actual use case, besides backwards compatibility,
> is the ability to read the timer register directly without a syscall.
>
> A ioctl to read the main counter would just duplicate clock_gettime(),
> but I cannot see any benefit over that.

In fact for my situation I attempted using clock_gettime first and found it
unsuitable. Specifically, my case is finding a substitute for RDTSC as a
constant-rate counter for use in correcting real-time clock drift. This is for
a CPU without constant TSC, as many are; and I was patching DJ Bernstein's
excellent program clockspeed to be workable in such a case.

Calculating the real time tick rate of clock_gettime using CLOCK_MONOTONIC
suspiciously yielded a rate almost exactly 1GHz, which seemed to imply some
feedback relative to real time was happening and thus wouldn't be reliably
constant rate in the face of slewing the clock. Whatever the reason, in fact
it wasn't and trying to depend on it as a constant rate timer while using
adjtime quickly led to inaccuracies... Which led me to HPET as a solution,
which worked swimmingly.

I do now see there's another clockid, CLOCK_MONOTONIC_RAW, whose name implies
it might work for me. I'll try it and see.

> Your mailer killed tabs and wrapped lines.

Whoops. Here I'm resubmitting the patch, avoiding the Gmail web client.

Signed-off-by: Magnus Lynch <maglyx@gmail.com>

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e481c59..8991227 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -244,16 +244,40 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
 
 static int hpet_open(struct inode *inode, struct file *file)
 {
-	struct hpet_dev *devp;
 	struct hpets *hpetp;
-	int i;
 
 	if (file->f_mode & FMODE_WRITE)
 		return -EINVAL;
 
+	hpetp = hpets;
+	/* starting with timer-neutral instance */
+	file->private_data = &hpetp->hp_dev[hpetp->hp_ntimer];
+
+	return 0;
+}
+
+static int hpet_alloc_timer(struct file *file)
+{
+	struct hpet_dev *devp;
+	struct hpets *hpetp;
+	int i;
+
+	/* once acquired, will remain */
+	devp = file->private_data;
+	if (devp->hd_timer)
+		return 0;
+
 	lock_kernel();
 	spin_lock_irq(&hpet_lock);
 
+	/* check for race acquiring */
+	devp = file->private_data;
+	if (devp->hd_timer) {
+		spin_unlock_irq(&hpet_lock);
+		unlock_kernel();
+		return 0;
+	}
+
 	for (devp = NULL, hpetp = hpets; hpetp && !devp; hpetp = hpetp->hp_next)
 		for (i = 0; i < hpetp->hp_ntimer; i++)
 			if (hpetp->hp_dev[i].hd_flags & HPET_OPEN)
@@ -384,6 +408,10 @@ static int hpet_fasync(int fd, struct file *file, int on)
 {
 	struct hpet_dev *devp;
 
+	int r = hpet_alloc_timer(file);
+	if (r < 0)
+		return r;
+
 	devp = file->private_data;
 
 	if (fasync_helper(fd, file, on, &devp->hd_async_queue) >= 0)
@@ -401,6 +429,9 @@ static int hpet_release(struct inode *inode, struct file *file)
 	devp = file->private_data;
 	timer = devp->hd_timer;
 
+	if (!timer)
+		goto out;
+
 	spin_lock_irq(&hpet_lock);
 
 	writeq((readq(&timer->hpet_config) & ~Tn_INT_ENB_CNF_MASK),
@@ -425,7 +456,7 @@ static int hpet_release(struct inode *inode, struct file *file)
 
 	if (irq)
 		free_irq(irq, devp);
-
+out:
 	file->private_data = NULL;
 	return 0;
 }
@@ -438,6 +469,17 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 {
 	struct hpet_dev *devp;
 
+	switch (cmd) {
+	case HPET_INFO:
+		break;
+	default:
+		{
+			int r = hpet_alloc_timer(file);
+			if (r < 0)
+				return r;
+		}
+	}
+
 	devp = file->private_data;
 	return hpet_ioctl_common(devp, cmd, arg, 0);
 }
@@ -570,6 +612,9 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg, int kernel)
 		break;
 	case HPET_IE_ON:
 		return hpet_ioctl_ieon(devp);
+	case HPET_ALLOC_TIMER:
+		/* nothing to do */
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -598,10 +643,16 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg, int kernel)
 					hpet_time_div(hpetp, devp->hd_ireqfreq);
 			else
 				info.hi_ireqfreq = 0;
-			info.hi_flags =
-			    readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
+			if (timer) {
+				info.hi_flags =
+					readq(&timer->hpet_config) &
+					Tn_PER_INT_CAP_MASK;
+				info.hi_timer = devp - hpetp->hp_dev;
+			} else {
+				info.hi_flags = 0;
+				info.hi_timer = -1;
+			}
 			info.hi_hpet = hpetp->hp_which;
-			info.hi_timer = devp - hpetp->hp_dev;
 			if (kernel)
 				memcpy((void *)arg, &info, sizeof(info));
 			else
@@ -794,7 +845,11 @@ int hpet_alloc(struct hpet_data *hdp)
 		return 0;
 	}
 
-	siz = sizeof(struct hpets) + ((hdp->hd_nirqs - 1) *
+	/*
+	 * last hpet_dev will have null timer pointer, gives timer-neutral
+	 * representation of block
+	 */
+	siz = sizeof(struct hpets) + ((hdp->hd_nirqs) *
 				      sizeof(struct hpet_dev));
 
 	hpetp = kzalloc(siz, GFP_KERNEL);
@@ -860,13 +915,16 @@ int hpet_alloc(struct hpet_data *hdp)
 		writeq(mcfg, &hpet->hpet_config);
 	}
 
-	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++, devp++) {
+	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer + 1;
+	     i++, devp++) {
 		struct hpet_timer __iomem *timer;
 
-		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
-
 		devp->hd_hpets = hpetp;
 		devp->hd_hpet = hpet;
+		if (i == hpetp->hp_ntimer)
+			continue;
+
+		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
 		devp->hd_timer = timer;
 
 		/*
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 219ca4f..d690c0f 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -125,6 +125,7 @@ struct hpet_info {
 #define	HPET_EPI	_IO('h', 0x04)	/* enable periodic */
 #define	HPET_DPI	_IO('h', 0x05)	/* disable periodic */
 #define	HPET_IRQFREQ	_IOW('h', 0x6, unsigned long)	/* IRQFREQ usec */
+#define	HPET_ALLOC_TIMER _IO('h', 0x7)
 
 #define MAX_HPET_TBS	8		/* maximum hpet timer blocks */
 

Re: [PATCH] hpet: factor timer allocate from open

[john stultz]

On Mon, Mar 1, 2010 at 12:24 PM, Magnus Lynch <maglyx@gmail.com> wrote:
> Clemens Ladisch wrote:
>> A ioctl to read the main counter would just duplicate clock_gettime(),
>> but I cannot see any benefit over that.
>
> In fact for my situation I attempted using clock_gettime first and found it
> unsuitable. Specifically, my case is finding a substitute for RDTSC as a
> constant-rate counter for use in correcting real-time clock drift. This is for
> a CPU without constant TSC, as many are; and I was patching DJ Bernstein's
> excellent program clockspeed to be workable in such a case.
>
> Calculating the real time tick rate of clock_gettime using CLOCK_MONOTONIC
> suspiciously yielded a rate almost exactly 1GHz, which seemed to imply some
> feedback relative to real time was happening and thus wouldn't be reliably
> constant rate in the face of slewing the clock. Whatever the reason, in fact
> it wasn't and trying to depend on it as a constant rate timer while using
> adjtime quickly led to inaccuracies... Which led me to HPET as a solution,
> which worked swimmingly.
>
> I do now see there's another clockid, CLOCK_MONOTONIC_RAW, whose name implies
> it might work for me. I'll try it and see.

Yes, CLOCK_MONOTONIC_RAW was added specifically to create a hardware
agnostic interface that provided a 1:1 ratio to the hardware cycle
counter used by the timekeeping core. No NTP corrections or slewing
are applied and it isn't affected by settimeofday calls.

Let me know if you run into any trouble with it.
thanks
-john

Re: [PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

On Mon, Mar 1, 2010 at 12:59 PM, john stultz <johnstul@us.ibm.com> wrote:
> Yes, CLOCK_MONOTONIC_RAW was added specifically to create a hardware
> agnostic interface that provided a 1:1 ratio to the hardware cycle
> counter used by the timekeeping core. No NTP corrections or slewing
> are applied and it isn't affected by settimeofday calls.
>
> Let me know if you run into any trouble with it.
> thanks

Actually I did run into trouble. CLOCK_MONOTONIC_RAW is defined in
linux/time.h and including that creates a lot of redefinition
conflicts with time.h. I see the linux/ includes aren't intended for
user space code.

Is this clockid meant to be of use to user space code and if so how
would one include the definition successfully? time.h is virtually
impossible to not have included if you're using most anything.

Re: [PATCH] hpet: factor timer allocate from open

[john stultz]

On Wed, 2010-03-03 at 01:07 -0800, Magnus Lynch wrote:
> On Mon, Mar 1, 2010 at 12:59 PM, john stultz <johnstul@us.ibm.com> wrote:
> > Yes, CLOCK_MONOTONIC_RAW was added specifically to create a hardware
> > agnostic interface that provided a 1:1 ratio to the hardware cycle
> > counter used by the timekeeping core. No NTP corrections or slewing
> > are applied and it isn't affected by settimeofday calls.
> >
> > Let me know if you run into any trouble with it.
> > thanks
> 
> Actually I did run into trouble. CLOCK_MONOTONIC_RAW is defined in
> linux/time.h and including that creates a lot of redefinition
> conflicts with time.h. I see the linux/ includes aren't intended for
> user space code.

Oh, right. Sorry about that, Glibc still hasn't picked these up.

I'll have to chase down how to get that done. 

For now, you can just add the following to your code:
#define CLOCK_MONOTONIC_RAW		4

Thanks for bringing it up!
-john

Re: [PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

On Wed, Mar 3, 2010 at 8:26 AM, john stultz <johnstul@us.ibm.com> wrote:
> Oh, right. Sorry about that, Glibc still hasn't picked these up.
>
> I'll have to chase down how to get that done.
>
> For now, you can just add the following to your code:
> #define CLOCK_MONOTONIC_RAW             4

Ah, thanks. Tried it out and it works how I needed. A sad omission
that this concept wasn't included in the original POSIX clockids. I
previously read a reference to the _RAW version but gave up when it
didn't seem to be present according to time.h.

Re: [PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

On Mon, Mar 1, 2010 at 1:59 AM, Clemens Ladisch <clemens@ladisch.de> wrote:
>> @@ -438,6 +469,17 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>>  {
>>        struct hpet_dev *devp;
>>
>> +       switch (cmd) {
>> +       case HPET_INFO:
>> +               break;
>> +       default:
>> +               {
>> +                       int r = hpet_alloc_timer(file);
>
> Applications might want to use HPET_INFO to find out which timer they
> got, so I think the driver cannot avoid allocating a timer in this case.

Oh, I missed commenting this in my first reply. I chose to make
HPET_INFO specify timer -1 in case a timer isn't currently allocated.
You think there are extant cases of programs depending on open then
immediate HPET_INFO having a timer allocated? Seems obscure to me. If
so I can change that. I did add the explicit form of allocating timers
to accomodate such a case hypothetically (open, allocate timer, get
info).

Re: [PATCH] hpet: factor timer allocate from open

[Clemens Ladisch]

Magnus Lynch wrote:
> On Mon, Mar 1, 2010 at 1:59 AM, Clemens Ladisch <clemens@ladisch.de> wrote:
> > Applications might want to use HPET_INFO to find out which timer they
> > got, so I think the driver cannot avoid allocating a timer in this case.
> 
> Oh, I missed commenting this in my first reply. I chose to make
> HPET_INFO specify timer -1 in case a timer isn't currently allocated.
> You think there are extant cases of programs depending on open then
> immediate HPET_INFO having a timer allocated? Seems obscure to me.

Get information about the just allocated timer seems to be the main
purpose of this ioctl to me.  None of the fields in struct hpet_info
looks to be interesting to an application that only wants to read the
main timer through mmap().

> If so I can change that. I did add the explicit form of allocating
> timers to accomodate such a case hypothetically (open, allocate timer,
> get info).

This wouldn't be backwards compatible.


Regards,
Clemens

Re: [PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

Clemens Ladisch wrote:
>> If so I can change that. I did add the explicit form of allocating
>> timers to accomodate such a case hypothetically (open, allocate timer,
>> get info).
>
> This wouldn't be backwards compatible.

OK, here's a version that retains HPET_INFO semantics.

Signed-off-by: Magnus Lynch <maglyx@gmail.com>
---
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e481c59..97f8d5b 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -244,16 +244,40 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
 
 static int hpet_open(struct inode *inode, struct file *file)
 {
-	struct hpet_dev *devp;
 	struct hpets *hpetp;
-	int i;
 
 	if (file->f_mode & FMODE_WRITE)
 		return -EINVAL;
 
+	hpetp = hpets;
+	/* starting with timer-neutral instance */
+	file->private_data = &hpetp->hp_dev[hpetp->hp_ntimer];
+
+	return 0;
+}
+
+static int hpet_alloc_timer(struct file *file)
+{
+	struct hpet_dev *devp;
+	struct hpets *hpetp;
+	int i;
+
+	/* once acquired, will remain */
+	devp = file->private_data;
+	if (devp->hd_timer)
+		return 0;
+
 	lock_kernel();
 	spin_lock_irq(&hpet_lock);
 
+	/* check for race acquiring */
+	devp = file->private_data;
+	if (devp->hd_timer) {
+		spin_unlock_irq(&hpet_lock);
+		unlock_kernel();
+		return 0;
+	}
+
 	for (devp = NULL, hpetp = hpets; hpetp && !devp; hpetp = hpetp->hp_next)
 		for (i = 0; i < hpetp->hp_ntimer; i++)
 			if (hpetp->hp_dev[i].hd_flags & HPET_OPEN)
@@ -384,6 +408,10 @@ static int hpet_fasync(int fd, struct file *file, int on)
 {
 	struct hpet_dev *devp;
 
+	int r = hpet_alloc_timer(file);
+	if (r < 0)
+		return r;
+
 	devp = file->private_data;
 
 	if (fasync_helper(fd, file, on, &devp->hd_async_queue) >= 0)
@@ -401,6 +429,9 @@ static int hpet_release(struct inode *inode, struct file *file)
 	devp = file->private_data;
 	timer = devp->hd_timer;
 
+	if (!timer)
+		goto out;
+
 	spin_lock_irq(&hpet_lock);
 
 	writeq((readq(&timer->hpet_config) & ~Tn_INT_ENB_CNF_MASK),
@@ -425,7 +456,7 @@ static int hpet_release(struct inode *inode, struct file *file)
 
 	if (irq)
 		free_irq(irq, devp);
-
+out:
 	file->private_data = NULL;
 	return 0;
 }
@@ -438,6 +469,10 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 {
 	struct hpet_dev *devp;
 
+	int r = hpet_alloc_timer(file);
+	if (r < 0)
+		return r;
+
 	devp = file->private_data;
 	return hpet_ioctl_common(devp, cmd, arg, 0);
 }
@@ -570,6 +605,9 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg, int kernel)
 		break;
 	case HPET_IE_ON:
 		return hpet_ioctl_ieon(devp);
+	case HPET_ALLOC_TIMER:
+		/* nothing to do */
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -794,7 +832,11 @@ int hpet_alloc(struct hpet_data *hdp)
 		return 0;
 	}
 
-	siz = sizeof(struct hpets) + ((hdp->hd_nirqs - 1) *
+	/*
+	 * last hpet_dev will have null timer pointer, gives timer-neutral
+	 * representation of block
+	 */
+	siz = sizeof(struct hpets) + ((hdp->hd_nirqs) *
 				      sizeof(struct hpet_dev));
 
 	hpetp = kzalloc(siz, GFP_KERNEL);
@@ -860,13 +902,16 @@ int hpet_alloc(struct hpet_data *hdp)
 		writeq(mcfg, &hpet->hpet_config);
 	}
 
-	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++, devp++) {
+	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer + 1;
+	     i++, devp++) {
 		struct hpet_timer __iomem *timer;
 
-		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
-
 		devp->hd_hpets = hpetp;
 		devp->hd_hpet = hpet;
+		if (i == hpetp->hp_ntimer)
+			continue;
+
+		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
 		devp->hd_timer = timer;
 
 		/*
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 219ca4f..d690c0f 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -125,6 +125,7 @@ struct hpet_info {
 #define	HPET_EPI	_IO('h', 0x04)	/* enable periodic */
 #define	HPET_DPI	_IO('h', 0x05)	/* disable periodic */
 #define	HPET_IRQFREQ	_IOW('h', 0x6, unsigned long)	/* IRQFREQ usec */
+#define	HPET_ALLOC_TIMER _IO('h', 0x7)
 
 #define MAX_HPET_TBS	8		/* maximum hpet timer blocks */
 

Re: [PATCH] hpet: factor timer allocate from open

[Clemens Ladisch]

Magnus Lynch wrote:
> Clemens Ladisch wrote:
>>> If so I can change that. I did add the explicit form of allocating
>>> timers to accomodate such a case hypothetically (open, allocate timer,
>>> get info).
>>
>> This wouldn't be backwards compatible.
> 
> OK, here's a version that retains HPET_INFO semantics.
> 
> Signed-off-by: Magnus Lynch <maglyx@gmail.com>

Acked-by: Clemens Ladisch <clemens@ladisch.de>

Re: [PATCH] hpet: factor timer allocate from open

[Andrew Morton]

On Sun, 07 Mar 2010 16:04:39 -0800 (PST)
Magnus Lynch <maglyx@gmail.com> wrote:

> Clemens Ladisch wrote:
> >> If so I can change that. I did add the explicit form of allocating
> >> timers to accomodate such a case hypothetically (open, allocate timer,
> >> get info).
> >
> > This wouldn't be backwards compatible.
> 
> OK, here's a version that retains HPET_INFO semantics.
> 
> Signed-off-by: Magnus Lynch <maglyx@gmail.com>

(wonders what this patch does)

Please always retain and maintain the changelog with each version of a patch.

Please resend this patch with a complete changelog.  Make sure that the
changelog addresses any questions which were raised during review. 
Please ensure that all participants in that review were cc'ed.

Thanks.

Re: [PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

Andrew Morton wrote:
> Please always retain and maintain the changelog with each version of a patch.
>
> Please resend this patch with a complete changelog.

OK, here's my description from the original posting:
<<
The current implementation of the /dev/hpet driver couples opening the
device with allocating one of the (scarce) timers (aka comparators).
This is a limitation in that the main counter may be valuable to
applications seeking a high-resolution timer who have no use for the
interrupt generating functionality of the comparators.

This patch alters the open semantics so that when the device is
opened, no timer is allocated. Operations that depend on a timer being
in context implicitly attempt allocating a timer, to maintain backward
compatibility. There is also an IOCTL (HPET_ALLOC_TIMER _IO) added so
that the allocation may be done explicitly. (I prefer the explicit
open then allocate pattern but don't know how practical it would be to
require all existing code to be changed.)
>>

The difference between the first and second version of my patch is that
the first altered the semantics of the HPET_INFO IOCTL so that it didn't
force allocation of a timer, which would not be strictly backwards compatible;
the second version keeps it so a device open followed by HPET_INFO call will
have allocated a timer (or attempted to and given an error).

Signed-off-by: Magnus Lynch <maglyx@gmail.com>

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e481c59..97f8d5b 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -244,16 +244,40 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
 
 static int hpet_open(struct inode *inode, struct file *file)
 {
-	struct hpet_dev *devp;
 	struct hpets *hpetp;
-	int i;
 
 	if (file->f_mode & FMODE_WRITE)
 		return -EINVAL;
 
+	hpetp = hpets;
+	/* starting with timer-neutral instance */
+	file->private_data = &hpetp->hp_dev[hpetp->hp_ntimer];
+
+	return 0;
+}
+
+static int hpet_alloc_timer(struct file *file)
+{
+	struct hpet_dev *devp;
+	struct hpets *hpetp;
+	int i;
+
+	/* once acquired, will remain */
+	devp = file->private_data;
+	if (devp->hd_timer)
+		return 0;
+
 	lock_kernel();
 	spin_lock_irq(&hpet_lock);
 
+	/* check for race acquiring */
+	devp = file->private_data;
+	if (devp->hd_timer) {
+		spin_unlock_irq(&hpet_lock);
+		unlock_kernel();
+		return 0;
+	}
+
 	for (devp = NULL, hpetp = hpets; hpetp && !devp; hpetp = hpetp->hp_next)
 		for (i = 0; i < hpetp->hp_ntimer; i++)
 			if (hpetp->hp_dev[i].hd_flags & HPET_OPEN)
@@ -384,6 +408,10 @@ static int hpet_fasync(int fd, struct file *file, int on)
 {
 	struct hpet_dev *devp;
 
+	int r = hpet_alloc_timer(file);
+	if (r < 0)
+		return r;
+
 	devp = file->private_data;
 
 	if (fasync_helper(fd, file, on, &devp->hd_async_queue) >= 0)
@@ -401,6 +429,9 @@ static int hpet_release(struct inode *inode, struct file *file)
 	devp = file->private_data;
 	timer = devp->hd_timer;
 
+	if (!timer)
+		goto out;
+
 	spin_lock_irq(&hpet_lock);
 
 	writeq((readq(&timer->hpet_config) & ~Tn_INT_ENB_CNF_MASK),
@@ -425,7 +456,7 @@ static int hpet_release(struct inode *inode, struct file *file)
 
 	if (irq)
 		free_irq(irq, devp);
-
+out:
 	file->private_data = NULL;
 	return 0;
 }
@@ -438,6 +469,10 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 {
 	struct hpet_dev *devp;
 
+	int r = hpet_alloc_timer(file);
+	if (r < 0)
+		return r;
+
 	devp = file->private_data;
 	return hpet_ioctl_common(devp, cmd, arg, 0);
 }
@@ -570,6 +605,9 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg, int kernel)
 		break;
 	case HPET_IE_ON:
 		return hpet_ioctl_ieon(devp);
+	case HPET_ALLOC_TIMER:
+		/* nothing to do */
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -794,7 +832,11 @@ int hpet_alloc(struct hpet_data *hdp)
 		return 0;
 	}
 
-	siz = sizeof(struct hpets) + ((hdp->hd_nirqs - 1) *
+	/*
+	 * last hpet_dev will have null timer pointer, gives timer-neutral
+	 * representation of block
+	 */
+	siz = sizeof(struct hpets) + ((hdp->hd_nirqs) *
 				      sizeof(struct hpet_dev));
 
 	hpetp = kzalloc(siz, GFP_KERNEL);
@@ -860,13 +902,16 @@ int hpet_alloc(struct hpet_data *hdp)
 		writeq(mcfg, &hpet->hpet_config);
 	}
 
-	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++, devp++) {
+	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer + 1;
+	     i++, devp++) {
 		struct hpet_timer __iomem *timer;
 
-		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
-
 		devp->hd_hpets = hpetp;
 		devp->hd_hpet = hpet;
+		if (i == hpetp->hp_ntimer)
+			continue;
+
+		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
 		devp->hd_timer = timer;
 
 		/*
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 219ca4f..d690c0f 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -125,6 +125,7 @@ struct hpet_info {
 #define	HPET_EPI	_IO('h', 0x04)	/* enable periodic */
 #define	HPET_DPI	_IO('h', 0x05)	/* disable periodic */
 #define	HPET_IRQFREQ	_IOW('h', 0x6, unsigned long)	/* IRQFREQ usec */
+#define	HPET_ALLOC_TIMER _IO('h', 0x7)
 
 #define MAX_HPET_TBS	8		/* maximum hpet timer blocks */
 

Re: [PATCH] hpet: factor timer allocate from open

[Andrew Morton]

On Thu, 18 Mar 2010 21:06:58 -0700 (PDT)
Magnus Lynch <maglyx@gmail.com> wrote:

> Andrew Morton wrote:
> > Please always retain and maintain the changelog with each version of a patch.
> >
> > Please resend this patch with a complete changelog.
> 
> OK, here's my description from the original posting:
> <<
> The current implementation of the /dev/hpet driver couples opening the
> device with allocating one of the (scarce) timers (aka comparators).
> This is a limitation in that the main counter may be valuable to
> applications seeking a high-resolution timer who have no use for the
> interrupt generating functionality of the comparators.
> 
> This patch alters the open semantics so that when the device is
> opened, no timer is allocated. Operations that depend on a timer being
> in context implicitly attempt allocating a timer, to maintain backward
> compatibility. There is also an IOCTL (HPET_ALLOC_TIMER _IO) added so
> that the allocation may be done explicitly. (I prefer the explicit
> open then allocate pattern but don't know how practical it would be to
> require all existing code to be changed.)

A stylistic nit:

> @@ -384,6 +408,10 @@ static int hpet_fasync(int fd, struct file *file, int on)
>  {
>  	struct hpet_dev *devp;
>  
> +	int r = hpet_alloc_timer(file);
> +	if (r < 0)
> +		return r;
> +
>  	devp = file->private_data;
>  
>  	if (fasync_helper(fd, file, on, &devp->hd_async_queue) >= 0)
>
> ...
>
> @@ -438,6 +469,10 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>  {
>  	struct hpet_dev *devp;
>  
> +	int r = hpet_alloc_timer(file);
> +	if (r < 0)
> +		return r;
> +
>  	devp = file->private_data;
>  	return hpet_ioctl_common(devp, cmd, arg, 0);
>  }

The above constructs make it harder for people to modify the code
later.  If they want to add a new local, where to put it?  If they want
to add more code, where to put it?  Plus there are risks that people
will accidentally turn the code into c99-style definitions.

One could do

 {
 	struct hpet_dev *devp;
	int r = hpet_alloc_timer(file);

	if (r < 0)
		return r;


but that's not terribly good either: it adds risk that someone will
later add a leak.

Better is the plain old simple approach:


 {
 	struct hpet_dev *devp;
	int r;

	r = hpet_alloc_timer(file);
	if (r < 0)
		return r;

Re: [PATCH] hpet: factor timer allocate from open

[Magnus Lynch]

On Mon, Mar 22, 2010 at 3:21 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> Better is the plain old simple approach:
>
>
>  {
>        struct hpet_dev *devp;
>        int r;
>
>        r = hpet_alloc_timer(file);
>        if (r < 0)
>                return r;

Fine with me, thanks for doing the patch yourself.