[PATCH 1/2] Add mutex around semaphore counting (LP:#1017388)

[Colin Ian King]

On 17/07/12 19:04, Alex Hung wrote:
> On 07/17/2012 02:08 AM, Colin King wrote:
>> From: Colin Ian King <colin.king at canonical.com>
>>
>> The ACPICA core can execute control methods in a non
>> serialized manner, so we should add a mutex lock around
>> the semaphore counting mechanism to remove any race
>> conditions.  This fixes bug LP:#1017388
>>
>> Signed-off-by: Colin Ian King <colin.king at canonical.com>
>> ---
>>   src/acpica/fwts_acpica.c |   16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
>> index b8119cb..cf9a4fe 100644
>> --- a/src/acpica/fwts_acpica.c
>> +++ b/src/acpica/fwts_acpica.c
>> @@ -29,6 +29,9 @@
>>   #include <errno.h>
>>   #include <semaphore.h>
>>   #include <signal.h>
>> +#include <pthread.h>
>> +
>> +static pthread_mutex_t mutex_lock_count;
>>
>>   #include "fwts.h"
>>
>> @@ -109,10 +112,14 @@ void fwts_acpica_sem_count_clear(void)
>>   {
>>       int i;
>>
>> +    pthread_mutex_lock(&mutex_lock_count);
>> +
>>       for (i=0;i<MAX_SEMAPHORES;i++) {
>>           sem_hash_table[i].sem = NULL;
>>           sem_hash_table[i].count = 0;
>>       }
>> +
>> +    pthread_mutex_unlock(&mutex_lock_count);
>>   }
>>
>>   /*
>> @@ -171,9 +178,12 @@ static unsigned int hash_sem_handle(sem_t *sem)
>>   static void hash_sem_inc_count(sem_t *sem)
>>   {
>>       unsigned int i = hash_sem_handle(sem);
>> +
>>       if (i != HASH_FULL) {
>> +        pthread_mutex_lock(&mutex_lock_count);
>>           sem_hash_table[i].sem = sem;
>>           sem_hash_table[i].count++;
>> +        pthread_mutex_unlock(&mutex_lock_count);
>>       }
>>   }
>>
>> @@ -184,9 +194,12 @@ static void hash_sem_inc_count(sem_t *sem)
>>   static void hash_sem_dec_count(sem_t *sem)
>>   {
>>       unsigned int i = hash_sem_handle(sem);
>> +
>>       if (i != HASH_FULL) {
>> +        pthread_mutex_lock(&mutex_lock_count);
>>           sem_hash_table[i].sem = sem;
>>           sem_hash_table[i].count--;
>> +        pthread_mutex_unlock(&mutex_lock_count);
>>       }
>>   }
>>
>> @@ -788,6 +801,8 @@ int fwts_acpica_init(fwts_framework *fw)
>>       if (fwts_acpica_init_called)
>>           return FWTS_ERROR;
>>
>> +    pthread_mutex_init(&mutex_lock_count, NULL);
>> +
>>       fwts_acpica_fw = fw;
>>
>>       AcpiDbgLevel = ACPI_NORMAL_DEFAULT;
>> @@ -974,6 +989,7 @@ int fwts_acpica_deinit(void)
>>           return FWTS_ERROR;
>>
>>       AcpiTerminate();
>> +    pthread_mutex_destroy(&mutex_lock_count);
>>
>>       FWTS_ACPICA_FREE(fwts_acpica_XSDT);
>>       FWTS_ACPICA_FREE(fwts_acpica_RSDT);
>>
>
> The patch looks good and necessary and worthes an ACK; however, fwts
> still generates the error messages: FAILED [MEDIUM] AMLLocksAcquired:
> Test 61, \_WAK left 1 locks in an acquired state
>
> I think it occurs less frequently. Can someone help test it, too?

After a lot of debugging tonight I found that this occurs in _WAK notify 
events. The notify is creating a thread which tries to execute _NOT 
which does not exit before the _WAK completes, hence we can end up with 
a notify thread still holding a mutex which causes the acquired lock 
issue.  At least I now know why it's failing.  Now I need to figure out 
a suitable fix.

Colin
>
> PS. I also attach the result (10 times of fwts method").
>
>
>