On Thu, 2013-10-10 at 10:32 +0200, Helge Deller wrote: > Hi James, > > On 10/10/2013 04:17 AM, James Bottomley wrote: > > On Wed, 2013-10-09 at 23:54 +0200, Helge Deller wrote: > >> Signed-off-by: Helge Deller <deller@xxxxxx> > >> > >> diff --git a/arch/parisc/kernel/irq.c b/arch/parisc/kernel/irq.c > >> index 2e6443b..c439c05 100644 > >> --- a/arch/parisc/kernel/irq.c > >> +++ b/arch/parisc/kernel/irq.c > >> @@ -529,8 +529,8 @@ void do_cpu_irq_mask(struct pt_regs *regs) > >> cpumask_t dest; > >> #endif > >> > >> - old_regs = set_irq_regs(regs); > >> local_irq_disable(); > >> + old_regs = set_irq_regs(regs); > > > > I don't quite understand why. set_irq_regs is just saving the current > > regs pointer. > > ...and setting a new one... > > > The design intent is to call it first thing in the > > interrupt routine but because of the way we use them, it makes no > > difference whether you do it before or after disabling interrupts > > because it's stacked. What was the reason for wanting to change it to a > > non-standard calling pattern? > > Is it really non-standard? Well, yes, x86 which is canonical tends to execute it immediately. > My first intention was to align the set_irq_regs() and entrance and exit > to the irq_enter() and irq_exit() functions. > With my change above it's now: > > old_regs = set_irq_regs(regs); > irq_enter(); > do_something...(); > irq_exit(); > set_irq_regs(old_regs); > > That's the same syntax as all other arches use. I honestly don't think it matters, but x86 does irq_ack irq_enter old_regs = set_irq_regs(regs); ... if you look in their apic code. > I think the main question is, if we need local_irq_disable() at all? The generic irq handler seems to expect it, so it looks like yes at the moment. I think the current pattern is that we call with disabled but the routine can re-enable. > At least moving the "old_regs = set_irq_regs(regs);" down after > local_irq_disable() > ensures that nobody else modifies the irq_regs pointer before we save > it into old_regs. Um, they can't. The regs pointer points to an on-stack saved area that was pushed when the interrupt was taken ... even if we get a nested interrupt, it will push a new stack frame and we'll still be back to this particular regs pointer when it returns. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
