RE: [PATCH] fix PCI interrupt setting for ia64

RE: [PATCH] fix PCI interrupt setting for ia64

[Liu, Benjamin]

Grant,

Both ISA and PCI device drivers would call arch/ia64/kernel/irq.c:request_irq()-->arch/ia64/kernel/irq.c:setup_irq() -->arch/ia64/kernel/iosapic.c:iosapic_startup_level_irq() or  arch/ia64/kernel/iosapic.c:iosapic_startup_edge_irq() function to unmask the IRQ. I believe the ISA can be handled gracefully, if any.

ISA is legacy to IA64. The configuration script of 2.4.23 has CONFIG_ISA off explicitly for IA64, 2.6.2 doesn't have this option for IA64. I just wonder whether the legacy probing method still exists on IA64.

Thanks,
Pingping (Benjamin) Liu
Intel China Software Center


>-----Original Message-----
>From: linux-ia64-owner@vger.kernel.org 
>[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Grant Grundler
>Sent: 2004年3月8日 14:31
>To: Kenji Kaneshige
>Cc: linux-ia64@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] fix PCI interrupt setting for ia64
>
>
>On Mon, Mar 08, 2004 at 11:49:10AM +0900, Kenji Kaneshige wrote:
>> In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are unmasked at the
>> boot time before installing device drivers. I think it is 
>very dangerous.
>
>Hi Kenji,
>I think this behavior exists to support "legacy" IRQ probing.
>I'm wondering if it would be sufficient to wrap the patch in
>"#ifndef CONFIG_ISA" or something like that.
>
>grant
>-
>To unsubscribe from this list: send the line "unsubscribe 
>linux-ia64" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] fix PCI interrupt setting for ia64

[Takayoshi Kochi]

Hi,

From: "Liu, Benjamin" <benjamin.liu@intel.com>
Subject: RE: [PATCH] fix PCI interrupt setting for ia64
Date: Mon, 8 Mar 2004 15:44:16 +0800

> ISA is legacy to IA64. The configuration script of 2.4.23 has
> CONFIG_ISA off explicitly for IA64, 2.6.2 doesn't have this
> option for IA64. I just wonder whether the legacy probing
> method still exists on IA64.

I think that's still true for IDE / serial port drivers.
Kaneshige-san, could you confirm your changes are compatible
with probe_irq_on()?

Itanium-generation machines (such as BigSur) depends on
probe_irq_on() for finding serial port IRQ.

---
Takayoshi Kochi <t-kochi@bq.jp.nec.com>

RE: [PATCH] fix PCI interrupt setting for ia64

[Kenji Kaneshige]

Hi,

> Kaneshige-san, could you confirm your changes are compatible
> with probe_irq_on()?

OK. I'll confirm.

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org
> [mailto:linux-kernel-owner@vger.kernel.org]On Behalf Of Takayoshi Kochi
> Sent: Monday, March 08, 2004 6:26 PM
> To: benjamin.liu@intel.com
> Cc: iod00d@hp.com; kaneshige.kenji@jp.fujitsu.com;
> linux-ia64@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] fix PCI interrupt setting for ia64
>
>
> Hi,
>
> From: "Liu, Benjamin" <benjamin.liu@intel.com>
> Subject: RE: [PATCH] fix PCI interrupt setting for ia64
> Date: Mon, 8 Mar 2004 15:44:16 +0800
>
> > ISA is legacy to IA64. The configuration script of 2.4.23 has
> > CONFIG_ISA off explicitly for IA64, 2.6.2 doesn't have this
> > option for IA64. I just wonder whether the legacy probing
> > method still exists on IA64.
>
> I think that's still true for IDE / serial port drivers.
> Kaneshige-san, could you confirm your changes are compatible
> with probe_irq_on()?
>
> Itanium-generation machines (such as BigSur) depends on
> probe_irq_on() for finding serial port IRQ.
>
> ---
> Takayoshi Kochi <t-kochi@bq.jp.nec.com>
> -
> 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] fix PCI interrupt setting for ia64

[Bjorn Helgaas]

On Monday 08 March 2004 2:25 am, Takayoshi Kochi wrote:
> I think that's still true for IDE / serial port drivers.
> Kaneshige-san, could you confirm your changes are compatible
> with probe_irq_on()?
> 
> Itanium-generation machines (such as BigSur) depends on
> probe_irq_on() for finding serial port IRQ.

Strictly speaking, since ACPI tells us about IRQs, we shouldn't need
probe_irq_on() on ia64, should we?

I don't see any ACPI smarts in the IDE driver, but I think the
serial driver needs only the attached patch to make it avoid
the use of probe_irq_on().  I tested this on i2k and various
HP zx1 boxes, and it works fine.

Russell, if you agree, would you mind applying this?

ACPI and HCDP tell us what IRQ the serial port uses, so there's
no need to have the driver probe for the IRQ.

