[RFC][PATCH] HPET: register additional counter-only char device

[RFC][PATCH] HPET: register additional counter-only char device

[Dimitri Gorokhovik]

Hi,

I need to have many processes all reading from userspace the counter register
of the (same) HPET hardware. Just reading counter values, not handling timers, 
enabling/disabling interrupts etc. `/dev/hpet' doesn't allow for this, as the 
number of times it can be opened is limited by the number of timers available. 

What would be the right way to implement such a support? For now, I simply
register a new misc device, '/dev/hpetctr', along with '/dev/hpet', for the same
ACPI device and on the same occasion.

Tested on 2.6.24-86_64-rt4 and on 2.6.25-rc8-git8.

---

Register a device `/dev/hpetctr' (10:229) giving access to the counter   
of a HPET hw module. The available operations are: `open', `close' and
a read-only `mmap'. Contrary to `dev/hpet' it can be opened an unlimited 
number of times. `read' not provided because of the cost of the system
call making the operation pointless.

Signed-off-by: Dimitri Gorokhovik <dimitri.gorokhovik@free.fr>
---
 drivers/char/hpet.c        |   71 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/miscdevice.h |    1 
 2 files changed, 72 insertions(+)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 1399971..d94c3ce 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -218,6 +218,24 @@ static int hpet_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int hpetctr_open(struct inode *inode, struct file *file)
+{
+	if (file->f_mode & FMODE_WRITE)
+		return -EINVAL;
+
+	spin_lock_irq(&hpet_lock);
+
+	if (hpets == NULL) {
+		spin_unlock_irq(&hpet_lock);
+		return -ENOSYS;
+	}
+
+	file->private_data = (void *)hpets->hp_hpet_phys;
+	spin_unlock_irq(&hpet_lock);
+
+	return 0;
+}
+
 static ssize_t
 hpet_read(struct file *file, char __user *buf, size_t count, loff_t * ppos)
 {
@@ -318,6 +336,34 @@ static int hpet_mmap(struct file *file, struct vm_area_struct *vma)
 #endif
 }
 
+
+static int hpetctr_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	unsigned long addr;
+
+	if (((vma->vm_end - vma->vm_start) != PAGE_SIZE) || vma->vm_pgoff)
+		return -EINVAL;
+
+	if (pgprot_val(vma->vm_page_prot) != pgprot_val(PAGE_READONLY))
+		return -EPERM;
+
+	addr = (unsigned long)file->private_data;
+	if (addr & (PAGE_SIZE - 1))
+		return -ENOSYS;
+
+	vma->vm_flags |= VM_IO;
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+	if (io_remap_pfn_range(vma, vma->vm_start, addr >> PAGE_SHIFT,
+					PAGE_SIZE, vma->vm_page_prot)) {
+		printk(KERN_ERR "%s: io_remap_pfn_range failed\n",
+			__FUNCTION__);
+		return -EAGAIN;
+	}
+
+	return 0;
+}
+
 static int hpet_fasync(int fd, struct file *file, int on)
 {
 	struct hpet_dev *devp;
@@ -371,6 +417,13 @@ static int hpet_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int hpetctr_release(struct inode *inode, struct file *file)
+{
+	file->private_data = NULL;
+	return 0;
+}
+
+
 static int hpet_ioctl_common(struct hpet_dev *, int, unsigned long, int);
 
 static int
@@ -589,6 +642,14 @@ static const struct file_operations hpet_fops = {
 	.mmap = hpet_mmap,
 };
 
+static const struct file_operations hpetctr_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.open = hpetctr_open,
+	.release = hpetctr_release,
+	.mmap = hpetctr_mmap,
+};
+
 static int hpet_is_known(struct hpet_data *hdp)
 {
 	struct hpets *hpetp;
@@ -954,6 +1015,9 @@ static struct acpi_driver hpet_acpi_driver = {
 };
 
 static struct miscdevice hpet_misc = { HPET_MINOR, "hpet", &hpet_fops };
+static struct miscdevice hpetctr_misc = {
+	HPETCTR_MINOR, "hpetctr", &hpetctr_fops
+};
 
 static int __init hpet_init(void)
 {
@@ -973,11 +1037,18 @@ static int __init hpet_init(void)
 		return result;
 	}
 
+	result = misc_register(&hpetctr_misc);
+	if (result < 0)
+		printk(KERN_ERR "%s: failed to register '/dev/hpetctr'\n",
+			__FUNCTION__);
+
 	return 0;
 }
 
 static void __exit hpet_exit(void)
 {
+	misc_deregister(&hpetctr_misc);
+
 	acpi_bus_unregister_driver(&hpet_acpi_driver);
 
 	if (sysctl_header)
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index 24b30b9..76c9b76 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -29,6 +29,7 @@
 
 #define TUN_MINOR	     200
 #define	HPET_MINOR	     228
+#define HPETCTR_MINOR	     229
 #define KVM_MINOR            232
 
 struct device;

Re: [RFC][PATCH] HPET: register additional counter-only char device

[Clemens Ladisch]

Dimitri Gorokhovik wrote:
> I need to have many processes all reading from userspace the counter register
> of the (same) HPET hardware. Just reading counter values, not handling timers,
> enabling/disabling interrupts etc. `/dev/hpet' doesn't allow for this, as the
> number of times it can be opened is limited by the number of timers available.

