* [PATCH 1/2] HPET: disallow zero interrupt frequency
@ 2005-09-22 15:08 Clemens Ladisch
2005-09-22 15:08 ` [PATCH 2/2] HPET: make frequency calculations 32 bit safe Clemens Ladisch
2005-09-22 19:14 ` [PATCH 1/2] HPET: disallow zero interrupt frequency Bob Picco
0 siblings, 2 replies; 8+ messages in thread
From: Clemens Ladisch @ 2005-09-22 15:08 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, Clemens Ladisch, Bob Picco
Trying to set an interrupt frequency of zero would result in a
division by zero, so disallow this.
Enabling the interrupt when the frequency hasn't yet been set would
use an interrupt period of minimum length, so disallow this, too.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-09-22 10:56:23.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-09-22 10:56:26.000000000 +0200
@@ -365,6 +365,9 @@ static int hpet_ioctl_ieon(struct hpet_d
hpet = devp->hd_hpet;
hpetp = devp->hd_hpets;
+ if (!devp->hd_ireqfreq)
+ return -EIO;
+
v = readq(&timer->hpet_config);
spin_lock_irq(&hpet_lock);
@@ -517,7 +520,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
break;
}
- if (arg & (arg - 1)) {
+ if (arg < 1 || (arg & (arg - 1))) {
err = -EINVAL;
break;
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] HPET: make frequency calculations 32 bit safe
2005-09-22 15:08 [PATCH 1/2] HPET: disallow zero interrupt frequency Clemens Ladisch
@ 2005-09-22 15:08 ` Clemens Ladisch
2005-09-27 14:57 ` Bob Picco
2005-09-22 19:14 ` [PATCH 1/2] HPET: disallow zero interrupt frequency Bob Picco
1 sibling, 1 reply; 8+ messages in thread
From: Clemens Ladisch @ 2005-09-22 15:08 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, Clemens Ladisch, Bob Picco
On 32-bit architectures, the multiplication in the argument for
hpet_time_div() often overflows. In the typical case of a 14.32 MHz
timer, this happens when the desired frequency exceeds 61 Hz.
To avoid this multiplication, we can precompute and store the hardware
timer frequency, instead of the period, in the device structure, which
leaves us with a simple division when computing the number of timer
ticks.
As a side effect, this also removes a theoretical bug where the timer
interpolator's frequency would be computed as a 32-bit value even if
the HPET frequency is greater than 2^32 Hz (the HPET spec allows up to
10 GHz).
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-09-22 11:10:01.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-09-22 12:08:48.000000000 +0200
@@ -78,7 +78,7 @@ struct hpets {
struct hpet __iomem *hp_hpet;
unsigned long hp_hpet_phys;
struct time_interpolator *hp_interpolator;
- unsigned long hp_period;
+ unsigned long long hp_tick_freq;
unsigned long hp_delta;
unsigned int hp_ntimer;
unsigned int hp_which;
@@ -427,12 +427,14 @@ static int hpet_ioctl_ieon(struct hpet_d
return 0;
}
-static inline unsigned long hpet_time_div(unsigned long dis)
+/* converts Hz to number of timer ticks */
+static inline unsigned long hpet_time_div(struct hpets *hpets,
+ unsigned long dis)
{
- unsigned long long m = 1000000000000000ULL;
+ unsigned long long m;
+ m = hpets->hp_tick_freq + (dis >> 1);
do_div(m, dis);
-
return (unsigned long)m;
}
@@ -480,7 +482,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
{
struct hpet_info info;
- info.hi_ireqfreq = hpet_time_div(hpetp->hp_period *
+ info.hi_ireqfreq = hpet_time_div(hpetp,
devp->hd_ireqfreq);
info.hi_flags =
readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
@@ -524,7 +526,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
break;
}
- devp->hd_ireqfreq = hpet_time_div(hpetp->hp_period * arg);
+ devp->hd_ireqfreq = hpet_time_div(hpetp, arg);
}
return err;
@@ -713,7 +715,7 @@ static void hpet_register_interpolator(s
ti->source = TIME_SOURCE_MMIO64;
ti->shift = 10;
ti->addr = &hpetp->hp_hpet->hpet_mc;
- ti->frequency = hpet_time_div(hpets->hp_period);
+ ti->frequency = hpetp->hp_tick_freq;
ti->drift = ti->frequency * HPET_DRIFT / 1000000;
ti->mask = -1;
@@ -750,7 +752,7 @@ static unsigned long hpet_calibrate(stru
t = read_counter(&timer->hpet_compare);
i = 0;
- count = hpet_time_div(hpetp->hp_period * TICK_CALIBRATE);
+ count = hpet_time_div(hpetp, TICK_CALIBRATE);
local_irq_save(flags);
@@ -775,7 +777,8 @@ int hpet_alloc(struct hpet_data *hdp)
size_t siz;
struct hpet __iomem *hpet;
static struct hpets *last = (struct hpets *)0;
- unsigned long ns;
+ unsigned long ns, period;
+ unsigned long long temp;
/*
* hpet_alloc can be called by platform dependent code.
@@ -825,8 +828,12 @@ int hpet_alloc(struct hpet_data *hdp)
last = hpetp;
- hpetp->hp_period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
- HPET_COUNTER_CLK_PERIOD_SHIFT;
+ period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
+ HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
+ temp = 1000000000000000uLL; /* 10^15 femtoseconds per second */
+ temp += period >> 1; /* round */
+ do_div(temp, period);
+ hpetp->hp_tick_freq = temp; /* ticks per second */
printk(KERN_INFO "hpet%d: at MMIO 0x%lx, IRQ%s",
hpetp->hp_which, hdp->hd_phys_address,
@@ -835,8 +842,7 @@ int hpet_alloc(struct hpet_data *hdp)
printk("%s %d", i > 0 ? "," : "", hdp->hd_irq[i]);
printk("\n");
- ns = hpetp->hp_period; /* femptoseconds, 10^-15 */
- ns /= 1000000; /* convert to nanoseconds, 10^-9 */
+ 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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] HPET: disallow zero interrupt frequency
2005-09-22 15:08 [PATCH 1/2] HPET: disallow zero interrupt frequency Clemens Ladisch
2005-09-22 15:08 ` [PATCH 2/2] HPET: make frequency calculations 32 bit safe Clemens Ladisch
@ 2005-09-22 19:14 ` Bob Picco
2005-09-28 6:51 ` Clemens Ladisch
1 sibling, 1 reply; 8+ messages in thread
From: Bob Picco @ 2005-09-22 19:14 UTC (permalink / raw)
To: Clemens Ladisch; +Cc: linux-kernel, akpm, Bob Picco
Clemens Ladisch wrote: [Thu Sep 22 2005, 11:08:32AM EDT]
> Trying to set an interrupt frequency of zero would result in a
> division by zero, so disallow this.
>
> Enabling the interrupt when the frequency hasn't yet been set would
> use an interrupt period of minimum length, so disallow this, too.
>
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
>
> --- linux-2.6.13.orig/drivers/char/hpet.c 2005-09-22 10:56:23.000000000 +0200
> +++ linux-2.6.13/drivers/char/hpet.c 2005-09-22 10:56:26.000000000 +0200
> @@ -365,6 +365,9 @@ static int hpet_ioctl_ieon(struct hpet_d
> hpet = devp->hd_hpet;
> hpetp = devp->hd_hpets;
>
> + if (!devp->hd_ireqfreq)
> + return -EIO;
> +
> v = readq(&timer->hpet_config);
> spin_lock_irq(&hpet_lock);
>
> @@ -517,7 +520,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
> break;
> }
>
> - if (arg & (arg - 1)) {
> + if (arg < 1 || (arg & (arg - 1))) {
Well it seems like what you want is:
if (!arg || (arg & (arg - 1))) {
> err = -EINVAL;
> break;
> }
> -
BTW, it will be a day or two before I can review your other patch.
thanks,
bob
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HPET: make frequency calculations 32 bit safe
2005-09-22 15:08 ` [PATCH 2/2] HPET: make frequency calculations 32 bit safe Clemens Ladisch
@ 2005-09-27 14:57 ` Bob Picco
2005-09-30 6:30 ` Clemens Ladisch
0 siblings, 1 reply; 8+ messages in thread
From: Bob Picco @ 2005-09-27 14:57 UTC (permalink / raw)
To: Clemens Ladisch; +Cc: linux-kernel, akpm, Bob Picco
Clemens Ladisch wrote: [Thu Sep 22 2005, 11:08:41AM EDT]
> On 32-bit architectures, the multiplication in the argument for
> hpet_time_div() often overflows. In the typical case of a 14.32 MHz
> timer, this happens when the desired frequency exceeds 61 Hz.
>
> To avoid this multiplication, we can precompute and store the hardware
> timer frequency, instead of the period, in the device structure, which
> leaves us with a simple division when computing the number of timer
> ticks.
>
> As a side effect, this also removes a theoretical bug where the timer
> interpolator's frequency would be computed as a 32-bit value even if
> the HPET frequency is greater than 2^32 Hz (the HPET spec allows up to
> 10 GHz).
>
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
>
> --- linux-2.6.13.orig/drivers/char/hpet.c 2005-09-22 11:10:01.000000000 +0200
> +++ linux-2.6.13/drivers/char/hpet.c 2005-09-22 12:08:48.000000000 +0200
> @@ -78,7 +78,7 @@ struct hpets {
> struct hpet __iomem *hp_hpet;
> unsigned long hp_hpet_phys;
> struct time_interpolator *hp_interpolator;
> - unsigned long hp_period;
> + unsigned long long hp_tick_freq;
> unsigned long hp_delta;
> unsigned int hp_ntimer;
> unsigned int hp_which;
> @@ -427,12 +427,14 @@ static int hpet_ioctl_ieon(struct hpet_d
> return 0;
> }
>
> -static inline unsigned long hpet_time_div(unsigned long dis)
> +/* converts Hz to number of timer ticks */
> +static inline unsigned long hpet_time_div(struct hpets *hpets,
> + unsigned long dis)
> {
> - unsigned long long m = 1000000000000000ULL;
> + unsigned long long m;
>
> + m = hpets->hp_tick_freq + (dis >> 1);
> do_div(m, dis);
> -
> return (unsigned long)m;
> }
>
> @@ -480,7 +482,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
> {
> struct hpet_info info;
>
> - info.hi_ireqfreq = hpet_time_div(hpetp->hp_period *
> + info.hi_ireqfreq = hpet_time_div(hpetp,
> devp->hd_ireqfreq);
> info.hi_flags =
> readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
> @@ -524,7 +526,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
> break;
> }
>
> - devp->hd_ireqfreq = hpet_time_div(hpetp->hp_period * arg);
> + devp->hd_ireqfreq = hpet_time_div(hpetp, arg);
> }
>
> return err;
> @@ -713,7 +715,7 @@ static void hpet_register_interpolator(s
> ti->source = TIME_SOURCE_MMIO64;
> ti->shift = 10;
> ti->addr = &hpetp->hp_hpet->hpet_mc;
> - ti->frequency = hpet_time_div(hpets->hp_period);
> + ti->frequency = hpetp->hp_tick_freq;
> ti->drift = ti->frequency * HPET_DRIFT / 1000000;
> ti->mask = -1;
>
> @@ -750,7 +752,7 @@ static unsigned long hpet_calibrate(stru
> t = read_counter(&timer->hpet_compare);
>
> i = 0;
> - count = hpet_time_div(hpetp->hp_period * TICK_CALIBRATE);
> + count = hpet_time_div(hpetp, TICK_CALIBRATE);
>
> local_irq_save(flags);
>
> @@ -775,7 +777,8 @@ int hpet_alloc(struct hpet_data *hdp)
> size_t siz;
> struct hpet __iomem *hpet;
> static struct hpets *last = (struct hpets *)0;
> - unsigned long ns;
> + unsigned long ns, period;
> + unsigned long long temp;
>
> /*
> * hpet_alloc can be called by platform dependent code.
> @@ -825,8 +828,12 @@ int hpet_alloc(struct hpet_data *hdp)
>
> last = hpetp;
>
> - hpetp->hp_period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
> - HPET_COUNTER_CLK_PERIOD_SHIFT;
> + period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
> + HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
> + temp = 1000000000000000uLL; /* 10^15 femtoseconds per second */
> + temp += period >> 1; /* round */
> + do_div(temp, period);
> + hpetp->hp_tick_freq = temp; /* ticks per second */
>
> printk(KERN_INFO "hpet%d: at MMIO 0x%lx, IRQ%s",
> hpetp->hp_which, hdp->hd_phys_address,
> @@ -835,8 +842,7 @@ int hpet_alloc(struct hpet_data *hdp)
> printk("%s %d", i > 0 ? "," : "", hdp->hd_irq[i]);
> printk("\n");
>
> - ns = hpetp->hp_period; /* femptoseconds, 10^-15 */
> - ns /= 1000000; /* convert to nanoseconds, 10^-9 */
> + 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);
> -
Sorry for the delay. Looks like my 32 bit code isn't correct for >61 Hz
Never had a 32 bit arch with HPET for testing. This patch looks fine.
thanks,
bob
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] HPET: disallow zero interrupt frequency
2005-09-22 19:14 ` [PATCH 1/2] HPET: disallow zero interrupt frequency Bob Picco
@ 2005-09-28 6:51 ` Clemens Ladisch
0 siblings, 0 replies; 8+ messages in thread
From: Clemens Ladisch @ 2005-09-28 6:51 UTC (permalink / raw)
To: Bob Picco; +Cc: linux-kernel, akpm
[-- Attachment #1: Type: TEXT/PLAIN, Size: 479 bytes --]
Bob Picco wrote:
> > - if (arg & (arg - 1)) {
> > + if (arg < 1 || (arg & (arg - 1))) {
>
> Well it seems like what you want is:
>
> if (!arg || (arg & (arg - 1))) {
Yes, it's the same. Here's the new patch:
Disallow setting an interrupt frequency of zero (which would result in
a division by zero), and disallow enabling the interrupt when the
frequency hasn't yet been set (which would use an interrupt period of
zero).
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
[-- Attachment #2: Type: TEXT/PLAIN, Size: 570 bytes --]
--- linux-2.6.13.orig/drivers/char/hpet.c 2005-09-22 10:56:23.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c 2005-09-25 19:02:02.000000000 +0200
@@ -365,6 +365,9 @@ static int hpet_ioctl_ieon(struct hpet_d
hpet = devp->hd_hpet;
hpetp = devp->hd_hpets;
+ if (!devp->hd_ireqfreq)
+ return -EIO;
+
v = readq(&timer->hpet_config);
spin_lock_irq(&hpet_lock);
@@ -517,7 +520,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
break;
}
- if (arg & (arg - 1)) {
+ if (!arg || (arg & (arg - 1))) {
err = -EINVAL;
break;
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HPET: make frequency calculations 32 bit safe
2005-09-27 14:57 ` Bob Picco
@ 2005-09-30 6:30 ` Clemens Ladisch
0 siblings, 0 replies; 8+ messages in thread
From: Clemens Ladisch @ 2005-09-30 6:30 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, Bob Picco
Bob Picco wrote:
> Clemens Ladisch wrote: [Thu Sep 22 2005, 11:08:41AM EDT]
> > [...]
>
> This patch looks fine.
Andrew, please apply this one, too.
Regards,
Clemens
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HPET: make frequency calculations 32 bit safe
2005-09-23 23:46 [PATCH 2/2] HPET: make frequency calculations 32 bit safe Karsten Wiese
@ 2005-09-26 7:30 ` Clemens Ladisch
0 siblings, 0 replies; 8+ messages in thread
From: Clemens Ladisch @ 2005-09-26 7:30 UTC (permalink / raw)
To: Karsten Wiese; +Cc: linux-kernel, Bob Picco, Ingo Molnar
Karsten Wiese wrote:
> > --- linux-2.6.13.orig/drivers/char/hpet.c 2005-09-22 11:10:01.000000000 +0200
> > +++ linux-2.6.13/drivers/char/hpet.c 2005-09-22 12:08:48.000000000 +0200
> > ...
> > - unsigned long hp_period;
> > + unsigned long long hp_tick_freq;
>
> An 'unsigned long' is enough.
> Are we expecting hpets stepping at more than 4GHz?
The HPET specification allows up to 10 GHz.
> They are called 'legacy' in some docs already ;-)
Do these docs mention a non-legacy alternative?
> Here the via8237's hpet runs at ~14MHz.
AFAIK all current implementations still use the good ol' 14.138180MHz
timer.
Regards,
Clemens
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HPET: make frequency calculations 32 bit safe
@ 2005-09-23 23:46 Karsten Wiese
2005-09-26 7:30 ` Clemens Ladisch
0 siblings, 1 reply; 8+ messages in thread
From: Karsten Wiese @ 2005-09-23 23:46 UTC (permalink / raw)
To: Clemens Ladisch; +Cc: linux-kernel, Bob Picco, Ingo Molnar
> --- linux-2.6.13.orig/drivers/char/hpet.c 2005-09-22 11:10:01.000000000 +0200
> +++ linux-2.6.13/drivers/char/hpet.c 2005-09-22 12:08:48.000000000 +0200
> @@ -78,7 +78,7 @@ struct hpets {
> struct hpet __iomem *hp_hpet;
> unsigned long hp_hpet_phys;
> struct time_interpolator *hp_interpolator;
> - unsigned long hp_period;
> + unsigned long long hp_tick_freq;
An 'unsigned long' is enough.
Are we expecting hpets stepping at more than 4GHz?
They are called 'legacy' in some docs already ;-)
Here the via8237's hpet runs at ~14MHz.
> unsigned long hp_delta;
> unsigned int hp_ntimer;
> unsigned int hp_which;
Tested OK on i386 2.6.13-rt14 with above nitpick applied.
Karsten
___________________________________________________________
Gesendet von Yahoo! Mail - Jetzt mit 1GB Speicher kostenlos - Hier anmelden: http://mail.yahoo.de
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2005-09-30 6:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-09-22 15:08 [PATCH 1/2] HPET: disallow zero interrupt frequency Clemens Ladisch
2005-09-22 15:08 ` [PATCH 2/2] HPET: make frequency calculations 32 bit safe Clemens Ladisch
2005-09-27 14:57 ` Bob Picco
2005-09-30 6:30 ` Clemens Ladisch
2005-09-22 19:14 ` [PATCH 1/2] HPET: disallow zero interrupt frequency Bob Picco
2005-09-28 6:51 ` Clemens Ladisch
2005-09-23 23:46 [PATCH 2/2] HPET: make frequency calculations 32 bit safe Karsten Wiese
2005-09-26 7:30 ` Clemens Ladisch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).