Patchwork [10/10] ARM: FIQ: Get rid of init_FIQ()

login
register
mail settings
Submitter Anton Vorontsov
Date Nov. 23, 2012, 12:50 a.m.
Message ID <1353631803-4853-10-git-send-email-anton.vorontsov@linaro.org>
Download mbox | patch
Permalink /patch/201229/
State New
Headers show

Comments

Anton Vorontsov - Nov. 23, 2012, 12:50 a.m.
The function only saves initial arch-specific "no FIQ" instruction, by
placing the code into set_fiq_handler() we can:

- Have less code and logic in the platform-specific files;
- Have the code that manages FIQ vector overwriting in one place, i.e.
  don't spread the logic around.

p.s. Also, I noticed that we saved no_fiq_insn w/o bothering about
!CONFIG_CPU_USE_DOMAINS case, but set_fiq_handler() handles this case
specifically. I wonder, was that on purpose, e.g. it does not matter for
init_FIQ(), or it was just overlooked? In the latter case, the patch fixes
the issue.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 arch/arm/include/asm/mach/irq.h |  2 --
 arch/arm/kernel/fiq.c           | 17 ++++++++---------
 arch/arm/mach-rpc/irq.c         |  2 --
 arch/arm/plat-mxc/avic.c        |  3 ---
 arch/arm/plat-mxc/tzic.c        |  3 ---
 arch/arm/plat-s3c24xx/irq.c     |  2 --
 6 files changed, 8 insertions(+), 21 deletions(-)