===== drivers/serial/8250_acpi.c 1.7 vs edited =====
--- 1.7/drivers/serial/8250_acpi.c	Fri Jan 16 15:01:45 2004
+++ edited/drivers/serial/8250_acpi.c	Mon Mar  8 11:14:51 2004
@@ -134,8 +134,7 @@
 	}
 
 	serial_req.baud_base = BASE_BAUD;
-	serial_req.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF |
-			   UPF_AUTO_IRQ  | UPF_RESOURCES;
+	serial_req.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_RESOURCES;
 
 	priv->line = register_serial(&serial_req);
 	if (priv->line < 0) {
===== drivers/serial/8250_hcdp.c 1.2 vs edited =====
--- 1.2/drivers/serial/8250_hcdp.c	Sun Jan 11 16:27:13 2004
+++ edited/drivers/serial/8250_hcdp.c	Mon Mar  8 11:28:27 2004
@@ -186,8 +186,6 @@
 		port.irq = gsi;
 #endif
 		port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_RESOURCES;
-		if (gsi)
-			port.flags |= ASYNC_AUTO_IRQ;
 
 		/*
 		 * Note: the above memset() initializes port.line to 0,

Re: [PATCH] fix PCI interrupt setting for ia64

[Bjorn Helgaas]

On Monday 08 March 2004 2:25 am, Takayoshi Kochi wrote:
> I think that's still true for IDE / serial port drivers.
> Kaneshige-san, could you confirm your changes are compatible
> with probe_irq_on()?
> 
> Itanium-generation machines (such as BigSur) depends on
> probe_irq_on() for finding serial port IRQ.

Strictly speaking, since ACPI tells us about IRQs, we shouldn't need
probe_irq_on() on ia64, should we?

I don't see any ACPI smarts in the IDE driver, but I think the
serial driver needs only the attached patch to make it avoid
the use of probe_irq_on().  I tested this on i2k and various
HP zx1 boxes, and it works fine.

Russell, if you agree, would you mind applying this?

ACPI and HCDP tell us what IRQ the serial port uses, so there's
no need to have the driver probe for the IRQ.

===== drivers/serial/8250_acpi.c 1.7 vs edited =====
--- 1.7/drivers/serial/8250_acpi.c	Fri Jan 16 15:01:45 2004
+++ edited/drivers/serial/8250_acpi.c	Mon Mar  8 11:14:51 2004
@@ -134,8 +134,7 @@
 	}
 
 	serial_req.baud_base = BASE_BAUD;
-	serial_req.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF |
-			   UPF_AUTO_IRQ  | UPF_RESOURCES;
+	serial_req.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_RESOURCES;
 
 	priv->line = register_serial(&serial_req);
 	if (priv->line < 0) {
===== drivers/serial/8250_hcdp.c 1.2 vs edited =====
--- 1.2/drivers/serial/8250_hcdp.c	Sun Jan 11 16:27:13 2004
+++ edited/drivers/serial/8250_hcdp.c	Mon Mar  8 11:28:27 2004
@@ -186,8 +186,6 @@
 		port.irq = gsi;
 #endif
 		port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_RESOURCES;
-		if (gsi)
-			port.flags |= ASYNC_AUTO_IRQ;
 
 		/*
 		 * Note: the above memset() initializes port.line to 0,

Re: [PATCH] fix PCI interrupt setting for ia64

[David Mosberger]

>>>>> On Mon, 8 Mar 2004 12:13:22 -0700, Bjorn Helgaas <bjorn.helgaas@hp.com> said:

  Bjorn> Strictly speaking, since ACPI tells us about IRQs, we
  Bjorn> shouldn't need probe_irq_on() on ia64, should we?

  Bjorn> I don't see any ACPI smarts in the IDE driver, but I think
  Bjorn> the serial driver needs only the attached patch to make it
  Bjorn> avoid the use of probe_irq_on().  I tested this on i2k and
  Bjorn> various HP zx1 boxes, and it works fine.

  Bjorn> Russell, if you agree, would you mind applying this?

  Bjorn> ACPI and HCDP tell us what IRQ the serial port uses, so
  Bjorn> there's no need to have the driver probe for the IRQ.

The patch looks good to me and I certainly agree that probe_irq_on()
should be avoided.  However, as long as this interface is part of the
Linux kernel, we should support it on ia64 since we can't know for
sure when there are no drivers left using that interface.  (And there
is no reason this interface is limited to ISA, though that certainly
is the bus that generally had to use it, for lack of explicit irq
info.)

	--david

Re: [PATCH] fix PCI interrupt setting for ia64

[Russell King]

On Mon, Mar 08, 2004 at 01:44:05PM -0800, David Mosberger wrote:
> >>>>> On Mon, 8 Mar 2004 12:13:22 -0700, Bjorn Helgaas <bjorn.helgaas@hp.com> said:
> 
>   Bjorn> Strictly speaking, since ACPI tells us about IRQs, we
>   Bjorn> shouldn't need probe_irq_on() on ia64, should we?
> 
>   Bjorn> I don't see any ACPI smarts in the IDE driver, but I think
>   Bjorn> the serial driver needs only the attached patch to make it
>   Bjorn> avoid the use of probe_irq_on().  I tested this on i2k and
>   Bjorn> various HP zx1 boxes, and it works fine.
> 
>   Bjorn> Russell, if you agree, would you mind applying this?
> 
>   Bjorn> ACPI and HCDP tell us what IRQ the serial port uses, so
>   Bjorn> there's no need to have the driver probe for the IRQ.
> 
> The patch looks good to me and I certainly agree that probe_irq_on()
> should be avoided.  However, as long as this interface is part of the
> Linux kernel, we should support it on ia64 since we can't know for
> sure when there are no drivers left using that interface.  (And there
> is no reason this interface is limited to ISA, though that certainly
> is the bus that generally had to use it, for lack of explicit irq
> info.)

OTOH, if you know "this port is absolutely definitely using this IRQ"
then you don't want to use IRQ probing.

As long as ACPI positively knows the correct IRQ, so we really shouldn't
be using IRQ probing here.  The only thing which causes me concern is...
what about ACPI table/BIOS bugs?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH] fix PCI interrupt setting for ia64

[Bjorn Helgaas]

On Monday 08 March 2004 2:54 pm, Russell King wrote:
> OTOH, if you know "this port is absolutely definitely using this IRQ"
> then you don't want to use IRQ probing.
> 
> As long as ACPI positively knows the correct IRQ, so we really shouldn't
> be using IRQ probing here.  The only thing which causes me concern is...
> what about ACPI table/BIOS bugs?

I suspect (not being expert in the serial innards) that with the old
code things would "just work", and the ACPI table bug would never
be noticed.

With the new code, my guess is that the device just wouldn't work
(we think it has an interrupt, but it's not hooked up correctly).

My inclination is that it's better to help find ACPI bugs, and
if broken tables turn out to be a problem, we can add some kind
of command-line switch or blacklist to deal with it.  But I
guess we should really get David's opinion, since this is a
potential issue for 2.6 distributions.

Bjorn

Re: [PATCH] fix PCI interrupt setting for ia64

[David Mosberger]

>>>>> On Mon, 8 Mar 2004 15:05:21 -0700, Bjorn Helgaas <bjorn.helgaas@hp.com> said:

  Bjorn> My inclination is that it's better to help find ACPI bugs,
  Bjorn> and if broken tables turn out to be a problem, we can add
  Bjorn> some kind of command-line switch or blacklist to deal with
  Bjorn> it.  But I guess we should really get David's opinion, since
  Bjorn> this is a potential issue for 2.6 distributions.

I agree with Bjorn's reasoning, but think that the patch should be
tested first on a Big Sur machine (with the latest official firmware).
If something breaks with old firmware, we can then at least ask the
affected people to upgrade their firmware (or come up with a kernel
workaround).

	--david

Re: [PATCH] fix PCI interrupt setting for ia64

[Bjorn Helgaas]

On Monday 08 March 2004 3:10 pm, David Mosberger wrote:
> I agree with Bjorn's reasoning, but think that the patch should be
> tested first on a Big Sur machine (with the latest official firmware).
> If something breaks with old firmware, we can then at least ask the
> affected people to upgrade their firmware (or come up with a kernel
> workaround).

I've tested it on my i2000, and it seems to work fine.  This is with
firmware version

    W460GXBS2.86E.0117C.P09.200108091154

This is the only BIOS version I found mentioned on www.hp.com,
so I assume it's the latest official version.

Re: [PATCH] fix PCI interrupt setting for ia64

[David Mosberger]

>>>>> On Mon, 8 Mar 2004 15:41:35 -0700, Bjorn Helgaas <bjorn.helgaas@hp.com> said:

  Bjorn> I've tested it on my i2000, and it seems to work fine.  This
  Bjorn> is with firmware version

  Bjorn>     W460GXBS2.86E.0117C.P09.200108091154

  Bjorn> This is the only BIOS version I found mentioned on
  Bjorn> www.hp.com, so I assume it's the latest official version.

I'm all for the patch then.

	--david

RE: [PATCH] fix PCI interrupt setting for ia64

[David Mosberger]

Kenji,

Your patch failed to apply apparently because all tabs got replaced by
blanks.  I fixed it by hand for now, but in the future, I'd appreciate
it if you could either fix your mailer config so it doesn't muck with
whitespace (or don't paste & cut patches if that's how the patch got
mangled).  As a last resort, send patches as a MIME attachment.

>>>>> On Thu, 11 Mar 2004 14:29:59 +0900, Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> said:

  Kenji> I think you are right. Probe_irq_on() still have another
  Kenji> problems even if my patch is applied. For example, if buggy
  Kenji> PCI device generates interrupts during its device driver
  Kenji> calls probe_irq_on(), these interrupts might be considered as
  Kenji> spurious and IRQ probing will fail. I think this is another
  Kenji> problem than I pointed out.

That should be OK.  The probe_irq_on() interface never was guaranteed
to work under all circumstances.

  Kenji> In addition to this, if probe_irq_on() is used for PCI
  Kenji> interrupts (level triggered), interrupts are generated
  Kenji> repeatedly because there are no handlers which clears these
  Kenji> interrupt request. But I think this is not a problem, because
  Kenji> these interrupts will be masked by probe_irq_on() or
  Kenji> probe_irq_off() soon. If this is a problem, I think
  Kenji> probe_irq_on() should never be used for PCI (level triggered)
  Kenji> interrupt probing.

That's a good point but I doubt it's going to cause trouble because of
the history behind this interface (it's mostly needed for ISA
devices).  If it did, it wouldn't be hard to fix, but I'm not sure
it's worth bothering.  Perhaps it would be a good idea to get rid of
the interface altogether in the 2.7 timeframe (for ia64 and other
ISA-free platforms at least; I don't think it can be gotten rid of for
x86).

	--david

RE: [PATCH] fix PCI interrupt setting for ia64

[David Mosberger]

>>>>> On Thu, 11 Mar 2004 09:34:06 +0900, Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> said:

  Kenji> Hi, I'm sorry that the report falls behind. I wanted to check
  Kenji> out by using real device driver which uses a probe_irq_on(),
  Kenji> but I don't have appropriate environment now.

  Kenji> Though I didn't check out on a real machine yet, I believe my
  Kenji> patch doesn't have any influence on probe_irq_on() because
  Kenji> current probe_irq_on() calls startup callback to unmask the
  Kenji> RTEs as you said before.

OK, I agree.

Thanks,

	--david

RE: [PATCH] fix PCI interrupt setting for ia64

[Kenji Kaneshige]

Hi,

Sorry, I misunderstood your concern.

I think you are right. Probe_irq_on() still have another problems even if my
patch
is applied. For example, if buggy PCI device generates interrupts during its
device
driver calls probe_irq_on(), these interrupts might be considered as
spurious and
IRQ probing will fail. I think this is another problem than I pointed out.

In addition to this, if probe_irq_on() is used for PCI interrupts (level
triggered),
interrupts are generated repeatedly because there are no handlers which
clears
these interrupt request. But I think this is not a problem, because these
interrupts
will be masked by probe_irq_on() or probe_irq_off() soon. If this is a
problem, I think
probe_irq_on() should never be used for PCI (level triggered) interrupt
probing.

Regards,
Kenji Kaneshige


> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org
> [mailto:linux-ia64-owner@vger.kernel.org]On Behalf Of Takayoshi Kochi
> Sent: Thursday, March 11, 2004 10:35 AM
> To: kaneshige.kenji@jp.fujitsu.com
> Cc: davidm@hpl.hp.com; linux-ia64@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] fix PCI interrupt setting for ia64
>
>
> Hi,
>
> From: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Subject: RE: [PATCH] fix PCI interrupt setting for ia64
> Date: Thu, 11 Mar 2004 09:34:06 +0900
>
> > Hi,
> >
> > I'm sorry that the report falls behind. I wanted to check out by using
> > real device driver which uses a probe_irq_on(), but I don't
> have appropriate
> > environment now.
> >
> > Though I didn't check out on a real machine yet, I believe my
> patch doesn't
> > have any influence on probe_irq_on() because current
> probe_irq_on() calls
> > startup callback to unmask the RTEs as you said before.
>
> My concern was that if there's a buggy PCI device that raises
> interrupts all the time until it's initialized by some device driver,
> probe_irq_on() would not work as expected regardless of whether
> your patch is applied or not.  I thought masking the interrupt line
> doesn't work around this case.
>
> ---
> Takayoshi Kochi <t-kochi@bq.jp.nec.com>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix PCI interrupt setting for ia64

[Takayoshi Kochi]

Hi,

From: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Subject: RE: [PATCH] fix PCI interrupt setting for ia64
Date: Thu, 11 Mar 2004 09:34:06 +0900

> Hi,
> 
> I'm sorry that the report falls behind. I wanted to check out by using
> real device driver which uses a probe_irq_on(), but I don't have appropriate
> environment now.
> 
> Though I didn't check out on a real machine yet, I believe my patch doesn't
> have any influence on probe_irq_on() because current probe_irq_on() calls
> startup callback to unmask the RTEs as you said before.

My concern was that if there's a buggy PCI device that raises
interrupts all the time until it's initialized by some device driver,
probe_irq_on() would not work as expected regardless of whether
your patch is applied or not.  I thought masking the interrupt line
doesn't work around this case.

---
Takayoshi Kochi <t-kochi@bq.jp.nec.com>

RE: [PATCH] fix PCI interrupt setting for ia64

[Kenji Kaneshige]

Hi,

I'm sorry that the report falls behind. I wanted to check out by using
real device driver which uses a probe_irq_on(), but I don't have appropriate
environment now.

Though I didn't check out on a real machine yet, I believe my patch doesn't
have any influence on probe_irq_on() because current probe_irq_on() calls
startup callback to unmask the RTEs as you said before.

Regards,
Kenji Kaneshige

> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org
> [mailto:linux-ia64-owner@vger.kernel.org]On Behalf Of David Mosberger
> Sent: Thursday, March 11, 2004 5:10 AM
> To: Kenji Kaneshige
> Cc: linux-ia64@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] fix PCI interrupt setting for ia64
>
>
> Kenji,
>
> Sorry, I lost track of the status of this patch.  Has it been checked
> out OK with respect to interrupt probing?
>
> 	--david
>
> >>>>> On Mon, 08 Mar 2004 11:49:10 +0900, Kenji Kaneshige
> <kaneshige.kenji@jp.fujitsu.com> said:
>
>   Kenji> Hi, In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are
>   Kenji> unmasked at the boot time before installing device drivers. I
>   Kenji> think it is very dangerous.  If some PCI devices without
>   Kenji> device driver generate interrupts, interrupts are generated
>   Kenji> repeatedly because these interrupt requests are never
>   Kenji> cleared. I think RTEs for PCI interrupts should be unmasked
>   Kenji> by device driver.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix PCI interrupt setting for ia64

[David Mosberger]

Kenji,

Sorry, I lost track of the status of this patch.  Has it been checked
out OK with respect to interrupt probing?

	--david

>>>>> On Mon, 08 Mar 2004 11:49:10 +0900, Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> said:

  Kenji> Hi, In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are
  Kenji> unmasked at the boot time before installing device drivers. I
  Kenji> think it is very dangerous.  If some PCI devices without
  Kenji> device driver generate interrupts, interrupts are generated
  Kenji> repeatedly because these interrupt requests are never
  Kenji> cleared. I think RTEs for PCI interrupts should be unmasked
  Kenji> by device driver.

Re: [PATCH] fix PCI interrupt setting for ia64

[David Mosberger]

>>>>> On Sun, 7 Mar 2004 22:30:44 -0800, Grant Grundler <iod00d@hp.com> said:

  Grant> Hi Kenji, I think this behavior exists to support "legacy"
  Grant> IRQ probing.

I don't think so.  probe_irq_on() calls the "startup" callback which
should do an unmask_irq() for I/O SAPIC.  Can somebody confirm that a
Merced box (without Bjorn's patch) will boot fine with Kenji
Kaneshige's patch applied?

	--david

RE: [PATCH] fix PCI interrupt setting for ia64

[Kenji Kaneshige]

Hi,

As a matter of fact, I don't have special reason to leave RTEs unmasked in
iosapic_register_intr(), iosapic_register_platform_intr(),
iosapic_override_isa_irq().
I think it is better that interrupts are unmasked by individual device
drivers, but
there are some exceptions. For example, PMI and INIT don't need device
drivers.
So I think more investigation is needed about them.

Regards,
Kenji Kaneshige

> -----Original Message-----
> From: Liu, Benjamin [mailto:benjamin.liu@intel.com]
> Sent: Monday, March 08, 2004 6:15 PM
> To: Kenji Kaneshige; linux-ia64@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] fix PCI interrupt setting for ia64
>
>
> Thank you for the information, Kenji. But is there any reason to
> leave it unmasked in iosapic_register_intr(),
> iosapic_register_platform_intr(), iosapic_override_isa_irq(),
> given the fact that they would be unmasked finally in individual
> device drivers?

RE: [PATCH] fix PCI interrupt setting for ia64

[Liu, Benjamin]

Thank you for the information, Kenji. But is there any reason to leave it unmasked in iosapic_register_intr(), iosapic_register_platform_intr(), iosapic_override_isa_irq(), given the fact that they would be unmasked finally in individual device drivers?

Thanks,
Pingping (Benjamin) Liu
Intel China Software Center


>-----Original Message-----
>From: Kenji Kaneshige [mailto:kaneshige.kenji@jp.fujitsu.com] 
>Sent: 2004年3月8日 16:31
>To: Liu, Benjamin; Kenji Kaneshige; linux-ia64@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org
>Subject: RE: [PATCH] fix PCI interrupt setting for ia64
>
>
>Hi Liu,
>
>I think it is not converse.
>What I wanted to do was to mask (disable) IRQ in iosapic_enable_intr().
>I think PCI IRQ should be unmasked (enabled) by device driver, not by
>iosapic_enable_intr() at the boot time.
>
>Regards,
>Kenji Kaneshige
>
>> -----Original Message-----
>> From: linux-ia64-owner@vger.kernel.org
>> [mailto:linux-ia64-owner@vger.kernel.org]On Behalf Of Liu, Benjamin
>> Sent: Monday, March 08, 2004 4:34 PM
>> To: Kenji Kaneshige; linux-ia64@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Subject: RE: [PATCH] fix PCI interrupt setting for ia64
>>
>>
>> Hi, Kenji,
>>
>> I am a little bit confused with the patch:
>> Mask bit of RDL's low word indicates the interrupt is enabled
>> when it is zero, disabled when it is 1. But from the patch below,
>> it seems that Kenji enable interrupts in iosapic_register_intr(),
>> iosapic_register_platform_intr(), iosapic_override_isa_irq(), and
>> disable the interrupt in iosapic_enable_intr(). Is it converse?
>>
>> Thanks,
>> Pingping (Benjamin) Liu
>> Intel China Software Center
>>
>>
>> >-----Original Message-----
>> >From: linux-ia64-owner@vger.kernel.org
>> >[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of 
>Kenji Kaneshige
>> >Sent: 2004年3月8日 10:49
>> >To: linux-ia64@vger.kernel.org
>> >Cc: linux-kernel@vger.kernel.org
>> >Subject: [PATCH] fix PCI interrupt setting for ia64
>> >
>> >
>> >Hi,
>> >
>> >In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are 
>unmasked at the
>> >boot time before installing device drivers. I think it is very
>> >dangerous.
>> >If some PCI devices without device driver generate interrupts,
>> >interrupts
>> >are generated repeatedly because these interrupt requests are never
>> >cleared. I think RTEs for PCI interrupts should be unmasked 
>by device
>> >driver.
>> >
>> >A following patch fixes this issue.
>> >
>> >Regards,
>> >Kenji Kaneshige
>> >
>> >
>> >diff -Naur linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c
>> >linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c
>> >--- linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c  2004-03-05
>> >15:13:53.155237277 +0900
>> >+++ linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c  2004-03-05
>> >16:48:31.856142526 +0900
>> >@@ -170,7 +170,7 @@
>> > }
>> >
>> > static void
>> >-set_rte (unsigned int vector, unsigned int dest)
>> >+set_rte (unsigned int vector, unsigned int dest, int mask)
>> > {
>> >        unsigned long pol, trigger, dmode;
>> >        u32 low32, high32;
>> >@@ -205,6 +205,7 @@
>> >        low32 = ((pol << IOSAPIC_POLARITY_SHIFT) |
>> >                 (trigger << IOSAPIC_TRIGGER_SHIFT) |
>> >                 (dmode << IOSAPIC_DELIVERY_SHIFT) |
>> >+                ((mask ? 1 : 0) << IOSAPIC_MASK_SHIFT) |
>> >                 vector);
>> >
>> >        /* dest contains both id and eid */
>> >@@ -509,7 +510,7 @@
>> >               (trigger == IOSAPIC_EDGE ? "edge" : "level"),
>> >dest, vector);
>> >
>> >        /* program the IOSAPIC routing table */
>> >-       set_rte(vector, dest);
>> >+       set_rte(vector, dest, 0);
>> >        return vector;
>> > }
>> >
>> >@@ -557,7 +558,7 @@
>> >               (trigger == IOSAPIC_EDGE ? "edge" : "level"),
>> >dest, vector);
>> >
>> >        /* program the IOSAPIC routing table */
>> >-       set_rte(vector, dest);
>> >+       set_rte(vector, dest, 0);
>> >        return vector;
>> > }
>> >
>> >@@ -583,7 +584,7 @@
>> >            trigger == IOSAPIC_EDGE ? "edge" : "level", 
>dest, vector);
>> >
>> >        /* program the IOSAPIC routing table */
>> >-       set_rte(vector, dest);
>> >+       set_rte(vector, dest, 0);
>> > }
>> >
>> > void __init
>> >@@ -669,7 +670,7 @@
>> >        /* direct the interrupt vector to the running cpu id */
>> >        dest = (ia64_getreg(_IA64_REG_CR_LID) >> 16) & 0xffff;
>> > #endif
>> >-       set_rte(vector, dest);
>> >+       set_rte(vector, dest, 1);
>> >
>> >        printk(KERN_INFO "IOSAPIC: vector %d -> CPU 0x%04x, 
>enabled\n",
>> >               vector, dest);
>> >
>> >-
>> >To unsubscribe from this list: send the line "unsubscribe
>> >linux-ia64" in
>> >the body of a message to majordomo@vger.kernel.org
>> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>> -
>> To unsubscribe from this list: send the line "unsubscribe 
>linux-ia64" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

RE: [PATCH] fix PCI interrupt setting for ia64

[Kenji Kaneshige]

Hi Liu,

I think it is not converse.
What I wanted to do was to mask (disable) IRQ in iosapic_enable_intr().
I think PCI IRQ should be unmasked (enabled) by device driver, not by
iosapic_enable_intr() at the boot time.

Regards,
Kenji Kaneshige

> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org
> [mailto:linux-ia64-owner@vger.kernel.org]On Behalf Of Liu, Benjamin
> Sent: Monday, March 08, 2004 4:34 PM
> To: Kenji Kaneshige; linux-ia64@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] fix PCI interrupt setting for ia64
>
>
> Hi, Kenji,
>
> I am a little bit confused with the patch:
> Mask bit of RDL's low word indicates the interrupt is enabled
> when it is zero, disabled when it is 1. But from the patch below,
> it seems that Kenji enable interrupts in iosapic_register_intr(),
> iosapic_register_platform_intr(), iosapic_override_isa_irq(), and
> disable the interrupt in iosapic_enable_intr(). Is it converse?
>
> Thanks,
> Pingping (Benjamin) Liu
> Intel China Software Center
>
>
> >-----Original Message-----
> >From: linux-ia64-owner@vger.kernel.org
> >[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Kenji Kaneshige
> >Sent: 2004年3月8日 10:49
> >To: linux-ia64@vger.kernel.org
> >Cc: linux-kernel@vger.kernel.org
> >Subject: [PATCH] fix PCI interrupt setting for ia64
> >
> >
> >Hi,
> >
> >In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are unmasked at the
> >boot time before installing device drivers. I think it is very
> >dangerous.
> >If some PCI devices without device driver generate interrupts,
> >interrupts
> >are generated repeatedly because these interrupt requests are never
> >cleared. I think RTEs for PCI interrupts should be unmasked by device
> >driver.
> >
> >A following patch fixes this issue.
> >
> >Regards,
> >Kenji Kaneshige
> >
> >
> >diff -Naur linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c
> >linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c
> >--- linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c  2004-03-05
> >15:13:53.155237277 +0900
> >+++ linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c  2004-03-05
> >16:48:31.856142526 +0900
> >@@ -170,7 +170,7 @@
> > }
> >
> > static void
> >-set_rte (unsigned int vector, unsigned int dest)
> >+set_rte (unsigned int vector, unsigned int dest, int mask)
> > {
> >        unsigned long pol, trigger, dmode;
> >        u32 low32, high32;
> >@@ -205,6 +205,7 @@
> >        low32 = ((pol << IOSAPIC_POLARITY_SHIFT) |
> >                 (trigger << IOSAPIC_TRIGGER_SHIFT) |
> >                 (dmode << IOSAPIC_DELIVERY_SHIFT) |
> >+                ((mask ? 1 : 0) << IOSAPIC_MASK_SHIFT) |
> >                 vector);
> >
> >        /* dest contains both id and eid */
> >@@ -509,7 +510,7 @@
> >               (trigger == IOSAPIC_EDGE ? "edge" : "level"),
> >dest, vector);
> >
> >        /* program the IOSAPIC routing table */
> >-       set_rte(vector, dest);
> >+       set_rte(vector, dest, 0);
> >        return vector;
> > }
> >
> >@@ -557,7 +558,7 @@
> >               (trigger == IOSAPIC_EDGE ? "edge" : "level"),
> >dest, vector);
> >
> >        /* program the IOSAPIC routing table */
> >-       set_rte(vector, dest);
> >+       set_rte(vector, dest, 0);
> >        return vector;
> > }
> >
> >@@ -583,7 +584,7 @@
> >            trigger == IOSAPIC_EDGE ? "edge" : "level", dest, vector);
> >
> >        /* program the IOSAPIC routing table */
> >-       set_rte(vector, dest);
> >+       set_rte(vector, dest, 0);
> > }
> >
> > void __init
> >@@ -669,7 +670,7 @@
> >        /* direct the interrupt vector to the running cpu id */
> >        dest = (ia64_getreg(_IA64_REG_CR_LID) >> 16) & 0xffff;
> > #endif
> >-       set_rte(vector, dest);
> >+       set_rte(vector, dest, 1);
> >
> >        printk(KERN_INFO "IOSAPIC: vector %d -> CPU 0x%04x, enabled\n",
> >               vector, dest);
> >
> >-
> >To unsubscribe from this list: send the line "unsubscribe
> >linux-ia64" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] fix PCI interrupt setting for ia64

