lkml.org 
[lkml]   [2012]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH v1 02/31] ARC: irqflags
    On Wed, 7 Nov 2012, Vineet Gupta wrote:
    > + ******************************************************************
    > + * Inline ASM macros to read/write AUX Regs
    > + * Essentially invocation of lr/sr insns from "C"
    > + */
    > +
    > +#if 1

    Leftover ???

    > +#define read_aux_reg(reg) __builtin_arc_lr(reg)
    > +
    > +/* gcc builtin sr needs reg param to be long immediate */
    > +#define write_aux_reg(reg_immed, val) \
    > + __builtin_arc_sr((unsigned int)val, reg_immed)
    > +
    > +#else
    > +/*
    > + * Conditionally Enable IRQs

    Unconditionally methinks


    The following two functions are related to irq chips I guess. So why
    would you want them here ?

    > +static inline void arch_mask_irq(unsigned int irq)
    > +{
    > + unsigned int ienb;
    > +
    > + ienb = read_aux_reg(AUX_IENABLE);
    > + ienb &= ~(1 << irq);
    > + write_aux_reg(AUX_IENABLE, ienb);
    > +}
    > +
    > +static inline void arch_unmask_irq(unsigned int irq)
    > +{
    > + unsigned int ienb;
    > +
    > + ienb = read_aux_reg(AUX_IENABLE);
    > + ienb |= (1 << irq);
    > + write_aux_reg(AUX_IENABLE, ienb);
    > +}

    The only user is the interrupt controller code, right?

    > diff --git a/arch/arc/kernel/irq.c b/arch/arc/kernel/irq.c
    > new file mode 100644
    > index 0000000..16fcbe8
    > --- /dev/null
    > +++ b/arch/arc/kernel/irq.c
    > @@ -0,0 +1,32 @@
    > +/*
    > + * Copyright (C) 2011-12 Synopsys, Inc. (www.synopsys.com)
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License version 2 as
    > + * published by the Free Software Foundation.
    > + *
    > + */
    > +
    > +#include <linux/interrupt.h>
    > +#include <linux/module.h>
    > +#include <asm/irqflags.h>
    > +#include <asm/arcregs.h>
    > +
    > +void arch_local_irq_enable(void)
    > +{
    > +
    > + unsigned long flags;
    > + flags = arch_local_save_flags();
    > + flags |= (STATUS_E1_MASK | STATUS_E2_MASK);
    > +
    > + /*
    > + * If called from hard ISR (between irq_enter and irq_exit)
    > + * don't allow Level 1. In Soft ISR we allow further Level 1s
    > + */
    > +
    > + if (in_irq())
    > + flags &= ~(STATUS_E1_MASK | STATUS_E2_MASK);

    Hmm. This looks weird and the comment is not very helpful. So using my
    crystal ball you want to enforce, that nothing enables interrupts
    while a hard interrupt handler is running, right?

    Is there a chip limitation which you have to enforce here? If yes,
    then please explain it.

    Btw, all hard interrupt handlers in Linux run with interrupts disabled and
    they are not supposed to reenable interrupts, which is true for almost
    all drivers except for a few archaic IDE drivers. In fact you might
    even WARN about it at least once, so the offending code gets fixed.

    Also the code flow is backwards. What about:

    unsigned long flags;

    if (in_irq())
    return;

    flags = ....


    > + arch_local_irq_restore(flags);
    > +}

    Thanks,

    tglx


    \
     
     \ /
      Last update: 2012-11-12 21:21    [W:4.048 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site