dangerous for initialize of MIPS timer

Hi! tsutsui-san,


From: Izumi Tsutsui <tsutsuiceres.dti.ne.jp>
Date: Sun, 24 Sep 2006 09:52:10 +0900

> kiyoharakk.iij4u.or.jp wrote:
> 
> > I think serious dangerous for initialization of
MIPS timer.  My Cobalt
> > Qube panic() in hardclock().  It reason for
hardclock() is called before
> > softclock is initialized. 
> 
> Yes, it could happen on my RaQ2 while it would be some
timing dependent.
> 
> Maybe we should move _splnone() from
autoconf.c:cpu_configure() to
> mips/mips3_clockintr():cpu_initclocks(), but I wonder
> we should still enable other interrupts than INT5 in
> cpu_configure() or not. (since initclocks() is called
> right after cpu_configure() in subr_autoconf.c).

All right.  Please check. 

For which port do you need it?

Thanks,
--
kiyohara

Index: arch/cobalt/cobalt/autoconf.c
============================================================
=======
RCS file: /cvsroot/src/sys/arch/cobalt/cobalt/autoconf.c,v
retrieving revision 1.23
diff -u -r1.23 autoconf.c
--- arch/cobalt/cobalt/autoconf.c	10 Sep 2006 06:41:09
-0000	1.23
+++ arch/cobalt/cobalt/autoconf.c	26 Sep 2006 17:28:33 -0000
 -55,8
+55,11 
 	if (config_rootfound("mainbus", NULL) == NULL)
 		panic("no mainbus found");
 
-	/* enable all interrupts */
-	_splnone();
+	/*
+	 * Enable interrupts excluding clock, because initclock()
not call
+	 * yet.
+	 */
+	_splnoclock();
 }
 
 void
Index: arch/cobalt/include/intr.h
============================================================
=======
RCS file: /cvsroot/src/sys/arch/cobalt/include/intr.h,v
retrieving revision 1.23
diff -u -r1.23 intr.h
--- arch/cobalt/include/intr.h	7 Sep 2006 03:38:55
-0000	1.23
+++ arch/cobalt/include/intr.h	26 Sep 2006 17:28:33 -0000
 -72,6
+72,7 
 void _splnone(void);
 void _setsoftintr(int);
 void _clrsoftintr(int);
+void _splnoclock(void);
 
 #define splhigh()       _splraise(MIPS_INT_MASK)
 #define spl0()          (void)_spllower(0)
Index: arch/mips/mips/locore.S
============================================================
=======
RCS file: /cvsroot/src/sys/arch/mips/mips/locore.S,v
retrieving revision 1.162
diff -u -r1.162 locore.S
--- arch/mips/mips/locore.S	7 Sep 2006 02:40:31 -0000	1.162
+++ arch/mips/mips/locore.S	26 Sep 2006 17:28:46 -0000
 -756,6
+756,18 
 	nop
 END(_splnone)
 
+LEAF(_splnoclock)
+	mtc0	zero, MIPS_COP_0_CAUSE		# clear SOFT_INT bits
+	COP0_SYNC
+	li	v0, ((MIPS_INT_MASK & ~MIPS_INT_MASK_5)|
MIPS_SR_INT_IE)
+	DYNAMIC_STATUS_MASK(v0,t0)		# machine dependent masking
+	mtc0	v0, MIPS_COP_0_STATUS		# enable all sources
+	COP0_SYNC
+	nop
+	j	ra
+	nop
+END(_splnoclock)
+
 /*
  * u_int32_t mips_cp0_cause_read(void)
  *
Index: arch/mips/mips/mips3_clockintr.c
============================================================
=======
RCS file:
/cvsroot/src/sys/arch/mips/mips/mips3_clockintr.c,v
retrieving revision 1.2
diff -u -r1.2 mips3_clockintr.c
--- arch/mips/mips/mips3_clockintr.c	10 Sep 2006 14:27:38
-0000	1.2
+++ arch/mips/mips/mips3_clockintr.c	26 Sep 2006 17:28:46
-0000
 -144,6
+144,9 
 #ifdef	__HAVE_TIMECOUNTER
 	mips3_init_tc();
 #endif
+
+	/* enable all interrupts */
+	_splnone();
 }
 
 /*

kiyoharakk.iij4u.or.jp wrote:

> > Maybe we should move _splnone() from
autoconf.c:cpu_configure() to
> > mips/mips3_clockintr():cpu_initclocks(), but I
wonder
> > we should still enable other interrupts than INT5
in
> > cpu_configure() or not. (since initclocks() is
called
> > right after cpu_configure() in subr_autoconf.c).
> 
> All right.  Please check. 

IMO "_spllower(MIPS_INT_MASK_5)" is enough like
arc/autoconf.c does
for optional statclock(9), and some people also said that we
should
have some comments why we needed such weird spl(9) handling
in both
cpu_configure(9) and cpu_initclocks(9).

> For which port do you need it?

According to "grep mips3_clockintr.c
/sys/arch/*/conf/files.*"
algor, cobalt, evbmips, and sgimips.
---
Izumi Tsutsui