[Liu, Benjamin]

Hi, Kenji,

I am a little bit confused with the patch:
Mask bit of RDL's low word indicates the interrupt is enabled when it is zero, disabled when it is 1. But from the patch below, it seems that Kenji enable interrupts in iosapic_register_intr(), iosapic_register_platform_intr(), iosapic_override_isa_irq(), and disable the interrupt in iosapic_enable_intr(). Is it converse?

Thanks,
Pingping (Benjamin) Liu
Intel China Software Center


>-----Original Message-----
>From: linux-ia64-owner@vger.kernel.org 
>[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Kenji Kaneshige
>Sent: 2004年3月8日 10:49
>To: linux-ia64@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org
>Subject: [PATCH] fix PCI interrupt setting for ia64
>
>
>Hi,
>
>In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are unmasked at the
>boot time before installing device drivers. I think it is very 
>dangerous.
>If some PCI devices without device driver generate interrupts, 
>interrupts
>are generated repeatedly because these interrupt requests are never
>cleared. I think RTEs for PCI interrupts should be unmasked by device
>driver.
>
>A following patch fixes this issue.
>
>Regards,
>Kenji Kaneshige
>
>
>diff -Naur linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c
>linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c
>--- linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c  2004-03-05
>15:13:53.155237277 +0900
>+++ linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c  2004-03-05
>16:48:31.856142526 +0900
>@@ -170,7 +170,7 @@
> }
>
> static void
>-set_rte (unsigned int vector, unsigned int dest)
>+set_rte (unsigned int vector, unsigned int dest, int mask)
> {
>        unsigned long pol, trigger, dmode;
>        u32 low32, high32;
>@@ -205,6 +205,7 @@
>        low32 = ((pol << IOSAPIC_POLARITY_SHIFT) |
>                 (trigger << IOSAPIC_TRIGGER_SHIFT) |
>                 (dmode << IOSAPIC_DELIVERY_SHIFT) |
>+                ((mask ? 1 : 0) << IOSAPIC_MASK_SHIFT) |
>                 vector);
>
>        /* dest contains both id and eid */
>@@ -509,7 +510,7 @@
>               (trigger == IOSAPIC_EDGE ? "edge" : "level"), 
>dest, vector);
>
>        /* program the IOSAPIC routing table */
>-       set_rte(vector, dest);
>+       set_rte(vector, dest, 0);
>        return vector;
> }
>
>@@ -557,7 +558,7 @@
>               (trigger == IOSAPIC_EDGE ? "edge" : "level"), 
>dest, vector);
>
>        /* program the IOSAPIC routing table */
>-       set_rte(vector, dest);
>+       set_rte(vector, dest, 0);
>        return vector;
> }
>
>@@ -583,7 +584,7 @@
>            trigger == IOSAPIC_EDGE ? "edge" : "level", dest, vector);
>
>        /* program the IOSAPIC routing table */
>-       set_rte(vector, dest);
>+       set_rte(vector, dest, 0);
> }
>
> void __init
>@@ -669,7 +670,7 @@
>        /* direct the interrupt vector to the running cpu id */
>        dest = (ia64_getreg(_IA64_REG_CR_LID) >> 16) & 0xffff;
> #endif
>-       set_rte(vector, dest);
>+       set_rte(vector, dest, 1);
>
>        printk(KERN_INFO "IOSAPIC: vector %d -> CPU 0x%04x, enabled\n",
>               vector, dest);
>
>-
>To unsubscribe from this list: send the line "unsubscribe 
>linux-ia64" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] fix PCI interrupt setting for ia64

