| Message ID | 20131009215413.GA30163@p100.box (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Wed, 2013-10-09 at 23:54 +0200, Helge Deller wrote: > Signed-off-by: Helge Deller <deller@gmx.de> > > 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. 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? James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmx.de> >> >> 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? 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 think the main question is, if we need local_irq_disable() at all? 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. Helge -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I believe do_cpu_irq_mask is called with interrupts disabled (virt_map sets PSW to KERNEL_PSW). So, the local_irq_disable(); line can go. It does look like set_irq_regs has to be atomic on a per cpu basis. So, if interrupts weren't already disabled, there would be a problem with current code. Dave On 10/10/2013 4:32 AM, 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@gmx.de> >>> >>> 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? > > 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 think the main question is, if we need local_irq_disable() at all? > 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. > > Helge > > -- > To unsubscribe from this list: send the line "unsubscribe linux-parisc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
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@gmx.de> > >> > >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); irq_enter(); eirr_val = mfctl(23) & cpu_eiem & per_cpu(local_ack_eiem, cpu);
Signed-off-by: Helge Deller <deller@gmx.de> -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html