It is possible to flood the console with call traces if the WARN_ON
condition is true because of the frequency with which this function is
called.
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/kernel/hpet.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 77017e8..f10f946 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -322,7 +322,7 @@ static int hpet_next_event(unsigned long delta,
* what we wrote hit the chip before we compare it to the
* counter.
*/
- WARN_ON((u32)hpet_readl(HPET_T0_CMP) != cnt);
+ WARN_ON_ONCE((u32)hpet_readl(HPET_T0_CMP) != cnt);
return (s32)((u32)hpet_readl(HPET_COUNTER) - cnt) >= 0 ? -ETIME : 0;
}
--
1.5.6.4
Some functions that may be called from this handler require that
interrupts are disabled.
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/kernel/hpet.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index f10f946..c28fff2 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -445,7 +445,8 @@ static int hpet_setup_irq(struct hpet_dev *dev)
{
if (request_irq(dev->irq, hpet_interrupt_handler,
- IRQF_SHARED|IRQF_NOBALANCING, dev->name, dev))
+ IRQF_DISABLED|IRQF_SHARED|IRQF_NOBALANCING,
+ dev->name, dev))
return -1;
disable_irq(dev->irq);
--
1.5.6.4
In hpet_next_event() we check that the value we just wrote to
HPET_Tn_CMP(timer) has reached the chip. Currently, we're checking that
the value we wrote to HPET_Tn_CMP(timer) is in HPET_T0_CMP, which, if
timer is anything other than timer 0, is likely to fail.
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/kernel/hpet.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index c28fff2..76feb2e 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -322,7 +322,7 @@ static int hpet_next_event(unsigned long delta,
* what we wrote hit the chip before we compare it to the
* counter.
*/
- WARN_ON_ONCE((u32)hpet_readl(HPET_T0_CMP) != cnt);
+ WARN_ON_ONCE((u32)hpet_readl(HPET_Tn_CMP(timer)) != cnt);
return (s32)((u32)hpet_readl(HPET_COUNTER) - cnt) >= 0 ? -ETIME : 0;
}
--
1.5.6.4
On Sun, Nov 2, 2008 at 4:04 PM, Matt Fleming <[email protected]> wrote:
> Some functions that may be called from this handler require that
> interrupts are disabled.
>
> Signed-off-by: Matt Fleming <[email protected]>
> ---
> arch/x86/kernel/hpet.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> index f10f946..c28fff2 100644
> --- a/arch/x86/kernel/hpet.c
> +++ b/arch/x86/kernel/hpet.c
> @@ -445,7 +445,8 @@ static int hpet_setup_irq(struct hpet_dev *dev)
> {
>
> if (request_irq(dev->irq, hpet_interrupt_handler,
> - IRQF_SHARED|IRQF_NOBALANCING, dev->name, dev))
> + IRQF_DISABLED|IRQF_SHARED|IRQF_NOBALANCING,
> + dev->name, dev))
Combining IRQF_DISABLED and IRQF_SHARED does not reliably disable
interrupts in the handler. Perhaps IRQF_SHARED should be removed at
the same time?
> return -1;
>
> disable_irq(dev->irq);
> --
> 1.5.6.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
2008/11/2 Will Newton <[email protected]>:
>
> Combining IRQF_DISABLED and IRQF_SHARED does not reliably disable
> interrupts in the handler. Perhaps IRQF_SHARED should be removed at
> the same time?
>
I didn't know that. Under what conditions is it unreliable?
I've attached a second attempt at this patch. It seems that the IRQ is
never shared anyway, so I followed your suggestion and removed
IRQF_SHARED. My machine seems to be running fine.
>From c445728f36de599770088e345fc03ef9fafd8470 Mon Sep 17 00:00:00 2001
From: Matt Fleming <[email protected]>
Date: Sun, 2 Nov 2008 14:00:23 +0000
Subject: [PATCH 2/3] HPET: Enter hpet_interrupt_handler with interrupts disabled
Some functions that may be called from this handler require that
interrupts are disabled. Also, combining IRQF_DISABLED and
IRQF_SHARED does not reliably disable interrupts in a handler, so
remove IRQF_SHARED from the irq flags (this irq is not shared anyway).
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/kernel/hpet.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index f10f946..bfb17f4 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -445,7 +445,7 @@ static int hpet_setup_irq(struct hpet_dev *dev)
{
if (request_irq(dev->irq, hpet_interrupt_handler,
- IRQF_SHARED|IRQF_NOBALANCING, dev->name, dev))
+ IRQF_DISABLED|IRQF_NOBALANCING, dev->name, dev))
return -1;
disable_irq(dev->irq);
--
1.5.6.4
2008/11/2 Matt Fleming <[email protected]>:
> In hpet_next_event() we check that the value we just wrote to
> HPET_Tn_CMP(timer) has reached the chip. Currently, we're checking that
> the value we wrote to HPET_Tn_CMP(timer) is in HPET_T0_CMP, which, if
> timer is anything other than timer 0, is likely to fail.
>
Any comments on this set of patches?
Matt,
On Sun, 9 Nov 2008, Matt Fleming wrote:
> 2008/11/2 Matt Fleming <[email protected]>:
> > In hpet_next_event() we check that the value we just wrote to
> > HPET_Tn_CMP(timer) has reached the chip. Currently, we're checking that
> > the value we wrote to HPET_Tn_CMP(timer) is in HPET_T0_CMP, which, if
> > timer is anything other than timer 0, is likely to fail.
> >
>
> Any comments on this set of patches?
good catch. Applied to tip timers/hpet and scheduled for linus.
Thanks,
tglx
At Sun, 2 Nov 2008 22:23:13 +0000,
Matt Fleming wrote:
>
> 2008/11/2 Will Newton <[email protected]>:
> >
> > Combining IRQF_DISABLED and IRQF_SHARED does not reliably disable
> > interrupts in the handler. Perhaps IRQF_SHARED should be removed at
> > the same time?
> >
>
> I didn't know that. Under what conditions is it unreliable?
The kernel checks IRQF_DISABLED bit of only the first handler
even if multiple handlers are assigned to a single IRQ line.
Thus, if another handler without IRQF_DISABLED has been already
registered before yours, your IRQF_DISABLED is ignored.
See kernel/irq/handle.c.
Takashi