Alexander Shiyan - Nov. 23, 2012, 3:40 a.m.
> The function only saves initial arch-specific "no FIQ" instruction, by
> placing the code into set_fiq_handler() we can:
> 
> - Have less code and logic in the platform-specific files;
> - Have the code that manages FIQ vector overwriting in one place, i.e.
>   don't spread the logic around.
> 
> p.s. Also, I noticed that we saved no_fiq_insn w/o bothering about
> !CONFIG_CPU_USE_DOMAINS case, but set_fiq_handler() handles this case
> specifically. I wonder, was that on purpose, e.g. it does not matter for
> init_FIQ(), or it was just overlooked? In the latter case, the patch fixes
> the issue.
...
> diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
> index 9bf3a60..3602df6 100644
> --- a/arch/arm/kernel/fiq.c
> +++ b/arch/arm/kernel/fiq.c
> @@ -49,6 +49,7 @@
>  #include <asm/mach/irq.h>
>  
>  static unsigned long no_fiq_insn;
> +static int got_no_fiq_insn;
>  
>  /* Default reacquire function
>   * - we always relinquish FIQ control
> @@ -78,11 +79,14 @@ void show_fiq_list(struct seq_file *p, int prec)
>  
>  void set_fiq_handler(void *start, unsigned int length)
>  {
> -#if defined(CONFIG_CPU_USE_DOMAINS)
> -	memcpy((void *)0xffff001c, start, length);
> -#else
> -	memcpy(vectors_page + 0x1c, start, length);
> +	unsigned long *addr = (void *)0xffff001c;
> +
> +#ifndef CONFIG_CPU_USE_DOMAINS
> +	addr = vectors_page + 0x1c;
>  #endif
> +	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
> +		no_fiq_insn = *addr;
> +	memcpy(addr, start, length);
>  	flush_icache_range(0xffff001c, 0xffff001c + length);
>  	if (!vectors_high())
>  		flush_icache_range(0x1c, 0x1c + length);
> @@ -126,8 +130,3 @@ EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
>  EXPORT_SYMBOL(__get_fiq_regs);	/* defined in fiqasm.S */
>  EXPORT_SYMBOL(claim_fiq);
>  EXPORT_SYMBOL(release_fiq);
> -
> -void __init init_FIQ(void)
> -{
> -	no_fiq_insn = *(unsigned long *)0xffff001c;

it seems that this is wrong. In this case we have an uninitialized variable and
sequential call claim_fiq and release_fiq could be fatal. FIXME please.

---
Anton Vorontsov - Nov. 23, 2012, 5:53 a.m.
On Fri, Nov 23, 2012 at 07:40:30AM +0400, Alexander Shiyan wrote:
[...]
> >  static unsigned long no_fiq_insn;
> > +static int got_no_fiq_insn;
> > @@ -78,11 +79,14 @@ void show_fiq_list(struct seq_file *p, int prec)
> >  
> >  void set_fiq_handler(void *start, unsigned int length)
> >  {
> > -#if defined(CONFIG_CPU_USE_DOMAINS)
> > -	memcpy((void *)0xffff001c, start, length);
> > -#else
> > -	memcpy(vectors_page + 0x1c, start, length);
> > +	unsigned long *addr = (void *)0xffff001c;
> > +
> > +#ifndef CONFIG_CPU_USE_DOMAINS
> > +	addr = vectors_page + 0x1c;
> >  #endif
> > +	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
> > +		no_fiq_insn = *addr;
> > +	memcpy(addr, start, length);
> >  	flush_icache_range(0xffff001c, 0xffff001c + length);
> >  	if (!vectors_high())
> >  		flush_icache_range(0x1c, 0x1c + length);
> > @@ -126,8 +130,3 @@ EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
> > -
> > -void __init init_FIQ(void)
> > -{
> > -	no_fiq_insn = *(unsigned long *)0xffff001c;
> 
> it seems that this is wrong. In this case we have an uninitialized variable and
> sequential call claim_fiq and release_fiq could be fatal. FIXME please.

Um... I don't think I understand, can you please elaborate?

Thanks,
Anton.
Alexander Shiyan - Nov. 23, 2012, 6:27 a.m.
> On Fri, Nov 23, 2012 at 07:40:30AM +0400, Alexander Shiyan wrote:
> [...]
> > >  static unsigned long no_fiq_insn;
> > > +static int got_no_fiq_insn;
> > > @@ -78,11 +79,14 @@ void show_fiq_list(struct seq_file *p, int prec)
> > >  
> > >  void set_fiq_handler(void *start, unsigned int length)
> > >  {
> > > -#if defined(CONFIG_CPU_USE_DOMAINS)
> > > -	memcpy((void *)0xffff001c, start, length);
> > > -#else
> > > -	memcpy(vectors_page + 0x1c, start, length);
> > > +	unsigned long *addr = (void *)0xffff001c;
> > > +
> > > +#ifndef CONFIG_CPU_USE_DOMAINS
> > > +	addr = vectors_page + 0x1c;
> > >  #endif
> > > +	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
> > > +		no_fiq_insn = *addr;
> > > +	memcpy(addr, start, length);
> > >  	flush_icache_range(0xffff001c, 0xffff001c + length);
> > >  	if (!vectors_high())
> > >  		flush_icache_range(0x1c, 0x1c + length);
> > > @@ -126,8 +130,3 @@ EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
> > > -
> > > -void __init init_FIQ(void)
> > > -{
> > > -	no_fiq_insn = *(unsigned long *)0xffff001c;
> > 
> > it seems that this is wrong. In this case we have an uninitialized variable and
> > sequential call claim_fiq and release_fiq could be fatal. FIXME please.
> 
> Um... I don't think I understand, can you please elaborate?

OK, I'll try to explain it.
At the end of the release_fiq function we have a call fiq_op. For the default
handler - is a fiq_def_op function, and we call this function with the option
"relinquish = 0", i.e. we want to restore old fiq_handler. But if we do not call
set_fiq_handler never before, we will have an uninitialized no_fiq_insn variable.

---
Anton Vorontsov - Nov. 23, 2012, 6:50 a.m.
On Fri, Nov 23, 2012 at 10:27:51AM +0400, Alexander Shiyan wrote:
> > On Fri, Nov 23, 2012 at 07:40:30AM +0400, Alexander Shiyan wrote:
> > [...]
> > > >  static unsigned long no_fiq_insn;
> > > > +static int got_no_fiq_insn;
> > > > @@ -78,11 +79,14 @@ void show_fiq_list(struct seq_file *p, int prec)
> > > >  
> > > >  void set_fiq_handler(void *start, unsigned int length)
> > > >  {
> > > > -#if defined(CONFIG_CPU_USE_DOMAINS)
> > > > -	memcpy((void *)0xffff001c, start, length);
> > > > -#else
> > > > -	memcpy(vectors_page + 0x1c, start, length);
> > > > +	unsigned long *addr = (void *)0xffff001c;
> > > > +
> > > > +#ifndef CONFIG_CPU_USE_DOMAINS
> > > > +	addr = vectors_page + 0x1c;
> > > >  #endif
> > > > +	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
> > > > +		no_fiq_insn = *addr;
> > > > +	memcpy(addr, start, length);
> > > >  	flush_icache_range(0xffff001c, 0xffff001c + length);
> > > >  	if (!vectors_high())
> > > >  		flush_icache_range(0x1c, 0x1c + length);
> > > > @@ -126,8 +130,3 @@ EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
> > > > -
> > > > -void __init init_FIQ(void)
> > > > -{
> > > > -	no_fiq_insn = *(unsigned long *)0xffff001c;
> > > 
> > > it seems that this is wrong. In this case we have an uninitialized variable and
> > > sequential call claim_fiq and release_fiq could be fatal. FIXME please.
> > 
> > Um... I don't think I understand, can you please elaborate?
> 
> OK, I'll try to explain it.
> At the end of the release_fiq function we have a call fiq_op. For the default
> handler - is a fiq_def_op function, and we call this function with the option
> "relinquish = 0", i.e. we want to restore old fiq_handler. But if we do not call
> set_fiq_handler never before, we will have an uninitialized no_fiq_insn variable.

It should not matter when or in what order anyone calls the
set_fiq_handler(), since it stores "no FIQ instruction" into no_fiq_insn
at its first invocation:

	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
		no_fiq_insn = *addr;

If we never called set_fiq_handler() before, during release_fiq() we'll:

1. Copy the initial instruction from the vector page to 'no_fiq_insn';
2. Copy the initial instruction from 'no_fiq_insn' to the vector page;

So no_fiq_insn gets initialized, then we just instantly write the same
value back.

Thanks,
Anton.
Alexander Shiyan - Nov. 23, 2012, 7:36 a.m.
> On Fri, Nov 23, 2012 at 10:27:51AM +0400, Alexander Shiyan wrote:
> > > On Fri, Nov 23, 2012 at 07:40:30AM +0400, Alexander Shiyan wrote:
> > > [...]
> > > > >  static unsigned long no_fiq_insn;
> > > > > +static int got_no_fiq_insn;
> > > > > @@ -78,11 +79,14 @@ void show_fiq_list(struct seq_file *p, int prec)
> > > > >  
> > > > >  void set_fiq_handler(void *start, unsigned int length)
> > > > >  {
> > > > > -#if defined(CONFIG_CPU_USE_DOMAINS)
> > > > > -	memcpy((void *)0xffff001c, start, length);
> > > > > -#else
> > > > > -	memcpy(vectors_page + 0x1c, start, length);
> > > > > +	unsigned long *addr = (void *)0xffff001c;
> > > > > +
> > > > > +#ifndef CONFIG_CPU_USE_DOMAINS
> > > > > +	addr = vectors_page + 0x1c;
> > > > >  #endif
> > > > > +	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
> > > > > +		no_fiq_insn = *addr;
> > > > > +	memcpy(addr, start, length);
> > > > >  	flush_icache_range(0xffff001c, 0xffff001c + length);
> > > > >  	if (!vectors_high())
> > > > >  		flush_icache_range(0x1c, 0x1c + length);
> > > > > @@ -126,8 +130,3 @@ EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
> > > > > -
> > > > > -void __init init_FIQ(void)
> > > > > -{
> > > > > -	no_fiq_insn = *(unsigned long *)0xffff001c;
> > > > 
> > > > it seems that this is wrong. In this case we have an uninitialized variable and
> > > > sequential call claim_fiq and release_fiq could be fatal. FIXME please.
> > > 
> > > Um... I don't think I understand, can you please elaborate?
> > 
> > OK, I'll try to explain it.
> > At the end of the release_fiq function we have a call fiq_op. For the default
> > handler - is a fiq_def_op function, and we call this function with the option
> > "relinquish = 0", i.e. we want to restore old fiq_handler. But if we do not call
> > set_fiq_handler never before, we will have an uninitialized no_fiq_insn variable.
> 
> It should not matter when or in what order anyone calls the
> set_fiq_handler(), since it stores "no FIQ instruction" into no_fiq_insn
> at its first invocation:
> 
> 	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
> 		no_fiq_insn = *addr;
> 
> If we never called set_fiq_handler() before, during release_fiq() we'll:
> 
> 1. Copy the initial instruction from the vector page to 'no_fiq_insn';
> 2. Copy the initial instruction from 'no_fiq_insn' to the vector page;
> 
> So no_fiq_insn gets initialized, then we just instantly write the same
> value back.

got_no_fiq_insn also not initialized.
I think as a whole on this issue requires additional comments from Russell
as the author of implementation.

PS: In any case, I can reproduce it and check it out if you send me a patchset
to my email as an attachment.


---
Anton Vorontsov - Nov. 23, 2012, 7:51 a.m.
On Fri, Nov 23, 2012 at 11:36:01AM +0400, Alexander Shiyan wrote:
[...]
> got_no_fiq_insn also not initialized.

It is static, so by definition it is initialized to 0.

> I think as a whole on this issue requires additional comments from Russell
> as the author of implementation.
> 
> PS: In any case, I can reproduce it and check it out if you send me a patchset
> to my email as an attachment.

Sent privately.

Thanks,
Anton.
Anton Vorontsov - Nov. 27, 2012, 9:05 a.m.
On Thu, Nov 22, 2012 at 11:51:38PM -0800, Anton Vorontsov wrote:
> On Fri, Nov 23, 2012 at 11:36:01AM +0400, Alexander Shiyan wrote:
> [...]
> > got_no_fiq_insn also not initialized.
> 
> It is static, so by definition it is initialized to 0.
> 
> > I think as a whole on this issue requires additional comments from Russell
> > as the author of implementation.
> > 
> > PS: In any case, I can reproduce it and check it out if you send me a patchset
> > to my email as an attachment.
> 
> Sent privately.

Andrew, Arnd, just FYI, it works for Alexander.

----- Forwarded message from Alexander Shiyan <shc_work@mail.ru> -----
Date: Tue, 27 Nov 2012 12:42:50 +0400
From: Alexander Shiyan <shc_work@mail.ru>
To: Anton Vorontsov <anton.vorontsov@linaro.org>
Subject: Re[2]: FIQ

> Hello Alexander,
> 
> On Tue, Nov 27, 2012 at 12:14:28PM +0400, Alexander Shiyan wrote:
> > Sorry, I lost the original message to the correct answer.
> > Today I had a test of removal init_FIQ.
> > The test took place on a custom board with a custom ARM CLPS711X OSS audio
> > driver, which uses the FIQ interrupts. Everything works fine.
> 
> Nice! Thanks a lot for testing!
> 
> Can you please reply to the original thread, with the
> 
> 	Tested-by: Alexander Shiyan <shc_work@mail.ru>
> ?
> 
> This will help pushing the series forward, plus will give you a credit for
> your efforts.

I am lost thread in my mailbox, you can just copy/paste my reply and ping for Arnd.

Patch

diff --git a/arch/arm/include/asm/mach/irq.h b/arch/arm/include/asm/mach/irq.h
index 8be5ba9..6e70ae3 100644
--- a/arch/arm/include/asm/mach/irq.h
+++ b/arch/arm/include/asm/mach/irq.h
@@ -18,10 +18,8 @@  struct seq_file;
  * This is internal.  Do not use it.
  */
 #ifdef CONFIG_FIQ
-extern void init_FIQ(void);
 extern void show_fiq_list(struct seq_file *, int);
 #else
-static inline void init_FIQ(void) {}
 static inline void show_fiq_list(struct seq_file *p, int prec) {}
 #endif
 
diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
index 9bf3a60..3602df6 100644
--- a/arch/arm/kernel/fiq.c
+++ b/arch/arm/kernel/fiq.c
@@ -49,6 +49,7 @@ 
 #include <asm/mach/irq.h>
 
 static unsigned long no_fiq_insn;
+static int got_no_fiq_insn;
 
 /* Default reacquire function
  * - we always relinquish FIQ control
@@ -78,11 +79,14 @@  void show_fiq_list(struct seq_file *p, int prec)
 
 void set_fiq_handler(void *start, unsigned int length)
 {
-#if defined(CONFIG_CPU_USE_DOMAINS)
-	memcpy((void *)0xffff001c, start, length);
-#else
-	memcpy(vectors_page + 0x1c, start, length);
+	unsigned long *addr = (void *)0xffff001c;
+
+#ifndef CONFIG_CPU_USE_DOMAINS
+	addr = vectors_page + 0x1c;
 #endif
+	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
+		no_fiq_insn = *addr;
+	memcpy(addr, start, length);
 	flush_icache_range(0xffff001c, 0xffff001c + length);
 	if (!vectors_high())
 		flush_icache_range(0x1c, 0x1c + length);
@@ -126,8 +130,3 @@  EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(__get_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(claim_fiq);
 EXPORT_SYMBOL(release_fiq);
-
-void __init init_FIQ(void)
-{
-	no_fiq_insn = *(unsigned long *)0xffff001c;
-}
diff --git a/arch/arm/mach-rpc/irq.c b/arch/arm/mach-rpc/irq.c
index 07770c8..2d915ba 100644
--- a/arch/arm/mach-rpc/irq.c
+++ b/arch/arm/mach-rpc/irq.c
@@ -151,7 +151,5 @@  void __init rpc_init_irq(void)
 			break;
 		}
 	}
-
-	init_FIQ();
 }
 
diff --git a/arch/arm/plat-mxc/avic.c b/arch/arm/plat-mxc/avic.c
index 426980c..b173dc6 100644
--- a/arch/arm/plat-mxc/avic.c
+++ b/arch/arm/plat-mxc/avic.c
@@ -216,8 +216,5 @@  void __init mxc_init_irq(void __iomem *irqbase)
 	for (i = 0; i < 8; i++)
 		__raw_writel(0, avic_base + AVIC_NIPRIORITY(i));
 
-	/* Initialize FIQ */
-	init_FIQ();
-
 	printk(KERN_INFO "MXC IRQ initialized\n");
 }
diff --git a/arch/arm/plat-mxc/tzic.c b/arch/arm/plat-mxc/tzic.c
index 8a5a633..889267c 100644
--- a/arch/arm/plat-mxc/tzic.c
+++ b/arch/arm/plat-mxc/tzic.c
@@ -191,9 +191,6 @@  void __init tzic_init_irq(void __iomem *irqbase)
 	for (i = 0; i < 4; i++, irq_base += 32)
 		tzic_init_gc(i, irq_base);
 
-	/* Initialize FIQ */
-	init_FIQ();
-
 	pr_info("TrustZone Interrupt Controller (TZIC) initialized\n");
 }
 
diff --git a/arch/arm/plat-s3c24xx/irq.c b/arch/arm/plat-s3c24xx/irq.c
index 531d6a4..66b8856 100644
--- a/arch/arm/plat-s3c24xx/irq.c
+++ b/arch/arm/plat-s3c24xx/irq.c
@@ -532,8 +532,6 @@  void __init s3c24xx_init_irq(void)
 	int irqno;
 	int i;
 
-	init_FIQ();
-
 	irqdbf("s3c2410_init_irq: clearing interrupt status flags\n");
 
 	/* first, clear all interrupts pending... */