[PATCH] parisc: call set_irq_regs() after disabling local irqs

[PATCH] parisc: call set_irq_regs() after disabling local irqs

[Helge Deller]

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);
 	irq_enter();
 
 	eirr_val = mfctl(23) & cpu_eiem & per_cpu(local_ack_eiem, cpu);

Re: [PATCH] parisc: call set_irq_regs() after disabling local irqs

[James Bottomley]

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

Re: [PATCH] parisc: call set_irq_regs() after disabling local irqs

[Helge Deller]

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

Re: [PATCH] parisc: call set_irq_regs() after disabling local irqs

[John David Anglin]

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
>
>


-- 
John David Anglin    dave.anglin@bell.net

Re: [PATCH] parisc: call set_irq_regs() after disabling local irqs

[James Bottomley]

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