Hi! tsutsui-san


From: Izumi Tsutsui <tsutsuiceres.dti.ne.jp>
Date: Wed, 27 Sep 2006 07:37:00 +0900

> > > Maybe we should move _splnone() from
autoconf.c:cpu_configure() to
> > > mips/mips3_clockintr():cpu_initclocks(), but
I wonder
> > > we should still enable other interrupts than
INT5 in
> > > cpu_configure() or not. (since initclocks()
is called
> > > right after cpu_configure() in
subr_autoconf.c).

> IMO "_spllower(MIPS_INT_MASK_5)" is enough
like arc/autoconf.c does
> for optional statclock(9), and some people also said
that we should
> have some comments why we needed such weird spl(9)
handling in both
> cpu_configure(9) and cpu_initclocks(9).

I agree.  However the bit MIPS_SR_INT_IE was already set?
I think that here is good because of it. 

  #define _splnoclock()	  _spllower(MIPS_INT_MASK_5 |
MIPS_SR_INT_IE)


Thanks,
--
kiyohara

kiyoharakk.iij4u.or.jp wrote:

> > IMO "_spllower(MIPS_INT_MASK_5)" is
enough like arc/autoconf.c does
> > for optional statclock(9), and some people also
said that we should
> > have some comments why we needed such weird spl(9)
handling in both
> > cpu_configure(9) and cpu_initclocks(9).
> 
> I agree.  However the bit MIPS_SR_INT_IE was already
set?

Ah, you are right..
(on the other hand it means we don't have to enable
interrupts there? 

> I think that here is good because of it. 
> 
>   #define _splnoclock()	  _spllower(MIPS_INT_MASK_5 |
MIPS_SR_INT_IE)

IMHO, this usage doesn't match "system priority
level" definitions
(i.e. splfoo() implies something to "raise" the
system priority level)
so it might be better to have some other name, like
enable_devintr()
or enable_extintr() etc? (in <mips/mips3_clock.h>?)
---
Izumi Tsutsui

Hi! tsutsui-san,


From: Izumi Tsutsui <tsutsuiceres.dti.ne.jp>
Date: Wed, 27 Sep 2006 23:26:29 +0900

> > I think that here is good because of it. 
> > 
> >   #define _splnoclock()	 
_spllower(MIPS_INT_MASK_5 | MIPS_SR_INT_IE)
> 
> IMHO, this usage doesn't match "system priority
level" definitions
> (i.e. splfoo() implies something to "raise"
the system priority level)
> so it might be better to have some other name, like
enable_devintr()
> or enable_extintr() etc? (in
<mips/mips3_clock.h>?)

It cannot be thought that the name such as devintr and
extintr is
appropriateness to me. 
In this case, I think that more directly name 'noclock' is
the best. 

Thanks,
--
kiyohara

Index: arch/algor/algor/autoconf.c
============================================================
=======
RCS file: /cvsroot/src/sys/arch/algor/algor/autoconf.c,v
retrieving revision 1.14
diff -u -r1.14 autoconf.c
--- arch/algor/algor/autoconf.c	5 May 2006 18:04:41
-0000	1.14
+++ arch/algor/algor/autoconf.c	28 Sep 2006 15:51:18 -0000
 -58,6
+58,8 
 #include <machine/autoconf.h>
 #include <machine/intr.h>
 
+#include <mips/mips3_clock.h>
+
 #ifdef ALGOR_P4032
 #include <algor/algor/algor_p4032var.h>
 #endif
 -71,7
+73,11 
 	(void) splhigh();
 	if (config_rootfound("mainbus", NULL) == 0)
 		panic("no mainbus found");