[Grant Grundler]

On Mon, Mar 08, 2004 at 11:49:10AM +0900, Kenji Kaneshige wrote:
> In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are unmasked at the
> boot time before installing device drivers. I think it is very dangerous.

Hi Kenji,
I think this behavior exists to support "legacy" IRQ probing.
I'm wondering if it would be sufficient to wrap the patch in
"#ifndef CONFIG_ISA" or something like that.

grant

[PATCH] fix PCI interrupt setting for ia64

[Kenji Kaneshige]

Hi,

In ia64 kernel, IOSAPIC's RTEs for PCI interrupts are unmasked at the
boot time before installing device drivers. I think it is very dangerous.
If some PCI devices without device driver generate interrupts, interrupts
are generated repeatedly because these interrupt requests are never
cleared. I think RTEs for PCI interrupts should be unmasked by device
driver.

A following patch fixes this issue.

Regards,
Kenji Kaneshige


diff -Naur linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c
linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c
--- linux-2.6.4-rc2/arch/ia64/kernel/iosapic.c  2004-03-05
15:13:53.155237277 +0900
+++ linux-2.6.4-rc2-changed/arch/ia64/kernel/iosapic.c  2004-03-05
16:48:31.856142526 +0900
@@ -170,7 +170,7 @@
 }

 static void
