> this is mostly a naming issue, and i agree it should be cleaned up some
> more . Right now the 'fastack' handler just cheats a bit and pretends
> that ->ack() is the EOI. But you are right and we could as well
> introduce a separate ->eoi() chip method that clearly separates an
> ack+mask+unmask based chip from an ->eoi chip. [I'd not reuse the
> ->end() method because that really is a flow thing in many places and
> the conversion is cleaner. Having one more entry in the chip operations
> structure is no big issue.]
Hrm... ok. Not sure I agree with adding one more callback but it doesn't
matter much.
Thing is, end() isn't used anymore at all now. Thus it's just basically
renaming end() to eoi() except that end() is still there for whoever
uses __do_IRQ() and ... handle_percpu_irq(). Doesn't make that much
sense to me. So I suppose you should also change handle_percpu_irq() to
use eoi() then and consider end() to be "legacy" (to be used only by
__do_IRQ) ?
> > [...] Beside, eoi _must_ be called wether it was masked in between or
> > not or the controller will lose track.
>
> correct - and that must happen for x86 APICs too. This is a plain bug in
> the fastack handler.
>
> sounds like a plan? The patch below works fine for me.
The patch is _almost_ right to me :) I don't need the
if (unlikely(desc->status & IRQ_DISABLED))
desc->chip->mask(irq);
At all. I suppose it won't harm, but it shouldn't be necessary for me and I'm not sure why it's
necessary on IO_APIC neither (but then I don't know those very well).
Ben.
>
> Ingo
>
> ----------------
> Subject: genirq: add chip->eoi(), fastack -> fasteoi
> From: Ingo Molnar <[email protected]>
>
> clean up the fastack concept by turning it into fasteoi and
> introducing the ->eoi() method for chips.
>
> this also allows the cleanup of an i386 EOI quirk - now the
> quirk is cleanly separated from the pure ACK implementation.
>
> the patch also enables the fasteoi handler for i386 level-triggered
> IO-APIC irqs.
>
> tested on i386 and x86_64 with MSI on and off as well.
>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> arch/i386/kernel/io_apic.c | 28 +++++++++++++++++++---------
> arch/x86_64/kernel/io_apic.c | 9 +++++----
> include/linux/irq.h | 6 ++++--
> kernel/irq/chip.c | 25 ++++++++++++-------------
> 4 files changed, 40 insertions(+), 28 deletions(-)
>
> Index: linux/arch/i386/kernel/io_apic.c
> ===================================================================
> --- linux.orig/arch/i386/kernel/io_apic.c
> +++ linux/arch/i386/kernel/io_apic.c
> @@ -1214,7 +1214,7 @@ static void ioapic_register_intr(int irq
> if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
> trigger == IOAPIC_LEVEL)
> set_irq_chip_and_handler(idx, &ioapic_chip,
> - handle_level_irq);
> + handle_fasteoi_irq);
> else
> set_irq_chip_and_handler(idx, &ioapic_chip,
> handle_edge_irq);
> @@ -1946,6 +1946,12 @@ static unsigned int startup_ioapic_irq(u
> return was_pending;
> }
>
> +static void ack_ioapic_irq(unsigned int irq)
> +{
> + move_irq(irq);
> + ack_APIC_irq();
> +}
> +
> static void ack_ioapic_quirk_irq(unsigned int irq)
> {
> unsigned long v;
> @@ -1971,11 +1977,6 @@ static void ack_ioapic_quirk_irq(unsigne
> * operation to prevent an edge-triggered interrupt escaping meanwhile.
> * The idea is from Manfred Spraul. --macro
> */
> - if (irq_desc[irq].handle_irq == handle_edge_irq) {
> - ack_APIC_irq();
> - return;
> - }
> -
> i = IO_APIC_VECTOR(irq);
>
> v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
> @@ -1998,6 +1999,14 @@ static unsigned int startup_ioapic_vecto
> return startup_ioapic_irq(irq);
> }
>
> +static void ack_ioapic_vector(unsigned int vector)
> +{
> + int irq = vector_to_irq(vector);
> +
> + move_native_irq(vector);
> + ack_ioapic_irq(irq);
> +}
> +
> static void ack_ioapic_quirk_vector(unsigned int vector)
> {
> int irq = vector_to_irq(vector);
> @@ -2050,7 +2059,8 @@ static struct irq_chip ioapic_chip __rea
> .startup = startup_ioapic_vector,
> .mask = mask_IO_APIC_vector,
> .unmask = unmask_IO_APIC_vector,
> - .ack = ack_ioapic_quirk_vector,
> + .ack = ack_ioapic_vector,
> + .eoi = ack_ioapic_quirk_vector,
> #ifdef CONFIG_SMP
> .set_affinity = set_ioapic_affinity,
> #endif
> @@ -2124,7 +2134,7 @@ static struct irq_chip lapic_chip __read
> .name = "local-APIC-edge",
> .mask = mask_lapic_irq,
> .unmask = unmask_lapic_irq,
> - .ack = ack_apic,
> + .eoi = ack_apic,
> };
>
> static void setup_nmi (void)
> @@ -2305,7 +2315,7 @@ static inline void check_timer(void)
> printk(KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
>
> disable_8259A_irq(0);
> - set_irq_chip_and_handler(0, &lapic_chip, handle_fastack_irq);
> + set_irq_chip_and_handler(0, &lapic_chip, handle_fasteoi_irq);
> apic_write_around(APIC_LVT0, APIC_DM_FIXED | vector); /* Fixed mode */
> enable_8259A_irq(0);
>
> Index: linux/arch/x86_64/kernel/io_apic.c
> ===================================================================
> --- linux.orig/arch/x86_64/kernel/io_apic.c
> +++ linux/arch/x86_64/kernel/io_apic.c
> @@ -865,7 +865,7 @@ static void ioapic_register_intr(int irq
> if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
> trigger == IOAPIC_LEVEL)
> set_irq_chip_and_handler(idx, &ioapic_chip,
> - handle_fastack_irq);
> + handle_fasteoi_irq);
> else
> set_irq_chip_and_handler(idx, &ioapic_chip,
> handle_edge_irq);
> @@ -970,7 +970,7 @@ static void __init setup_ExtINT_IRQ0_pin
> * The timer IRQ doesn't have to know that behind the
> * scene we have a 8259A-master in AEOI mode ...
> */
> - set_irq_chip_and_handler(0, &ioapic_chip, handle_fastack_irq);
> + set_irq_chip_and_handler(0, &ioapic_chip, handle_fasteoi_irq);
>
> /*
> * Add it to the IO-APIC irq-routing table:
> @@ -1563,6 +1563,7 @@ static struct irq_chip ioapic_chip __rea
> .mask = mask_ioapic_vector,
> .unmask = unmask_ioapic_vector,
> .ack = ack_apic,
> + .eoi = ack_apic,
> #ifdef CONFIG_SMP
> .set_affinity = set_ioapic_affinity_vector,
> #endif
> @@ -1626,7 +1627,7 @@ static struct irq_chip lapic_chip __read
> .name = "local-APIC-edge",
> .mask = mask_lapic_irq,
> .unmask = unmask_lapic_irq,
> - .ack = ack_apic,
> + .eoi = ack_apic,
> };
>
> static void setup_nmi (void)
> @@ -1808,7 +1809,7 @@ static inline void check_timer(void)
> apic_printk(APIC_VERBOSE, KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
>
> disable_8259A_irq(0);
> - set_irq_chip_and_handler(0, &lapic_chip, handle_fastack_irq);
> + set_irq_chip_and_handler(0, &lapic_chip, handle_fasteoi_irq);
> apic_write(APIC_LVT0, APIC_DM_FIXED | vector); /* Fixed mode */
> enable_8259A_irq(0);
>
> Index: linux/include/linux/irq.h
> ===================================================================
> --- linux.orig/include/linux/irq.h
> +++ linux/include/linux/irq.h
> @@ -73,7 +73,8 @@ struct proc_dir_entry;
> * @mask: mask an interrupt source
> * @mask_ack: ack and mask an interrupt source
> * @unmask: unmask an interrupt source
> - * @end: end of interrupt
> + * @eoi: end of interrupt - chip level
> + * @end: end of interrupt - flow level
> * @set_affinity: set the CPU affinity on SMP machines
> * @retrigger: resend an IRQ to the CPU
> * @set_type: set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
> @@ -93,6 +94,7 @@ struct irq_chip {
> void (*mask)(unsigned int irq);
> void (*mask_ack)(unsigned int irq);
> void (*unmask)(unsigned int irq);
> + void (*eoi)(unsigned int irq);
>
> void (*end)(unsigned int irq);
> void (*set_affinity)(unsigned int irq, cpumask_t dest);
> @@ -286,7 +288,7 @@ extern int handle_IRQ_event(unsigned int
> extern void fastcall
> handle_level_irq(unsigned int irq, struct irq_desc *desc, struct pt_regs *regs);
> extern void fastcall
> -handle_fastack_irq(unsigned int irq, struct irq_desc *desc,
> +handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc,
> struct pt_regs *regs);
> extern void fastcall
> handle_edge_irq(unsigned int irq, struct irq_desc *desc, struct pt_regs *regs);
> Index: linux/kernel/irq/chip.c
> ===================================================================
> --- linux.orig/kernel/irq/chip.c
> +++ linux/kernel/irq/chip.c
> @@ -280,18 +280,18 @@ out:
> }
>
> /**
> - * handle_fastack_irq - irq handler for transparent controllers
> + * handle_fasteoi_irq - irq handler for transparent controllers
> * @irq: the interrupt number
> * @desc: the interrupt description structure for this irq
> * @regs: pointer to a register structure
> *
> - * Only a single callback will be issued to the chip: an ->ack()
> + * Only a single callback will be issued to the chip: an ->eoi()
> * call when the interrupt has been serviced. This enables support
> * for modern forms of interrupt handlers, which handle the flow
> * details in hardware, transparently.
> */
> void fastcall
> -handle_fastack_irq(unsigned int irq, struct irq_desc *desc,
> +handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc,
> struct pt_regs *regs)
> {
> unsigned int cpu = smp_processor_id();
> @@ -324,10 +324,9 @@ handle_fastack_irq(unsigned int irq, str
> spin_lock(&desc->lock);
> desc->status &= ~IRQ_INPROGRESS;
> out:
> - if (!(desc->status & IRQ_DISABLED))
> - desc->chip->ack(irq);
> - else
> + if (unlikely(desc->status & IRQ_DISABLED))
> desc->chip->mask(irq);
> + desc->chip->eoi(irq);
>
> spin_unlock(&desc->lock);
> }
> @@ -507,19 +506,19 @@ handle_irq_name(void fastcall (*handle)(
> struct pt_regs *))
> {
> if (handle == handle_level_irq)
> - return "level ";
> - if (handle == handle_fastack_irq)
> - return "level ";
> + return "level ";
> + if (handle == handle_fasteoi_irq)
> + return "fasteoi";
> if (handle == handle_edge_irq)
> - return "edge ";
> + return "edge ";
> if (handle == handle_simple_irq)
> - return "simple";
> + return "simple ";
> #ifdef CONFIG_SMP
> if (handle == handle_percpu_irq)
> - return "percpu";
> + return "percpu ";
> #endif
> if (handle == handle_bad_irq)
> - return "bad ";
> + return "bad ";
>
> return NULL;
> }
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