-	(void) spl0();
+	/*
+	 * Enable interrupts excluding clock, because initclock()
not call
+	 * yet.
+	 */
+	enable_intr_noclock();
 }
 
 void
Index: arch/cobalt/cobalt/autoconf.c
============================================================
=======
RCS file: /cvsroot/src/sys/arch/cobalt/cobalt/autoconf.c,v
retrieving revision 1.23
diff -u -r1.23 autoconf.c
--- arch/cobalt/cobalt/autoconf.c	10 Sep 2006 06:41:09
-0000	1.23
+++ arch/cobalt/cobalt/autoconf.c	28 Sep 2006 15:51:21 -0000
 -37,6
+37,8 
 #include <machine/cpu.h>
 #include <machine/intr.h>
 
+#include <mips/mips3_clock.h>
+
 extern char	bootstring[];
 extern int	netboot;
 extern int	bootunit;
 -55,8
+57,11 
 	if (config_rootfound("mainbus", NULL) == NULL)
 		panic("no mainbus found");
 
-	/* enable all interrupts */
-	_splnone();
+	/*
+	 * Enable interrupts excluding clock, because initclock()
not call
+	 * yet.
+	 */
+	enable_intr_noclock();
 }
 
 void
Index: arch/evbmips/alchemy/autoconf.c
============================================================
=======
RCS file: /cvsroot/src/sys/arch/evbmips/alchemy/autoconf.c,v
retrieving revision 1.12
diff -u -r1.12 autoconf.c
--- arch/evbmips/alchemy/autoconf.c	5 May 2006 18:04:41
-0000	1.12
+++ arch/evbmips/alchemy/autoconf.c	28 Sep 2006 15:51:22
-0000
 -53,6
+53,7 
 #include <mips/alchemy/include/aureg.h>
 #include <mips/alchemy/include/auvar.h>
 #include <mips/alchemy/include/aubusvar.h>
+#include <mips/mips3_clock.h>
 
 /*
  * Configure all devices on system
 -67,7
+68,11 
 	(void)splhigh();
 	if (config_rootfound("mainbus", NULL) == NULL)
 		panic("no mainbus found");
-	(void)spl0();
+	/*
+	 * Enable interrupts excluding clock, because initclock()
not call
+	 * yet.
+	 */
+	enable_intr_noclock();
 }
 
 void
Index: arch/evbmips/atheros/autoconf.c
============================================================
=======
RCS file: /cvsroot/src/sys/arch/evbmips/atheros/autoconf.c,v
retrieving revision 1.6
diff -u -r1.6 autoconf.c
--- arch/evbmips/atheros/autoconf.c	4 Sep 2006 05:17:26
-0000	1.6
+++ arch/evbmips/atheros/autoconf.c	28 Sep 2006 15:51:22
-0000
 -47,6
+47,7 
 
 #include <mips/atheros/include/ar531xvar.h>
 #include <mips/atheros/include/arbusvar.h>
+#include <mips/mips3_clock.h>
 
 /*
  * Configure all devices on system
 -61,7
+62,11 
 	(void)splhigh();
 	if (config_rootfound("mainbus", NULL) == NULL)
 		panic("no mainbus found");
-	(void)spl0();
+	/*
+	 * Enable interrupts excluding clock, because initclock()
not call
+	 * yet.
+	 */
+	enable_intr_noclock();
 }
 
 void
Index: arch/evbmips/malta/autoconf.c
============================================================
=======
RCS file: /cvsroot/src/sys/arch/evbmips/malta/autoconf.c,v
retrieving revision 1.9
diff -u -r1.9 autoconf.c
--- arch/evbmips/malta/autoconf.c	26 Feb 2006 05:24:52
-0000	1.9
+++ arch/evbmips/malta/autoconf.c	28 Sep 2006 15:51:23 -0000
 -46,6
+46,8 
 
 #include <machine/cpu.h>
 
+#include <mips/mips3_clock.h>
+
 static void	findroot(void);
 
 void
 -58,7
+60,11 
 	(void)splhigh();
 	if (config_rootfound("mainbus", NULL) == NULL)
 		panic("no mainbus found");
-	(void)spl0();
+	/*
+	 * Enable interrupts excluding clock, because initclock()
not call
+	 * yet.
+	 */
+	enable_intr_noclock();
 }
 
 void
Index: arch/mips/include/mips3_clock.h
============================================================
=======
RCS file: /cvsroot/src/sys/arch/mips/include/mips3_clock.h,v
retrieving revision 1.4
diff -u -r1.4 mips3_clock.h
--- arch/mips/include/mips3_clock.h	10 Sep 2006 14:27:38
-0000	1.4
+++ arch/mips/include/mips3_clock.h	28 Sep 2006 15:51:31
-0000
 -43,4
+43,6 
 void	mips3_init_tc(void);
 #endif
 
+#define	enable_intr_noclock()	_spllower(MIPS_INT_MASK_5 |
MIPS_SR_INT_IE)
+
 #endif	/* _MIPS3_CLOCK_H */
Index: arch/mips/mips/mips3_clockintr.c
============================================================
=======
RCS file:
/cvsroot/src/sys/arch/mips/mips/mips3_clockintr.c,v
retrieving revision 1.2
diff -u -r1.2 mips3_clockintr.c
--- arch/mips/mips/mips3_clockintr.c	10 Sep 2006 14:27:38
-0000	1.2
+++ arch/mips/mips/mips3_clockintr.c	28 Sep 2006 15:51:31
-0000
 -144,6
+144,9 
 #ifdef	__HAVE_TIMECOUNTER
 	mips3_init_tc();
 #endif
+
+	/* enable all interrupts */
+	_splnone();
 }
 
 /*
Index: arch/sgimips/sgimips/autoconf.c
============================================================
=======
RCS file: /cvsroot/src/sys/arch/sgimips/sgimips/autoconf.c,v
retrieving revision 1.31
diff -u -r1.31 autoconf.c
--- arch/sgimips/sgimips/autoconf.c	10 Jul 2006 16:28:44
-0000	1.31
+++ arch/sgimips/sgimips/autoconf.c	28 Sep 2006 15:51:35
-0000
 -48,6
+48,8 
 #include <machine/machtype.h>
 #include <machine/autoconf.h>
 
+#include <mips/mips3_clock.h>
+
 #include <dev/pci/pcivar.h>
 
 #include <dev/scsipi/scsi_all.h>
 -81,7
+83,11 
 	    splmasks[IPL_BIO] >> 8, splmasks[IPL_NET]
>> 8, 
 	    splmasks[IPL_TTY] >> 8, splmasks[IPL_CLOCK]
>> 8);
 
-	_splnone();
+	/*
+	 * Enable interrupts excluding clock, because initclock()
not call
+	 * yet.
+	 */
+	enable_intr_noclock();
 }
 
 /*

kiyoharakk.iij4u.or.jp wrote:

> > >   #define _splnoclock()	 
_spllower(MIPS_INT_MASK_5 | MIPS_SR_INT_IE)
> > 
> > IMHO, this usage doesn't match "system
priority level" definitions
> > (i.e. splfoo() implies something to
"raise" the system priority level)
> > so it might be better to have some other name,
like enable_devintr()
> > or enable_extintr() etc? (in
<mips/mips3_clock.h>?)
> 
> It cannot be thought that the name such as devintr and
extintr is
> appropriateness to me. 
> In this case, I think that more directly name 'noclock'
is the best. 

IMO `enable_intr_noclock()' also looks ambiguous, but we
should ask
other native mips guys...

Another possible option is just removing spl calls from MD
cpu_configure(9)
so that we don't have to choose any names?
---
Izumi Tsutsui

Hi! tsutsui-san,


From: Izumi Tsutsui <tsutsuiceres.dti.ne.jp>
Date: Sat, 30 Sep 2006 09:49:12 +0900

> kiyoharakk.iij4u.or.jp wrote:
> 
> > > >   #define _splnoclock()	 
_spllower(MIPS_INT_MASK_5 | MIPS_SR_INT_IE)
> > > 
> > > IMHO, this usage doesn't match "system
priority level" definitions
> > > (i.e. splfoo() implies something to
"raise" the system priority level)
> > > so it might be better to have some other
name, like enable_devintr()
> > > or enable_extintr() etc? (in
<mips/mips3_clock.h>?)
> > 
> > It cannot be thought that the name such as devintr
and extintr is
> > appropriateness to me. 
> > In this case, I think that more directly name
'noclock' is the best. 
> 
> IMO `enable_intr_noclock()' also looks ambiguous, but
we should ask
> other native mips guys...
> 
> Another possible option is just removing spl calls from
MD cpu_configure(9)
> so that we don't have to choose any names?

hmm...
How is this problem done?
There are none of reactions from mips guys... ;-<

Thanks,
--
kiyohara