parisc: call set_irq_regs() after disabling local irqs

PATCH In review Helge Deller 3 participants 5 messages Oct 9, 2013 – Oct 11, 2013 PA-RISC architecture

lore

Share & tools

lore

Helge Deller

author maintainer

2013-10-09 21:54 UTC #0 lore raw

PATCH 1/1 parisc: call set_irq_regs() after disabling local irqs

Signed-off-by Helge Deller <[email protected]>

arch/parisc/kernel/irq.c +1 -1

index 2e6443b..c439c05 100644
@@ -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);

James Bottomley

maintainer

2013-10-10 02:17 UTC #1 lore raw

arch/parisc/kernel/irq.c void do_cpu_irq_mask(struct pt_regs *regs) · line 533

Quoted 13 lines · Helge Deller

Signed-off-by Helge Deller <[email protected]>

arch/parisc/kernel/irq.c +1 -1

index 2e6443b..c439c05 100644
@@ -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

Helge Deller

author maintainer

2013-10-10 08:32 UTC #2 lore raw

Hi James,

Quoted 11 lines · James Bottomley

Quoted 10 lines · Helge Deller
Signed-off-by Helge Deller <[email protected]>
arch/parisc/kernel/irq.c +0 -0
index 2e6443b..c439c05 100644
@@ -529,8 +529,8 @@ void do_cpu_irq_mask(struct pt_regs *regs)
 	cpumask_t dest;
 #endif
 

Quoted 6 lines

arch/parisc/kernel/irq.c void do_cpu_irq_mask(struct pt_regs *regs) · line 533
Quoted 3 lines
arch/parisc/kernel/irq.c +1 -1
-	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...

Quoted 5 lines

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

John David Anglin

2013-10-10 15:05 UTC #3 lore raw

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

Quoted 52 lines · Helge Deller

Hi James,
Quoted 11 lines · James Bottomley
Quoted 10 lines · Helge Deller
Signed-off-by Helge Deller <[email protected]>
arch/parisc/kernel/irq.c +0 -0
index 2e6443b..c439c05 100644
@@ -529,8 +529,8 @@ void do_cpu_irq_mask(struct pt_regs *regs)
  	cpumask_t dest;
  #endif
 
Quoted 5 lines
arch/parisc/kernel/irq.c void do_cpu_irq_mask(struct pt_regs *regs) · line 533
Quoted 3 lines
arch/parisc/kernel/irq.c +1 -1
-	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...
Quoted 5 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

John David Anglin    [email protected]

James Bottomley

maintainer

2013-10-11 11:54 UTC #4 lore raw

In reply to Helge Deller

Quoted 23 lines · Helge Deller

Hi James,
Quoted 11 lines · James Bottomley
Quoted 10 lines · Helge Deller
Signed-off-by Helge Deller <[email protected]>
arch/parisc/kernel/irq.c +0 -0
index 2e6443b..c439c05 100644
@@ -529,8 +529,8 @@ void do_cpu_irq_mask(struct pt_regs *regs)
 	cpumask_t dest;
 #endif
 
Quoted 6 lines
arch/parisc/kernel/irq.c void do_cpu_irq_mask(struct pt_regs *regs) · line 533
Quoted 3 lines
arch/parisc/kernel/irq.c +1 -1
-	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...

Quoted 7 lines

Quoted 5 lines
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.

Quoted 11 lines

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.

Quoted 1 line

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.

Quoted 4 lines

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