-set_rte (unsigned int vector, unsigned int dest)
+set_rte (unsigned int vector, unsigned int dest, int mask)
 {
        unsigned long pol, trigger, dmode;
        u32 low32, high32;
@@ -205,6 +205,7 @@
        low32 = ((pol << IOSAPIC_POLARITY_SHIFT) |
                 (trigger << IOSAPIC_TRIGGER_SHIFT) |
                 (dmode << IOSAPIC_DELIVERY_SHIFT) |
+                ((mask ? 1 : 0) << IOSAPIC_MASK_SHIFT) |
                 vector);

        /* dest contains both id and eid */
@@ -509,7 +510,7 @@
               (trigger == IOSAPIC_EDGE ? "edge" : "level"), dest, vector);

        /* program the IOSAPIC routing table */
-       set_rte(vector, dest);
+       set_rte(vector, dest, 0);
        return vector;
 }

@@ -557,7 +558,7 @@
               (trigger == IOSAPIC_EDGE ? "edge" : "level"), dest, vector);

        /* program the IOSAPIC routing table */
-       set_rte(vector, dest);
+       set_rte(vector, dest, 0);
        return vector;
 }

@@ -583,7 +584,7 @@
            trigger == IOSAPIC_EDGE ? "edge" : "level", dest, vector);

        /* program the IOSAPIC routing table */
-       set_rte(vector, dest);
+       set_rte(vector, dest, 0);
 }

 void __init
@@ -669,7 +670,7 @@
        /* direct the interrupt vector to the running cpu id */
        dest = (ia64_getreg(_IA64_REG_CR_LID) >> 16) & 0xffff;
 #endif
-       set_rte(vector, dest);
+       set_rte(vector, dest, 1);

        printk(KERN_INFO "IOSAPIC: vector %d -> CPU 0x%04x, enabled\n",
               vector, dest);