| Message ID | 1342462140-16674-2-git-send-email-colin.king@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
On 07/17/2012 02:08 AM, Colin King wrote: > From: Colin Ian King <colin.king@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@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? PS. I also attach the result (10 times of fwts method").
On 07/17/2012 02:08 AM, Colin King wrote: > From: Colin Ian King <colin.king@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@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); > Acked-by: Ivan Hu<ivan.hu@canonical.com>
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@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@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? Can you dump the acpitables and send them to me (off the mailing list) so I can see why you are seeing the locking issue? Thanks, Colin > > PS. I also attach the result (10 times of fwts method"). > > >
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@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@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"). > > >
On 07/17/2012 02:08 AM, Colin King wrote: > From: Colin Ian King <colin.king@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@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); > Acked-by: Alex Hung <alex.hung@canonical.com>
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);