Another round of HPET bugfixes and cleanups.
drivers/char/hpet.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
From: Clemens Ladisch <[email protected]>
Clear the ht_opaque field in the hpet_register() function before
searching for a free timer to prevent the function from incorrectly
assuming that the search succeeded afterwards.
Signed-off-by: Clemens Ladisch <[email protected]>
Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-10-03 22:53:15.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-10-03 22:53:18.000000000 +0200
@@ -587,6 +587,8 @@ int hpet_register(struct hpet_task *tp,
return -EINVAL;
}
+ tp->ht_opaque = NULL;
+
spin_lock_irq(&hpet_task_lock);
spin_lock(&hpet_lock);
From: Clemens Ladisch <[email protected]>
Fix two instances where a function would access the first HPET device
instead of the current one.
Signed-off-by: Clemens Ladisch <[email protected]>
Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-10-03 22:53:18.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-10-03 22:53:21.000000000 +0200
@@ -430,7 +430,7 @@ static int hpet_ioctl_ieon(struct hpet_d
}
if (devp->hd_flags & HPET_SHARED_IRQ) {
- isr = 1 << (devp - hpets->hp_dev);
+ isr = 1 << (devp - devp->hd_hpets->hp_dev);
writel(isr, &hpet->hpet_isr);
}
writeq(g, &timer->hpet_config);
@@ -769,7 +769,7 @@ static unsigned long hpet_calibrate(stru
if (!timer)
return 0;
- hpet = hpets->hp_hpet;
+ hpet = hpetp->hp_hpet;
t = read_counter(&timer->hpet_compare);
i = 0;
From: Clemens Ladisch <[email protected]>
Fix a division by zero that happened when the HPET_INFO ioctl was
called before a timer frequency had been set.
Signed-off-by: Clemens Ladisch <[email protected]>
Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-10-03 22:53:12.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-10-03 22:53:15.000000000 +0200
@@ -494,8 +494,11 @@ hpet_ioctl_common(struct hpet_dev *devp,
{
struct hpet_info info;
- info.hi_ireqfreq = hpet_time_div(hpetp,
- devp->hd_ireqfreq);
+ if (devp->hd_ireqfreq)
+ info.hi_ireqfreq =
+ hpet_time_div(hpetp, devp->hd_ireqfreq);
+ else
+ info.hi_ireqfreq = 0;
info.hi_flags =
readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
info.hi_hpet = devp->hd_hpets->hp_which;
From: Keir Fraser <[email protected]>
The address passed to io_remap_pfn_range() in hpet_mmap() does not
need to be converted using __pa(): it is already a physical
address. This bug was found and the patch suggested by Clay Harris.
I introduced this particular bug when making io_remap_pfn_range
changes a few months ago. In fact mmap()ing /dev/hpet has *never*
previously worked: before my changes __pa() was being executed on an
ioremap()ed virtual address, which is also invalid.
Signed-off-by: Keir Fraser <[email protected]>
Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-10-03 22:52:30.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-10-03 22:53:09.000000000 +0200
@@ -279,7 +279,6 @@ static int hpet_mmap(struct file *file,
vma->vm_flags |= VM_IO;
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
- addr = __pa(addr);
if (io_remap_pfn_range(vma, vma->vm_start, addr >> PAGE_SHIFT,
PAGE_SIZE, vma->vm_page_prot)) {
From: Clemens Ladisch <[email protected]>
In the hpet_ioctl_common() function, devp->hd_hpets is already cached
in the hpetp variable, so we can use just that.
Signed-off-by: Clemens Ladisch <[email protected]>
Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-10-03 22:53:21.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-10-03 22:53:24.000000000 +0200
@@ -501,8 +501,8 @@ hpet_ioctl_common(struct hpet_dev *devp,
info.hi_ireqfreq = 0;
info.hi_flags =
readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
- info.hi_hpet = devp->hd_hpets->hp_which;
- info.hi_timer = devp - devp->hd_hpets->hp_dev;
+ info.hi_hpet = hpetp->hp_which;
+ info.hi_timer = devp - hpetp->hp_dev;
if (kernel)
memcpy((void *)arg, &info, sizeof(info));
else
From: Clemens Ladisch <[email protected]>
When booting, display the timer frequency in Hertz instead of as tick
length in nanoseconds. Apart from saving a local variable, this makes
the message more easily comprehensible.
Signed-off-by: Clemens Ladisch <[email protected]>
Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-10-03 22:53:24.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-10-03 22:53:28.000000000 +0200
@@ -798,7 +798,7 @@ int hpet_alloc(struct hpet_data *hdp)
size_t siz;
struct hpet __iomem *hpet;
static struct hpets *last = (struct hpets *)0;
- unsigned long ns, period;
+ unsigned long period;
unsigned long long temp;
/*
@@ -863,10 +863,9 @@ int hpet_alloc(struct hpet_data *hdp)
printk("%s %d", i > 0 ? "," : "", hdp->hd_irq[i]);
printk("\n");
- ns = period / 1000000; /* convert to nanoseconds, 10^-9 */
- printk(KERN_INFO "hpet%d: %ldns tick, %d %d-bit timers\n",
- hpetp->hp_which, ns, hpetp->hp_ntimer,
- cap & HPET_COUNTER_SIZE_MASK ? 64 : 32);
+ printk(KERN_INFO "hpet%u: %u %d-bit timers, %Lu Hz\n",
+ hpetp->hp_which, hpetp->hp_ntimer,
+ cap & HPET_COUNTER_SIZE_MASK ? 64 : 32, hpetp->hp_tick_freq);
mcfg = readq(&hpet->hpet_config);
if ((mcfg & HPET_ENABLE_CNF_MASK) == 0) {
From: Clemens Ladisch <[email protected]>
Fix a wrong memory access in hpet_ioctl_common(). It was not possible
to use the HPET_INFO ioctl from kernel space because it always called
copy_to_user().
Signed-off-by: Clemens Ladisch <[email protected]>
Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-10-03 22:53:09.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-10-03 22:53:12.000000000 +0200
@@ -500,8 +500,12 @@ hpet_ioctl_common(struct hpet_dev *devp,
readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
info.hi_hpet = devp->hd_hpets->hp_which;
info.hi_timer = devp - devp->hd_hpets->hp_dev;
- if (copy_to_user((void __user *)arg, &info, sizeof(info)))
- err = -EFAULT;
+ if (kernel)
+ memcpy((void *)arg, &info, sizeof(info));
+ else
+ if (copy_to_user((void __user *)arg, &info,
+ sizeof(info)))
+ err = -EFAULT;
break;
}
case HPET_EPI:
Clemens Ladisch wrote: [Tue Oct 04 2005, 08:41:26AM EDT]
> Another round of HPET bugfixes and cleanups.
>
> drivers/char/hpet.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
Clemens:
These improvements and fixes look good to me. Thanks for your work.
bob
On Tue, 04 Oct 2005 14:41:26 +0200 (MEST) Clemens Ladisch wrote:
> Another round of HPET bugfixes and cleanups.
>
> drivers/char/hpet.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
Hi,
I've applied and tested all of these along with what is
currently in -mm (only -mm hpet + timer patches).
By "tested" I mean that I booted the kernel. :)
What kind of testing have you done?
Do you have any timer test tools that you use to verify that
timers are actually working as expected?
Or maybe I could/should ask one or all of:
- John Stultz
- Thomas Gleixner
- George Anzinger (HRT project does have some tests)
Thanks,
---
~Randy
Randy.Dunlap wrote:
> On Tue, 04 Oct 2005 14:41:26 +0200 (MEST) Clemens Ladisch wrote:
>
> > Another round of HPET bugfixes and cleanups.
>
> I've applied and tested all of these along with what is
> currently in -mm (only -mm hpet + timer patches).
>
> By "tested" I mean that I booted the kernel. :)
>
> What kind of testing have you done?
> Do you have any timer test tools that you use to verify that
> timers are actually working as expected?
Apart from the test program in hpet.txt, I'm using the ALSA HPET
driver (contained in the ALSA 1.0.9 package, but not yet in the kernel
tree) and then just test it using the ALSA API and/or use it as the
MIDI sequencer timer.
HTH
Clemens
At Mon, 17 Oct 2005 18:30:40 +0200 (METDST),
Clemens Ladisch wrote:
>
> Randy.Dunlap wrote:
>
> > On Tue, 04 Oct 2005 14:41:26 +0200 (MEST) Clemens Ladisch wrote:
> >
> > > Another round of HPET bugfixes and cleanups.
> >
> > I've applied and tested all of these along with what is
> > currently in -mm (only -mm hpet + timer patches).
> >
> > By "tested" I mean that I booted the kernel. :)
> >
> > What kind of testing have you done?
> > Do you have any timer test tools that you use to verify that
> > timers are actually working as expected?
>
> Apart from the test program in hpet.txt, I'm using the ALSA HPET
> driver (contained in the ALSA 1.0.9 package, but not yet in the kernel
> tree) and then just test it using the ALSA API and/or use it as the
> MIDI sequencer timer.
Let's push it to kernel tree :)
Takashi
Randy.Dunlap wrote:
> On Tue, 18 Oct 2005, Clemens Ladisch wrote:
>
> > Randy.Dunlap wrote:
> > > hpet_poll, HPET_IE_ON failed: (5) Input/output error
> >
> > This probably means that the interrupt isn't free.
>
> Isn't free because it is already in use as a system timer
> interrupt, for example?
If it tries to use interrupt 0, yes.
> Does this test work (succeed) for you? If so, is HPET being
> used as replacement for legacy timer and RTC? (as it is where I
> am testing)
Yes. Yes.
However, I've patched my kernel to initialize the HPET manually
because my BIOS doesn't bother to do it at all. A quick Google search
shows that in most cases where the BIOS _does_ bother, the third timer
(which is the only free one after system timer and RTC have grabbed
theirs) didn't get initialized and is still set to interrupt 0 (which
isn't actually supported by most HPET hardware).
This means that hpet.c must initialize the interrupt routing register
in this case. I'll write a patch for this.
Regards,
Clemens
Clemens Ladisch wrote:
> Randy.Dunlap wrote:
> However, I've patched my kernel to initialize the HPET manually
> because my BIOS doesn't bother to do it at all. A quick Google search
> shows that in most cases where the BIOS _does_ bother, the third timer
> (which is the only free one after system timer and RTC have grabbed
> theirs) didn't get initialized and is still set to interrupt 0 (which
> isn't actually supported by most HPET hardware).
>
> This means that hpet.c must initialize the interrupt routing register
> in this case. I'll write a patch for this.
I'm using attached diff. But I gave up on HPET. On VIA periodic mode
is hopelessly broken, on AMD HPET read takes about 1500ns (23 HPET cycles),
and current Linux RTC emulation has problem that when interrupt is delayed
it stops until counter rollover. And fixing this would add at least 1.5us
to the interrupt handler, and it seems quite lot to me...
Petr
I wrote:
> This means that hpet.c must initialize the interrupt routing register
> in this case. I'll write a patch for this.
Okay, this is a quick hack, untested. It just tries to set the first
interrupt that the timer could use.
Regards,
Clemens
Petr Vandrovec wrote:
> Clemens Ladisch wrote:
> > However, I've patched my kernel to initialize the HPET manually
> > because my BIOS doesn't bother to do it at all. A quick Google search
> > shows that in most cases where the BIOS _does_ bother, the third timer
> > (which is the only free one after system timer and RTC have grabbed
> > theirs) didn't get initialized and is still set to interrupt 0 (which
> > isn't actually supported by most HPET hardware).
> >
> > This means that hpet.c must initialize the interrupt routing register
> > in this case. I'll write a patch for this.
>
> I'm using attached diff.
The other changes of your patch are already in the -mm kernel.
> But I gave up on HPET. On VIA periodic mode is hopelessly broken,
I've heard it works with timer 0, and the capability bit on timer 1 is
just wrong.
> on AMD HPET read takes about 1500ns (23 HPET cycles),
It's a round trip to the southbridge. Intel needs about 500 ns.
> and current Linux RTC emulation has problem that when interrupt is
> delayed it stops until counter rollover.
I'm planning to fix this.
OTOH, the hpet.c implementation has the problem that all interrupt
delays affect the timer, i.e., it isn't precise anymore.
> And fixing this would add at least 1.5us to the interrupt handler,
> and it seems quite lot to me...
I didn't measure how much reading the RTC registers costs us, but
those aren't likely to be faster.
I'm thinking of a different approach: Assuming that such a big delay
almost never actually does happen, we run a separate watchdog timer
(using a kernel timer that is guaranteed to work) at a much lower
frequency to check whether the real timer got stuck. This trades off
the HPET register read against the timer_list overhead (and that we
still lose _some_ interrupts when the worst case happens).
Regards,
Clemens
>-----Original Message-----
>From: [email protected]
>[mailto:[email protected]] On Behalf Of
>Clemens Ladisch
>Sent: Wednesday, October 19, 2005 6:49 AM
>To: Randy.Dunlap
>Cc: [email protected]
>Subject: Re: [PATCH 0/7] more HPET fixes and enhancements
>
>I wrote:
>> This means that hpet.c must initialize the interrupt routing register
>> in this case. I'll write a patch for this.
>
>Okay, this is a quick hack, untested. It just tries to set the first
>interrupt that the timer could use.
>
You will need more changes to make interrupt work. Especially if the
particular IRQ that you are using here is not used by any other PCI
device in the system. Then BIOS won't report anything on that IRQ and
all the things done in setup_IO_APIC_irqs() should be done for this
particular IRQ. And it will depend on whether IOAPIC/PIC is ised for
interrupts and such things as well. Atleast that what I remember from
last time I got HPET to work with IRQs other than IRQ0 and IRQ8.
Thanks,
Venki
Clemens Ladisch wrote:
> Petr Vandrovec wrote:
>
>>Clemens Ladisch wrote:
>>
>>>However, I've patched my kernel to initialize the HPET manually
>>>because my BIOS doesn't bother to do it at all. A quick Google search
>>>shows that in most cases where the BIOS _does_ bother, the third timer
>>>(which is the only free one after system timer and RTC have grabbed
>>>theirs) didn't get initialized and is still set to interrupt 0 (which
>>>isn't actually supported by most HPET hardware).
>>>
>>>This means that hpet.c must initialize the interrupt routing register
>>>in this case. I'll write a patch for this.
>>
>>I'm using attached diff.
>
>
> The other changes of your patch are already in the -mm kernel.
>
>
>>But I gave up on HPET. On VIA periodic mode is hopelessly broken,
>
>
> I've heard it works with timer 0, and the capability bit on timer 1 is
> just wrong.
Nope. Periodic mode works (I've made my tests on timer #2), you can just
set only period (through way which sets value according to the spec), and
you cannot set current value (at least I do not know how). So I can
program VIA hardware to generate periodic interrupt, there is just
unavoidable delay up to 5 minutes. I've worked around by setting period
to 1 tick, so in 5 minutes value and main timer synchronize, and if
timer is not stopped after that then it stays synchronized with main timer.
>>And fixing this would add at least 1.5us to the interrupt handler,
>>and it seems quite lot to me...
>
>
> I didn't measure how much reading the RTC registers costs us, but
> those aren't likely to be faster.
>
> I'm thinking of a different approach: Assuming that such a big delay
> almost never actually does happen, we run a separate watchdog timer
> (using a kernel timer that is guaranteed to work) at a much lower
> frequency to check whether the real timer got stuck. This trades off
> the HPET register read against the timer_list overhead (and that we
> still lose _some_ interrupts when the worst case happens).
It would work for VMware's use of /dev/rtc if number of missed interrupts
will be reported on next read. Otherwise it might be a problem for
keeping time between host and virtual machines in sync.
Petr