Possible bug in _CORE_mutex_Seize()

[Joel Sherrill]

Till Straumann wrote:

> Joel Sherrill wrote:
>
>> Phil Torre wrote:
>>
>>> I'm running now with some of the changes suggested by Till and Joel:
>>>
>>> 1.    "Paranoia" added to _CORE_mutex_Seize() which checks
>>>     _Thread_Dispatch_disable_level and throws an internal error if
>>>     its nonzero when it shouldn't be.
>>
>>
>>
>> Be careful with this one.  I don't think this is the right place to
>> check it. You can attempt to obtain a mutex in an ISR and
>> _Thread_Dispatch_disable_level will definitely be non-zero.  You
>> can't use the Allocator Mutex when _Thread_Dispatch_disable_level
>> is non-zero but it is conceivable that you could use another mutex.
>>
>
> ??? What is a legal scenario to obtain a CORE_mutex in an ISR - 
> especially
> with _wait == TRUE  -- and even if it is FALSE, I doubt it's OK. The ISR
> would lock a mutex for the interrupted task. 

You are right in that it is only valid to call it for non-blocking 
requests.  But the
ISR disable part is OK since _ISR_Disable/_ISR_Enable pairs save the current
ISR mask level and restore it. 

> Plus: CORE_mutex_Seize() is always executed with IRQs disabled - it's 
> hard
> to imagine that it should be legal to call it from an ISR.

That isn't a problem since _ISR_Enable just restores the level from the 
_ISR_Disable.
It doesn't arbitrarily enable them.

>
>
> The simplest scenario goes like this (summarizing):
>
> if ( !_CORE_mutex_Is_locked( the_mutex ) )
>   the_mutex->holder = _Thread_Executing;
>   /* set other fields in the_mutex and _Thread_Executing */
>   _ISR_enable(level);
> return 0;
>
> It is *legal* however, to call CORE_mutex_Seize() from a dispatching
> disabled section (in a task context) provided that "_wait == FALSE", a 
> case
> which is tolerated by the paranoia check. Of course, the caller is 
> expected
> to check/convert the ("indirect") return code before proceeding.

Exactly.  Sounds like I thought your check was too tight and it isn't.

> I added the check at the top of CORE_mutex_Seize() just *because*
> I'd like to catch other possible misuses of mutexes. 

OK.  But in general we don't want too tight a check.  I t hink you managed
to do that from what I am seeing.

>
> So far, I caught no other one running a rather complex EPICS application.

That's great to hear.

>
> T.
>
>> I would take this out.
>>
>>> 2.    The reent struct is now allocated from the workspace using
>>>     _Workspace_Allocate() from libc_create_hook rather than the start
>>>     hook.  If _Workspace_Allocate() returns NULL, the create hook
>>>     returns false.
>>>
>>> This seems to be working (given several minutes of exhaustive testing),
>>> but I notice a few things:  Since I'm now calling _Workspace_Allocate()
>>> directly, _RTEMS_Allocator_mutex is not being locked.  That should 
>>> be OK, because _Thread_Dispatch_disable_level is nonzero, correct?
>>
>>
>>
>> Correct.
>>
>> It should be right because _Workspace_Allocate() is a normal part of
>> rtems_task_create().  The stack, FP context, and API extensions
>> are allocated from the Workspace as part of task creation.
>>
>>
>>
>>> -Phil
>>>
>>
>>
>
>