Indeed.  The driver assumes that userspace wants to program its own
timer (like an RTC device).  Allowing mmap() on the hardware counter was
more an afterthought.

> What would be the right way to implement such a support? For now, I simply
> register a new misc device, '/dev/hpetctr', along with '/dev/hpet', for the same
> ACPI device and on the same occasion.

Your patch circumvents CONFIG_HPET_MMAP.

Another possibility would be to allow the device to be opened
infinitely many times but not to allocate a hardware timer until one of
the ioctls is called.  This means that opening /dev/hpet does not
guarantee that a timer is available, but this has already been possible
previously because request_irq() might fail.


Regards,
Clemens

Re: [RFC][PATCH] HPET: register additional counter-only char device

[Dimitri Gorokhovik]

On Fri, 2008-04-11 at 15:20 +0200, Clemens Ladisch wrote:
> Dimitri Gorokhovik wrote:
> > I need to have many processes all reading from userspace the counter register
> > of the (same) HPET hardware. Just reading counter values, not handling timers,
> > enabling/disabling interrupts etc. `/dev/hpet' doesn't allow for this, as the
> > number of times it can be opened is limited by the number of timers available.
> 
> Indeed.  The driver assumes that userspace wants to program its own
> timer (like an RTC device).  Allowing mmap() on the hardware counter was
> more an afterthought.
> 
> > What would be the right way to implement such a support? For now, I simply
> > register a new misc device, '/dev/hpetctr', along with '/dev/hpet', for the same
> > ACPI device and on the same occasion.
> 
> Your patch circumvents CONFIG_HPET_MMAP.

Right. I hesitated to put it in, but multiple #ifdef/endif clutter too
much the resulting code. Something better should be devised in this
case, like separating the added code into another file, changing a
couple of symbols from static to extern etc.

However, why people wanted the original 'mmap' of /dev/hpet to be
disabled? Probably to prevent the HPET registers from poking with? Gain
in code size is too small, can't think it was the primary reason. If so,
isn't having a read-only mapping enough in itself?


> Another possibility would be to allow the device to be opened
> infinitely many times but not to allocate a hardware timer until one of
> the ioctls is called.  This means that opening /dev/hpet does not
> guarantee that a timer is available, but this has already been possible
> previously because request_irq() might fail.

Good point. The patch would be much more intrusive and voluminous (and
coming from a total newcomer). Would there be an interest for such a
patch? If yes, I might find some time to spend on it.

I did try to figure out the cleanest way by myself before posting, and I
couldn't mix and match several points about HPET:

-- /dev/hpet can only be opened with O_RDONLY, but for fiddling with
interrupts IMO write perms suit much better. One could imagine opening
lots of instances under O_RDONLY. Obviously, this way is doomed now
because of legacy apps.

-- to me, /dev/hpet is primarily a timer, not just a counter. It seems
wrong to me if it, once opened, would mostly fail to its primary
function (but this of course is all subjective matter).


Thanks for your comments!



 
> 
> 
> Regards,
> Clemens

Re: [RFC][PATCH] HPET: register additional counter-only char device

[Clemens Ladisch]

Dimitri Gorokhovik wrote:
> On Fri, 2008-04-11 at 15:20 +0200, Clemens Ladisch wrote:
> > Dimitri Gorokhovik wrote:
> > > I need to have many processes all reading from userspace the counter register
> > > of the (same) HPET hardware.  [...]
> > > What would be the right way to implement such a support? For now, I simply
> > > register a new misc device, '/dev/hpetctr', along with '/dev/hpet', for the same
> > > ACPI device and on the same occasion.
> >
> > Your patch circumvents CONFIG_HPET_MMAP.
>
> Right. I hesitated to put it in, but multiple #ifdef/endif clutter too
> much the resulting code. Something better should be devised in this
> case, like separating the added code into another file, changing a
> couple of symbols from static to extern etc.
>
> However, why people wanted the original 'mmap' of /dev/hpet to be
> disabled? Probably to prevent the HPET registers from poking with?

Reading HPET registers should not be dangerous in any way, but it might
be possible to read other devices' registers that happen to lie inside
the same memory page (HPET registers are only 1024 bytes).

> > Another possibility would be to allow the device to be opened
> > infinitely many times but not to allocate a hardware timer until one of
> > the ioctls is called.  This means that opening /dev/hpet does not
> > guarantee that a timer is available, but this has already been possible
> > previously because request_irq() might fail.
>
> Good point. The patch would be much more intrusive and voluminous (and
> coming from a total newcomer). Would there be an interest for such a
> patch?

Yes, definitely.  I've wanted to do this patch for some time but haven't
found the time.

> -- to me, /dev/hpet is primarily a timer, not just a counter. It seems
> wrong to me if it, once opened, would mostly fail to its primary
> function (but this of course is all subjective matter).

There are at least as many available timers as before the change -- a
program that tries to use a timer will still get a timer, and now this
will succeed even if there are other programs that use only the counter.

Even now, for most programs using /dev/hpet, it actually is just a
counter.


Regards,
Clemens

Re: [RFC][PATCH] HPET: register additional counter-only char device

[Dimitri Gorokhovik]

On Fri, 2008-04-11 at 17:57 +0200, Clemens Ladisch wrote: 
> Dimitri Gorokhovik wrote:
> > 
> > -- to me, /dev/hpet is primarily a timer, not just a counter. It seems
> > wrong to me if it, once opened, would mostly fail to its primary
> > function (but this of course is all subjective matter).
> 
> There are at least as many available timers as before the change -- a
> program that tries to use a timer will still get a timer, and now this
> will succeed even if there are other programs that use only the counter.
> 
> Even now, for most programs using /dev/hpet, it actually is just a
> counter.

The problem with the implementation of this approach is the multiple hw
instrances of HPET, which is currently supported. Unless one can rip it
all off and hardwire '/dev/hpet' to a single HPET hardware block (can
one ?), I can't seem to figure out how to implement the proposed
modification.

An example:

-- an application opens '/dev/hpet'. There are several HPET hw blocks
in the system -- physical address of some of them is used -- success is
returned. A hw timer slot is not allocated at this point.

-- the app 'mmap's the opened device (the phys address selected in the
'open' is used).

-- some time later, the app issues an 'ioctl' call -- it wants the
interrupt enabled and so a hw timer slot has to be allocated.

It appears that there are no timers available on the hw block whose phys
addr has been used for open/mmap, but there are free timers on the
other(s) HPET hw blocks.

Our choice:
a) 'ioctl' should fail;
b) 'ioctl' may return a timer from another hw block.

I think b) is fundamentally broken. a) risks to return too many failures
with timer slots still available, unless some strategy is devised for
distribution of 'open's across the hw blocks.

Now imagine HPET hw blocks are not created equal in the system (most
likely, clocking frequency is different). What would be such a strategy
in this case ?

All this yields a complex implementation without a real reason (many
(most?) systems with only one HPET hw block), and still a problematic
user interface (how does one chooses the HPET block he or she wants?),
only in the name of the compatibility with the existing interface.

The clean interface would be to have:

/dev/hpet0/counter
          /timer0
          /timer1
          /timer2
	  /...

[/dev/hpet1/...]

'counter' device allowing 'mmap' and 'ioctl(HPET_INFO)', and timer
devices allowing rest of IOCTLs and other fops except mmap. Choosing a
suitable HPET hw is a policy belonging in the app -- right now it is in
the kernel, even thought it is as simple as walking a list.

But then again, such implementation would break the existing interface.

Regards,

Dimitri