Quick skim looks good.
On Fri, Jun 19, 2015 at 5:02 PM, Sebastian Huber
<sebastian.hu...@embedded-brains.de> wrote:
> Add rtems_interrupt_local_disable|enable() as suggested by Pavel Pisa to
> emphasize that interrupts are only disabled on the current processor.
> Do not define the rtems_interrupt_disable|enable|flash() macros and
> functions on SMP configurations since they don't ensure system wide
> mutual exclusion.
> ---
> c/src/lib/libbsp/shared/bootcard.c | 2 +-
> cpukit/rtems/include/rtems/rtems/intr.h | 38 ++++++++
> cpukit/rtems/src/intrbody.c | 4 +
> doc/user/intr.t | 94 ++++++++++++++++++
> testsuites/libtests/heapwalk/init.c | 4 +-
> testsuites/samples/Makefile.am | 3 +
> testsuites/samples/configure.ac | 2 +
> testsuites/sptests/sp37/init.c | 164
> ++++++++++++++++++--------------
> testsuites/sptests/sp40/init.c | 6 +-
> testsuites/tmtests/tm26/task1.c | 9 +-
> 10 files changed, 244 insertions(+), 82 deletions(-)
>
> diff --git a/c/src/lib/libbsp/shared/bootcard.c
> b/c/src/lib/libbsp/shared/bootcard.c
> index 4f91aa9..0ee5e12 100644
> --- a/c/src/lib/libbsp/shared/bootcard.c
> +++ b/c/src/lib/libbsp/shared/bootcard.c
> @@ -72,7 +72,7 @@ void boot_card(
> * Make sure interrupts are disabled.
> */
> (void) bsp_isr_level;
> - rtems_interrupt_disable( bsp_isr_level );
> + rtems_interrupt_local_disable( bsp_isr_level );
>
> bsp_boot_cmdline = cmdline;
>
> diff --git a/cpukit/rtems/include/rtems/rtems/intr.h
> b/cpukit/rtems/include/rtems/rtems/intr.h
> index 259120f..d084959 100644
> --- a/cpukit/rtems/include/rtems/rtems/intr.h
> +++ b/cpukit/rtems/include/rtems/rtems/intr.h
> @@ -89,10 +89,15 @@ rtems_status_code rtems_interrupt_catch(
> );
> #endif
>
> +#if !defined(RTEMS_SMP)
> +
> /**
> * @brief Disable RTEMS Interrupt
> *
> * @note The interrupt level shall be of type @ref rtems_interrupt_level.
> + *
> + * This macro is only available on uni-processor configurations. The macro
> + * rtems_interrupt_local_disable() is available on all configurations.
> */
> #define rtems_interrupt_disable( _isr_cookie ) \
> _ISR_Disable(_isr_cookie)
> @@ -101,6 +106,9 @@ rtems_status_code rtems_interrupt_catch(
> * @brief Enable RTEMS Interrupt
> *
> * @note The interrupt level shall be of type @ref rtems_interrupt_level.
> + *
> + * This macro is only available on uni-processor configurations. The macro
> + * rtems_interrupt_local_enable() is available on all configurations.
> */
> #define rtems_interrupt_enable( _isr_cookie ) \
> _ISR_Enable(_isr_cookie)
> @@ -109,10 +117,40 @@ rtems_status_code rtems_interrupt_catch(
> * @brief Flash RTEMS Interrupt
> *
> * @note The interrupt level shall be of type @ref rtems_interrupt_level.
> + *
> + * This macro is only available on uni-processor configurations. The macro
> + * rtems_interrupt_local_disable() and rtems_interrupt_local_enable() is
> + * available on all configurations.
> */
> #define rtems_interrupt_flash( _isr_cookie ) \
> _ISR_Flash(_isr_cookie)
>
> +#endif /* RTEMS_SMP */
> +
> +/**
> + * @brief This macro disables the interrupts on the current processor.
> + *
> + * On SMP configurations this will not ensure system wide mutual exclusion.
> + * Use interrupt locks instead.
> + *
> + * @param[in] _isr_cookie The previous interrupt level is returned. The type
> + * of this variable must be rtems_interrupt_level.
> + *
> + * @see rtems_interrupt_local_enable().
> + */
> +#define rtems_interrupt_local_disable( _isr_cookie ) \
> + _ISR_Disable_without_giant( _isr_cookie )
> +
> +/**
> + * @brief This macro restores the previous interrupt level on the current
> + * processor.
> + *
> + * @param[in] _isr_cookie The previous interrupt level returned by
> + * rtems_interrupt_local_disable().
> + */
> +#define rtems_interrupt_local_enable( _isr_cookie ) \
> + _ISR_Enable_without_giant( _isr_cookie )
> +
> /**
> * @brief RTEMS Interrupt Is in Progress
> *
> diff --git a/cpukit/rtems/src/intrbody.c b/cpukit/rtems/src/intrbody.c
> index 6b37eb2..deb0dc0 100644
> --- a/cpukit/rtems/src/intrbody.c
> +++ b/cpukit/rtems/src/intrbody.c
> @@ -23,6 +23,8 @@
> #include <rtems/score/isr.h>
> #include <rtems/rtems/intr.h>
>
> +#if !defined(RTEMS_SMP)
> +
> /*
> * Undefine all of these is normally a macro and we want a real body in
> * the library for other language bindings.
> @@ -70,3 +72,5 @@ bool rtems_interrupt_is_in_progress( void )
> {
> return _ISR_Is_in_progress();
> }
> +
> +#endif /* RTEMS_SMP */
> diff --git a/doc/user/intr.t b/doc/user/intr.t
> index 6cb6a26..cf106f3 100644
> --- a/doc/user/intr.t
> +++ b/doc/user/intr.t
> @@ -21,6 +21,8 @@ directive:
> @item @code{@value{DIRPREFIX}interrupt_disable} - Disable Interrupts
> @item @code{@value{DIRPREFIX}interrupt_enable} - Enable Interrupts
> @item @code{@value{DIRPREFIX}interrupt_flash} - Flash Interrupt
> +@item @code{@value{DIRPREFIX}interrupt_local_disable} - Disable Interrupts
> on Current Processor
> +@item @code{@value{DIRPREFIX}interrupt_local_enable} - Enable Interrupts on
> Current Processor
> @item @code{@value{DIRPREFIX}interrupt_lock_initialize} - Initialize an ISR
> Lock
> @item @code{@value{DIRPREFIX}interrupt_lock_acquire} - Acquire an ISR Lock
> @item @code{@value{DIRPREFIX}interrupt_lock_release} - Release an ISR Lock
> @@ -397,6 +399,10 @@ This directive will not cause the calling task to be
> preempted.
> parameter.}
> @end ifset
>
> +This directive is only available on uni-processor configurations. The
> +directive @code{@value{DIRPREFIX}interrupt_local_disable} is available on all
> +configurations.
> +
> @c
> @c
> @c
> @@ -441,6 +447,9 @@ and will be enabled when this directive returns to the
> caller.
>
> This directive will not cause the calling task to be preempted.
>
> +This directive is only available on uni-processor configurations. The
> +directive @code{@value{DIRPREFIX}interrupt_local_enable} is available on all
> +configurations.
>
> @c
> @c
> @@ -486,6 +495,91 @@ and will be redisabled when this directive returns to
> the caller.
>
> This directive will not cause the calling task to be preempted.
>
> +This directive is only available on uni-processor configurations. The
> +directives @code{@value{DIRPREFIX}interrupt_local_disable} and
> +@code{@value{DIRPREFIX}interrupt_local_enable} is available on all
> +configurations.
> +
> +@c
> +@c
> +@c
> +@page
> +@subsection INTERRUPT_LOCAL_DISABLE - Disable Interrupts on Current Processor
> +
> +@cindex disable interrupts
> +
> +@subheading CALLING SEQUENCE:
> +
> +@ifset is-C
> +@findex rtems_interrupt_local_disable
> +@example
> +void rtems_interrupt_local_disable(
> + rtems_interrupt_level level
> +);
> +
> +/* this is implemented as a macro and sets level as a side-effect */
> +@end example
> +@end ifset
> +
> +@subheading DIRECTIVE STATUS CODES:
> +
> +NONE
> +
> +@subheading DESCRIPTION:
> +
> +This directive disables all maskable interrupts and returns
> +the previous @code{level}. A later invocation of the
> +@code{@value{DIRPREFIX}interrupt_local_enable} directive should be used to
> +restore the interrupt level.
> +
> +@subheading NOTES:
> +
> +This directive will not cause the calling task to be preempted.
> +
> +@ifset is-C
> +@b{This directive is implemented as a macro which modifies the @code{level}
> +parameter.}
> +@end ifset
> +
> +On SMP configurations this will not ensure system wide mutual exclusion. Use
> +interrupt locks instead.
> +
> +@c
> +@c
> +@c
> +@page
> +@subsection INTERRUPT_LOCAL_ENABLE - Enable Interrupts on Current Processor
> +
> +@cindex enable interrupts
> +
> +@subheading CALLING SEQUENCE:
> +
> +@ifset is-C
> +@findex rtems_interrupt_local_enable
> +@example
> +void rtems_interrupt_local_enable(
> + rtems_interrupt_level level
> +);
> +@end example
> +@end ifset
> +
> +@subheading DIRECTIVE STATUS CODES:
> +
> +NONE
> +
> +@subheading DESCRIPTION:
> +
> +This directive enables maskable interrupts to the @code{level}
> +which was returned by a previous call to
> +@code{@value{DIRPREFIX}interrupt_local_disable}.
> +Immediately prior to invoking this directive, maskable interrupts should
> +be disabled by a call to @code{@value{DIRPREFIX}interrupt_local_disable}
> +and will be enabled when this directive returns to the caller.
> +
> +@subheading NOTES:
> +
> +This directive will not cause the calling task to be preempted.
> +
> @c
> @c
> @c
> diff --git a/testsuites/libtests/heapwalk/init.c
> b/testsuites/libtests/heapwalk/init.c
> index 80073e8..3bd6c91 100644
> --- a/testsuites/libtests/heapwalk/init.c
> +++ b/testsuites/libtests/heapwalk/init.c
> @@ -94,12 +94,12 @@ static void test_system_not_up(void)
>
> puts( "start with a system state != SYSTEM_STATE_UP" );
>
> - rtems_interrupt_disable( level );
> + rtems_interrupt_local_disable( level );
> System_state_Codes state = _System_state_Get();
> _System_state_Set( SYSTEM_STATE_TERMINATED );
> test_call_heap_walk( true );
> _System_state_Set( state );
> - rtems_interrupt_enable( level );
> + rtems_interrupt_local_enable( level );
> }
>
> static void test_check_control(void)
> diff --git a/testsuites/samples/Makefile.am b/testsuites/samples/Makefile.am
> index 08455d3..374617b 100644
> --- a/testsuites/samples/Makefile.am
> +++ b/testsuites/samples/Makefile.am
> @@ -18,8 +18,11 @@ endif
> if NETTESTS
> ## loopback tests a network loopback interface
> _SUBDIRS += loopback
> +if HAS_SMP
> +else
> _SUBDIRS += pppd
> endif
> +endif
>
> include $(top_srcdir)/../automake/test-subdirs.am
> include $(top_srcdir)/../automake/local.am
> diff --git a/testsuites/samples/configure.ac b/testsuites/samples/configure.ac
> index e6f12d0..91a3661 100644
> --- a/testsuites/samples/configure.ac
> +++ b/testsuites/samples/configure.ac
> @@ -26,6 +26,7 @@ RTEMS_CHECK_CUSTOM_BSP(RTEMS_BSP)
> RTEMS_CHECK_CPUOPTS([RTEMS_MULTIPROCESSING])
> RTEMS_CHECK_CXX(RTEMS_BSP)
> RTEMS_CHECK_CPUOPTS([RTEMS_NETWORKING])
> +RTEMS_CHECK_CPUOPTS([RTEMS_SMP])
>
> CXXTESTS=$HAS_CPLUSPLUS
> AS_IF([test $HAS_CPLUSPLUS = yes],[
> @@ -52,6 +53,7 @@ AS_IF([test $HAS_CPLUSPLUS = yes],[
> AM_CONDITIONAL([CXXTESTS],[test $CXXTESTS = "yes"])
> AM_CONDITIONAL(NETTESTS,test "$rtems_cv_RTEMS_NETWORKING" = "yes")
> AM_CONDITIONAL(MPTESTS,test "$rtems_cv_RTEMS_MULTIPROCESSING" = "yes")
> +AM_CONDITIONAL(HAS_SMP,test "$rtems_cv_RTEMS_SMP" = "yes")
>
> # FIXME: We should get rid of this. It's a cludge.
> AC_CHECK_SIZEOF([time_t])
> diff --git a/testsuites/sptests/sp37/init.c b/testsuites/sptests/sp37/init.c
> index 0846491..a963a30 100644
> --- a/testsuites/sptests/sp37/init.c
> +++ b/testsuites/sptests/sp37/init.c
> @@ -243,8 +243,7 @@ static void test_clock_tick_functions( void )
> rtems_interrupt_level level;
> Watchdog_Interval saved_ticks;
>
> - _Thread_Disable_dispatch();
> - rtems_interrupt_disable( level );
> + rtems_interrupt_local_disable( level );
>
> saved_ticks = _Watchdog_Ticks_since_boot;
>
> @@ -287,16 +286,19 @@ static void test_clock_tick_functions( void )
>
> _Watchdog_Ticks_since_boot = saved_ticks;
>
> - rtems_interrupt_enable( level );
> - _Thread_Enable_dispatch();
> + rtems_interrupt_local_enable( level );
> }
>
> void test_interrupt_inline(void)
> {
> rtems_interrupt_level level;
> + rtems_interrupt_level level_1;
> rtems_mode level_mode_body;
> rtems_mode level_mode_macro;
> bool in_isr;
> + uint32_t isr_level_0;
> + uint32_t isr_level_1;
> + uint32_t isr_level_2;
>
> puts( "interrupt is in progress (use body)" );
> in_isr = rtems_interrupt_is_in_progress();
> @@ -305,20 +307,33 @@ void test_interrupt_inline(void)
> rtems_test_exit( 0 );
> }
>
> +#if !defined(RTEMS_SMP)
> puts( "interrupt disable (use inline)" );
> - _Thread_Disable_dispatch();
> rtems_interrupt_disable( level );
> - _Thread_Enable_dispatch();
>
> puts( "interrupt flash (use inline)" );
> - _Thread_Disable_dispatch();
> rtems_interrupt_flash( level );
> - _Thread_Enable_dispatch();
>
> puts( "interrupt enable (use inline)" );
> - _Thread_Disable_dispatch();
> rtems_interrupt_enable( level );
> - _Thread_Enable_dispatch();
> +#endif /* RTEMS_SMP */
> +
> + isr_level_0 = _ISR_Get_level();
> + rtems_test_assert( isr_level_0 == 0 );
> +
> + rtems_interrupt_local_disable( level );
> + isr_level_1 = _ISR_Get_level();
> + rtems_test_assert( isr_level_1 != isr_level_0 );
> +
> + rtems_interrupt_local_disable( level_1 );
> + isr_level_2 = _ISR_Get_level();
> + rtems_test_assert( isr_level_2 == isr_level_1 );
> +
> + rtems_interrupt_local_enable( level_1 );
> + rtems_test_assert( _ISR_Get_level() == isr_level_1 );
> +
> + rtems_interrupt_local_enable( level );
> + rtems_test_assert( _ISR_Get_level() == isr_level_0 );
>
> puts( "interrupt level mode (use inline)" );
> level_mode_body = rtems_interrupt_level_body( level );
> @@ -328,7 +343,10 @@ void test_interrupt_inline(void)
> }
> }
>
> +#if !defined(RTEMS_SMP)
> volatile int isr_in_progress_body;
> +#endif
> +
> volatile int isr_in_progress_inline;
>
> void check_isr_in_progress_inline(void)
> @@ -336,25 +354,6 @@ void check_isr_in_progress_inline(void)
> isr_in_progress_inline = rtems_interrupt_is_in_progress() ? 1 : 2;
> }
>
> -#undef rtems_interrupt_disable
> -extern rtems_interrupt_level rtems_interrupt_disable(void);
> -#undef rtems_interrupt_enable
> -extern void rtems_interrupt_enable(rtems_interrupt_level previous_level);
> -#undef rtems_interrupt_flash
> -extern void rtems_interrupt_flash(rtems_interrupt_level previous_level);
> -#undef rtems_interrupt_is_in_progress
> -extern bool rtems_interrupt_is_in_progress(void);
> -
> -rtems_timer_service_routine test_isr_in_progress(
> - rtems_id timer,
> - void *arg
> -)
> -{
> - check_isr_in_progress_inline();
> -
> - isr_in_progress_body = rtems_interrupt_is_in_progress() ? 1 : 2;
> -}
> -
> void check_isr_worked(
> char *s,
> int result
> @@ -429,16 +428,72 @@ rtems_timer_service_routine test_unblock_task(
> directive_failed( status, "rtems_task_resume" );
> }
>
> +#undef rtems_interrupt_disable
> +extern rtems_interrupt_level rtems_interrupt_disable(void);
> +#undef rtems_interrupt_enable
> +extern void rtems_interrupt_enable(rtems_interrupt_level previous_level);
> +#undef rtems_interrupt_flash
> +extern void rtems_interrupt_flash(rtems_interrupt_level previous_level);
> +#undef rtems_interrupt_is_in_progress
> +extern bool rtems_interrupt_is_in_progress(void);
> +
> +static void test_interrupt_body(void)
> +{
> +#if !defined(RTEMS_SMP)
> + rtems_interrupt_level level;
> + rtems_mode level_mode_body;
> + rtems_mode level_mode_macro;
> + bool in_isr;
> +
> + puts( "interrupt disable (use body)" );
> + level = rtems_interrupt_disable();
> +
> + puts( "interrupt disable (use body)" );
> + level = rtems_interrupt_disable();
> +
> + puts( "interrupt flash (use body)" );
> + rtems_interrupt_flash( level );
> +
> + puts( "interrupt enable (use body)" );
> + rtems_interrupt_enable( level );
> +
> + puts( "interrupt level mode (use body)" );
> + level_mode_body = rtems_interrupt_level_body( level );
> + level_mode_macro = RTEMS_INTERRUPT_LEVEL(level);
> + if ( level_mode_macro == level_mode_body ) {
> + puts("test seems to work");
> + }
> +
> + /*
> + * Test interrupt bodies
> + */
> + puts( "interrupt is in progress (use body)" );
> + in_isr = rtems_interrupt_is_in_progress();
> + if ( in_isr ) {
> + puts( "interrupt reported to be is in progress (body)" );
> + rtems_test_exit( 0 );
> + }
> +#endif /* RTEMS_SMP */
> +}
> +
> +rtems_timer_service_routine test_isr_in_progress(
> + rtems_id timer,
> + void *arg
> +)
> +{
> + check_isr_in_progress_inline();
> +
> +#if !defined(RTEMS_SMP)
> + isr_in_progress_body = rtems_interrupt_is_in_progress() ? 1 : 2;
> +#endif
> +}
> +
> rtems_task Init(
> rtems_task_argument argument
> )
> {
> rtems_time_of_day time;
> rtems_status_code status;
> - rtems_interrupt_level level;
> - rtems_mode level_mode_body;
> - rtems_mode level_mode_macro;
> - bool in_isr;
> rtems_id timer;
> int i;
>
> @@ -523,47 +578,8 @@ rtems_task Init(
> break;
> }
>
> - /*
> - * Test interrupt inline versions
> - */
> test_interrupt_inline();
> -
> - /*
> - * Test interrupt bodies
> - */
> - puts( "interrupt is in progress (use body)" );
> - in_isr = rtems_interrupt_is_in_progress();
> - if ( in_isr ) {
> - puts( "interrupt reported to be is in progress (body)" );
> - rtems_test_exit( 0 );
> - }
> -
> - puts( "interrupt disable (use body)" );
> - _Thread_Disable_dispatch();
> - level = rtems_interrupt_disable();
> - _Thread_Enable_dispatch();
> -
> - puts( "interrupt disable (use body)" );
> - _Thread_Disable_dispatch();
> - level = rtems_interrupt_disable();
> - _Thread_Enable_dispatch();
> -
> - puts( "interrupt flash (use body)" );
> - _Thread_Disable_dispatch();
> - rtems_interrupt_flash( level );
> - _Thread_Enable_dispatch();
> -
> - puts( "interrupt enable (use body)" );
> - _Thread_Disable_dispatch();
> - rtems_interrupt_enable( level );
> - _Thread_Enable_dispatch();
> -
> - puts( "interrupt level mode (use body)" );
> - level_mode_body = rtems_interrupt_level_body( level );
> - level_mode_macro = RTEMS_INTERRUPT_LEVEL(level);
> - if ( level_mode_macro == level_mode_body ) {
> - puts("test seems to work");
> - }
> + test_interrupt_body();
>
> /*
> * Test ISR in progress from actual ISR
> @@ -576,7 +592,9 @@ rtems_task Init(
>
> check_isr_worked( "inline", isr_in_progress_inline );
>
> +#if !defined(RTEMS_SMP)
> check_isr_worked( "body", isr_in_progress_body );
> +#endif
>
> TEST_END();
> rtems_test_exit( 0 );
> diff --git a/testsuites/sptests/sp40/init.c b/testsuites/sptests/sp40/init.c
> index 5b0ad7d..d3b547b 100644
> --- a/testsuites/sptests/sp40/init.c
> +++ b/testsuites/sptests/sp40/init.c
> @@ -42,16 +42,14 @@ static rtems_driver_address_table test_driver = {
>
> #define test_interrupt_context_enter( level ) \
> do { \
> - _Thread_Disable_dispatch(); \
> - rtems_interrupt_disable( level ); \
> + rtems_interrupt_local_disable( level ); \
> ++_ISR_Nest_level; \
> } while (0)
>
> #define test_interrupt_context_leave( level ) \
> do { \
> --_ISR_Nest_level; \
> - rtems_interrupt_enable( level ); \
> - _Thread_Enable_dispatch(); \
> + rtems_interrupt_local_enable( level ); \
> } while (0)
>
> rtems_task Init(
> diff --git a/testsuites/tmtests/tm26/task1.c b/testsuites/tmtests/tm26/task1.c
> index ee66218..5b19c3d 100644
> --- a/testsuites/tmtests/tm26/task1.c
> +++ b/testsuites/tmtests/tm26/task1.c
> @@ -341,15 +341,20 @@ rtems_task High_task(
> _Thread_Disable_dispatch();
>
> benchmark_timer_initialize();
> - rtems_interrupt_disable( level );
> + rtems_interrupt_local_disable( level );
> isr_disable_time = benchmark_timer_read();
>
> benchmark_timer_initialize();
> +#if defined(RTEMS_SMP)
> + rtems_interrupt_local_enable( level );
> + rtems_interrupt_local_disable( level );
> +#else
> rtems_interrupt_flash( level );
> +#endif
> isr_flash_time = benchmark_timer_read();
>
> benchmark_timer_initialize();
> - rtems_interrupt_enable( level );
> + rtems_interrupt_local_enable( level );
> isr_enable_time = benchmark_timer_read();
>
> _Thread_Enable_dispatch();
> --
> 1.8.1.4
>
> _______________________________________________
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
