[PATCH 1/3] HPET: Convert WARN_ON to WARN_ON_ONCE

[PATCH 1/3] HPET: Convert WARN_ON to WARN_ON_ONCE

[Matt Fleming]

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 <mjf@gentoo.org>
---
 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

[PATCH 2/3] HPET: Enter hpet_interrupt_handler with interrupts disabled

[Matt Fleming]

Some functions that may be called from this handler require that
interrupts are disabled.

Signed-off-by: Matt Fleming <mjf@gentoo.org>
---
 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

[PATCH 3/3] HPET: Read from HPET_Tn_CMP() not HPET_T0_CMP

[Matt Fleming]

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 <mjf@gentoo.org>
---
 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

Re: [PATCH 2/3] HPET: Enter hpet_interrupt_handler with interrupts disabled

[Will Newton]

On Sun, Nov 2, 2008 at 4:04 PM, Matt Fleming <mjf@gentoo.org> wrote:
> Some functions that may be called from this handler require that
> interrupts are disabled.
>
> Signed-off-by: Matt Fleming <mjf@gentoo.org>
> ---
>  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 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/
>

Re: [PATCH 2/3] HPET: Enter hpet_interrupt_handler with interrupts disabled

[Matt Fleming]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

2008/11/2 Will Newton <will.newton@gmail.com>:
>
> 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 <mjf@gentoo.org>
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 <mjf@gentoo.org>
---
 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

[-- Attachment #2: 0002-HPET-Enter-hpet_interrupt_handler-with-interrupts-d.patch --]
[-- Type: application/octet-stream, Size: 1054 bytes --]

From c445728f36de599770088e345fc03ef9fafd8470 Mon Sep 17 00:00:00 2001
From: Matt Fleming <mjf@gentoo.org>
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 <mjf@gentoo.org>
---
 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

Re: [PATCH 3/3] HPET: Read from HPET_Tn_CMP() not HPET_T0_CMP

[Matt Fleming]

2008/11/2 Matt Fleming <mjf@gentoo.org>:
> 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?

Re: [PATCH 3/3] HPET: Read from HPET_Tn_CMP() not HPET_T0_CMP

[Thomas Gleixner]

Matt,

On Sun, 9 Nov 2008, Matt Fleming wrote:

> 2008/11/2 Matt Fleming <mjf@gentoo.org>:
> > 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

Re: [PATCH 2/3] HPET: Enter hpet_interrupt_handler with interrupts disabled

[Takashi Iwai]

At Sun, 2 Nov 2008 22:23:13 +0000,
Matt Fleming wrote:
> 
> 2008/11/2 Will Newton <will.newton@gmail.com>:
> >
> > 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