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

This is a discussion on [RFC][PATCH] HPET: register additional counter-only char device - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 5 of 5

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

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

    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
    ---
    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;

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread