[PATCH 3/3] posix: Allow pthread_cancel() from within ISRs

[Gedare Bloom]

These looked ok to me, I skimmed before but forgot to ACK...

On Mon, May 17, 2021 at 7:07 AM Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
>
> Close #4413.
> ---
>  cpukit/posix/src/cancel.c              | 13 ++---
>  testsuites/psxtests/psxcancel01/init.c | 72 +++++++++++++++-----------
>  2 files changed, 45 insertions(+), 40 deletions(-)
>
> diff --git a/cpukit/posix/src/cancel.c b/cpukit/posix/src/cancel.c
> index f2636e6a97..aa4a434037 100644
> --- a/cpukit/posix/src/cancel.c
> +++ b/cpukit/posix/src/cancel.c
> @@ -38,14 +38,6 @@ int pthread_cancel( pthread_t thread )
>    Thread_Control   *executing;
>    Per_CPU_Control  *cpu_self;
>
> -  /*
> -   *  Don't even think about deleting a resource from an ISR.
> -   */

But I really appreciated the irony of removing this comment.

> -
> -  if ( _ISR_Is_in_progress() ) {
> -    return EPROTO;
> -  }
> -
>    the_thread = _Thread_Get( thread, &lock_context );
>
>    if ( the_thread == NULL ) {
> @@ -55,7 +47,10 @@ int pthread_cancel( pthread_t thread )
>    cpu_self = _Per_CPU_Get();
>    executing = _Per_CPU_Get_executing( cpu_self );
>
> -  if ( the_thread == executing ) {
> +  if (
> +    the_thread == executing &&
> +    !_Per_CPU_Is_ISR_in_progress( cpu_self )
> +  ) {
>      _ISR_lock_ISR_enable( &lock_context );
>      _Thread_Exit( PTHREAD_CANCELED, THREAD_LIFE_TERMINATING );
>    } else {
> diff --git a/testsuites/psxtests/psxcancel01/init.c b/testsuites/psxtests/psxcancel01/init.c
> index d87295ccd1..b0da991bad 100644
> --- a/testsuites/psxtests/psxcancel01/init.c
> +++ b/testsuites/psxtests/psxcancel01/init.c
> @@ -18,38 +18,37 @@ const char rtems_test_name[] = "PSXCANCEL 1";
>
>  /* forward declarations to avoid warnings */
>  void *POSIX_Init(void *argument);
> -rtems_timer_service_routine Cancel_duringISR_TSR(
> -  rtems_id  ignored_id,
> -  void     *ignored_address
> -);
> -rtems_timer_service_routine SetState_duringISR_TSR(
> -  rtems_id  ignored_id,
> -  void     *ignored_address
> -);
> -rtems_timer_service_routine SetType_duringISR_TSR(
> -  rtems_id  ignored_id,
> -  void     *ignored_address
> -);
> -void doit(
> -  rtems_timer_service_routine (*TSR)(rtems_id, void *),
> -  const char                   *method
> -);
>
> -volatile int TSR_occurred;
> -volatile int TSR_status;
> +static volatile int TSR_occurred;
> +
> +static volatile int TSR_status;
> +
> +static rtems_id  timer_id;
>
> -rtems_id  timer_id;
> +static pthread_t thread;
>
> -rtems_timer_service_routine Cancel_duringISR_TSR(
> +static void *suspend_self( void *arg )
> +{
> +  rtems_status_code status;
> +
> +  (void) arg;
> +
> +  status = rtems_task_suspend( RTEMS_SELF);
> +  rtems_test_assert( status == RTEMS_SUCCESSFUL );
> +
> +  return NULL;
> +}
> +
> +static rtems_timer_service_routine Cancel_duringISR_TSR(
>    rtems_id  ignored_id,
>    void     *ignored_address
>  )
>  {
> -  TSR_status = pthread_cancel( pthread_self() );
> +  TSR_status = pthread_cancel( thread );
>    TSR_occurred = 1;
>  }
>
> -rtems_timer_service_routine SetState_duringISR_TSR(
> +static rtems_timer_service_routine SetState_duringISR_TSR(
>    rtems_id  ignored_id,
>    void     *ignored_address
>  )
> @@ -60,7 +59,7 @@ rtems_timer_service_routine SetState_duringISR_TSR(
>    TSR_occurred = 1;
>  }
>
> -rtems_timer_service_routine SetType_duringISR_TSR(
> +static rtems_timer_service_routine SetType_duringISR_TSR(
>    rtems_id  ignored_id,
>    void     *ignored_address
>  )
> @@ -71,9 +70,10 @@ rtems_timer_service_routine SetType_duringISR_TSR(
>    TSR_occurred = 1;
>  }
>
> -void doit(
> +static void doit(
>    rtems_timer_service_routine (*TSR)(rtems_id, void *),
> -  const char                   *method
> +  const char                   *method,
> +  int                           expected_status
>  )
>  {
>    rtems_interval    start;
> @@ -97,11 +97,11 @@ void doit(
>      printf( "%s did not occur\n", method );
>      rtems_test_exit(0);
>    }
> -  if ( TSR_status != EPROTO ) {
> +  if ( TSR_status != expected_status ) {
>      printf( "%s returned %s\n", method, strerror(TSR_status) );
>      rtems_test_exit(0);
>    }
> -  printf( "%s - from ISR returns EPROTO - OK\n", method );
> +  printf( "%s - from ISR returns expected status - OK\n", method );
>
>  }
>
> @@ -110,6 +110,8 @@ void *POSIX_Init(
>  )
>  {
>    rtems_status_code status;
> +  int               eno;
> +  void             *value;
>
>    TEST_BEGIN();
>
> @@ -119,9 +121,17 @@ void *POSIX_Init(
>    );
>    rtems_test_assert( !status );
>
> -  doit( Cancel_duringISR_TSR, "pthread_cancel" );
> -  doit( SetState_duringISR_TSR, "pthread_setcancelstate" );
> -  doit( SetType_duringISR_TSR, "pthread_setcanceltype" );
> +  eno = pthread_create( &thread, NULL, suspend_self, NULL );
> +  rtems_test_assert( eno == 0 );
> +
> +  doit( Cancel_duringISR_TSR, "pthread_cancel", 0 );
> +  doit( SetState_duringISR_TSR, "pthread_setcancelstate", EPROTO );
> +  doit( SetType_duringISR_TSR, "pthread_setcanceltype", EPROTO );
> +
> +  value = NULL;
> +  eno = pthread_join( thread, &value );
> +  rtems_test_assert( eno == 0 );
> +  rtems_test_assert( value == PTHREAD_CANCELED );
>
>    TEST_END();
>    rtems_test_exit(0);
> @@ -137,7 +147,7 @@ void *POSIX_Init(
>
>  #define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
>
> -#define CONFIGURE_MAXIMUM_POSIX_THREADS        1
> +#define CONFIGURE_MAXIMUM_POSIX_THREADS        2
>  #define CONFIGURE_POSIX_INIT_THREAD_TABLE
>
>  #define CONFIGURE_INIT
> --
> 2.26.2
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel