| Message ID | 1337159028-20597-1-git-send-email-colin.king@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, May 16, 2012 at 5:03 PM, Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > The ACPICA Semaphore deletion does not free the allocated semaphore so > we re-implement the creation and deletion for fwts to fix this ACPICA core > bug. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > src/acpica/Makefile.am | 2 ++ > src/acpica/fwts_acpica.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+) > > diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am > index 5a475e9..7d1249c 100644 > --- a/src/acpica/Makefile.am > +++ b/src/acpica/Makefile.am > @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c > sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \ > sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \ > sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \ > + sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \ > + sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \ > sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \ > sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \ > sed 's/^AcpiOsSleep/__AcpiOsSleep/' \ > diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c > index 3dc5ced..b8119cb 100644 > --- a/src/acpica/fwts_acpica.c > +++ b/src/acpica/fwts_acpica.c > @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args) > } > > /* > + * AcpiOsCreateSemaphore() > + * Override ACPICA AcpiOsCreateSemaphore > + */ > +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits, > + UINT32 InitialUnits, > + ACPI_HANDLE *OutHandle) > +{ > + sem_t *sem; > + > + if (!OutHandle) > + return AE_BAD_PARAMETER; > + > + sem = AcpiOsAllocate(sizeof(sem_t)); > + if (!sem) > + return AE_NO_MEMORY; > + > + if (sem_init(sem, 0, InitialUnits) == -1) { > + AcpiOsFree(sem); > + return AE_BAD_PARAMETER; > + } > + > + *OutHandle = (ACPI_HANDLE)sem; > + > + return AE_OK; > +} > + > +/* > + * AcpiOsDeleteSemaphore() > + * Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing > + */ > +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle) > +{ > + sem_t *sem = (sem_t *)Handle; > + > + if (!sem) > + return AE_BAD_PARAMETER; > + > + if (sem_destroy(sem) == -1) > + return AE_BAD_PARAMETER; > + > + AcpiOsFree(sem); > + > + return AE_OK; > +} > + > + > +/* > * AcpiOsWaitSemaphore() > * Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires > * so that we can see if any methods are sloppy in their releases. > -- > 1.7.10 > Hi Colin, Is there a leak detected by valgrind on this? sem_destroy() seems to be supposed to do the house keeping. cheers, -kengyu
On 17/05/12 07:43, Keng-Yu Lin wrote: > On Wed, May 16, 2012 at 5:03 PM, Colin King <colin.king@canonical.com> wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> The ACPICA Semaphore deletion does not free the allocated semaphore so >> we re-implement the creation and deletion for fwts to fix this ACPICA core >> bug. >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> src/acpica/Makefile.am | 2 ++ >> src/acpica/fwts_acpica.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am >> index 5a475e9..7d1249c 100644 >> --- a/src/acpica/Makefile.am >> +++ b/src/acpica/Makefile.am >> @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c >> sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \ >> sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \ >> sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \ >> + sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \ >> + sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \ >> sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \ >> sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \ >> sed 's/^AcpiOsSleep/__AcpiOsSleep/' \ >> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c >> index 3dc5ced..b8119cb 100644 >> --- a/src/acpica/fwts_acpica.c >> +++ b/src/acpica/fwts_acpica.c >> @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args) >> } >> >> /* >> + * AcpiOsCreateSemaphore() >> + * Override ACPICA AcpiOsCreateSemaphore >> + */ >> +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits, >> + UINT32 InitialUnits, >> + ACPI_HANDLE *OutHandle) >> +{ >> + sem_t *sem; >> + >> + if (!OutHandle) >> + return AE_BAD_PARAMETER; >> + >> + sem = AcpiOsAllocate(sizeof(sem_t)); >> + if (!sem) >> + return AE_NO_MEMORY; >> + >> + if (sem_init(sem, 0, InitialUnits) == -1) { >> + AcpiOsFree(sem); >> + return AE_BAD_PARAMETER; >> + } >> + >> + *OutHandle = (ACPI_HANDLE)sem; >> + >> + return AE_OK; >> +} >> + >> +/* >> + * AcpiOsDeleteSemaphore() >> + * Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing >> + */ >> +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle) >> +{ >> + sem_t *sem = (sem_t *)Handle; >> + >> + if (!sem) >> + return AE_BAD_PARAMETER; >> + >> + if (sem_destroy(sem) == -1) >> + return AE_BAD_PARAMETER; >> + >> + AcpiOsFree(sem); >> + >> + return AE_OK; >> +} >> + >> + >> +/* >> * AcpiOsWaitSemaphore() >> * Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires >> * so that we can see if any methods are sloppy in their releases. >> -- >> 1.7.10 >> > > Hi Colin, > Is there a leak detected by valgrind on this? Yes there is, this is how I found it. > sem_destroy() seems to be supposed to do the house keeping. > sem_destroy() destroys the semaphore but we still have the memory allocated for the semaphore to free afterwards, as allocated by the AcpiOsCreateSemaphore(). Normal use where the semaphore is a stack based variable is as follows: sem_t sem; sem_init(&sem, ...); .. do stuff.. sem_destroy(&sem); and for a heap bases semaphore: sem_t *sem; sem = malloc(sizeof(sem_t)); sem_init(sem, ...); ... do stuff .. sem_destroy(sem); free(sem); Colin > cheers, > -kengyu >
On Thu, May 17, 2012 at 4:14 PM, Colin Ian King <colin.king@canonical.com> wrote: > On 17/05/12 07:43, Keng-Yu Lin wrote: >> >> On Wed, May 16, 2012 at 5:03 PM, Colin King <colin.king@canonical.com> >> wrote: >>> >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> The ACPICA Semaphore deletion does not free the allocated semaphore so >>> we re-implement the creation and deletion for fwts to fix this ACPICA >>> core >>> bug. >>> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>> --- >>> src/acpica/Makefile.am | 2 ++ >>> src/acpica/fwts_acpica.c | 47 >>> ++++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 49 insertions(+) >>> >>> diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am >>> index 5a475e9..7d1249c 100644 >>> --- a/src/acpica/Makefile.am >>> +++ b/src/acpica/Makefile.am >>> @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c >>> sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' >>> | \ >>> sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \ >>> sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \ >>> + sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \ >>> + sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \ >>> sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \ >>> sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \ >>> sed 's/^AcpiOsSleep/__AcpiOsSleep/' \ >>> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c >>> index 3dc5ced..b8119cb 100644 >>> --- a/src/acpica/fwts_acpica.c >>> +++ b/src/acpica/fwts_acpica.c >>> @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args) >>> } >>> >>> /* >>> + * AcpiOsCreateSemaphore() >>> + * Override ACPICA AcpiOsCreateSemaphore >>> + */ >>> +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits, >>> + UINT32 InitialUnits, >>> + ACPI_HANDLE *OutHandle) >>> +{ >>> + sem_t *sem; >>> + >>> + if (!OutHandle) >>> + return AE_BAD_PARAMETER; >>> + >>> + sem = AcpiOsAllocate(sizeof(sem_t)); >>> + if (!sem) >>> + return AE_NO_MEMORY; >>> + >>> + if (sem_init(sem, 0, InitialUnits) == -1) { >>> + AcpiOsFree(sem); >>> + return AE_BAD_PARAMETER; >>> + } >>> + >>> + *OutHandle = (ACPI_HANDLE)sem; >>> + >>> + return AE_OK; >>> +} >>> + >>> +/* >>> + * AcpiOsDeleteSemaphore() >>> + * Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory >>> freeing >>> + */ >>> +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle) >>> +{ >>> + sem_t *sem = (sem_t *)Handle; >>> + >>> + if (!sem) >>> + return AE_BAD_PARAMETER; >>> + >>> + if (sem_destroy(sem) == -1) >>> + return AE_BAD_PARAMETER; >>> + >>> + AcpiOsFree(sem); >>> + >>> + return AE_OK; >>> +} >>> + >>> + >>> +/* >>> * AcpiOsWaitSemaphore() >>> * Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore >>> acquires >>> * so that we can see if any methods are sloppy in their releases. >>> -- >>> 1.7.10 >>> >> >> Hi Colin, >> Is there a leak detected by valgrind on this? > > > Yes there is, this is how I found it. > > >> sem_destroy() seems to be supposed to do the house keeping. >> > > sem_destroy() destroys the semaphore but we still have the memory allocated > for the semaphore to free afterwards, as allocated by the > AcpiOsCreateSemaphore(). Normal use where the semaphore is a stack based > variable is as follows: > > sem_t sem; > > sem_init(&sem, ...); > .. do stuff.. > > sem_destroy(&sem); > > and for a heap bases semaphore: > > sem_t *sem; > > sem = malloc(sizeof(sem_t)); > > sem_init(sem, ...); > ... do stuff .. > > sem_destroy(sem); > free(sem); > > Colin > Thanks for providing this. :-) Acked-by: Keng-Yu Lin <kengyu@canonical.com>
On 05/16/2012 05:03 PM, Colin King wrote: > From: Colin Ian King<colin.king@canonical.com> > > The ACPICA Semaphore deletion does not free the allocated semaphore so > we re-implement the creation and deletion for fwts to fix this ACPICA core > bug. > > Signed-off-by: Colin Ian King<colin.king@canonical.com> > --- > src/acpica/Makefile.am | 2 ++ > src/acpica/fwts_acpica.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+) > > diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am > index 5a475e9..7d1249c 100644 > --- a/src/acpica/Makefile.am > +++ b/src/acpica/Makefile.am > @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c > sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \ > sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \ > sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \ > + sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \ > + sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \ > sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \ > sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \ > sed 's/^AcpiOsSleep/__AcpiOsSleep/' \ > diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c > index 3dc5ced..b8119cb 100644 > --- a/src/acpica/fwts_acpica.c > +++ b/src/acpica/fwts_acpica.c > @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args) > } > > /* > + * AcpiOsCreateSemaphore() > + * Override ACPICA AcpiOsCreateSemaphore > + */ > +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits, > + UINT32 InitialUnits, > + ACPI_HANDLE *OutHandle) > +{ > + sem_t *sem; > + > + if (!OutHandle) > + return AE_BAD_PARAMETER; > + > + sem = AcpiOsAllocate(sizeof(sem_t)); > + if (!sem) > + return AE_NO_MEMORY; > + > + if (sem_init(sem, 0, InitialUnits) == -1) { > + AcpiOsFree(sem); > + return AE_BAD_PARAMETER; > + } > + > + *OutHandle = (ACPI_HANDLE)sem; > + > + return AE_OK; > +} > + > +/* > + * AcpiOsDeleteSemaphore() > + * Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing > + */ > +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle) > +{ > + sem_t *sem = (sem_t *)Handle; > + > + if (!sem) > + return AE_BAD_PARAMETER; > + > + if (sem_destroy(sem) == -1) > + return AE_BAD_PARAMETER; > + > + AcpiOsFree(sem); > + > + return AE_OK; > +} > + > + > +/* > * AcpiOsWaitSemaphore() > * Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires > * so that we can see if any methods are sloppy in their releases. Acked-by: Ivan Hu<ivan.hu@canonical.com>
diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am index 5a475e9..7d1249c 100644 --- a/src/acpica/Makefile.am +++ b/src/acpica/Makefile.am @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \ sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \ sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \ + sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \ + sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \ sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \ sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \ sed 's/^AcpiOsSleep/__AcpiOsSleep/' \ diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c index 3dc5ced..b8119cb 100644 --- a/src/acpica/fwts_acpica.c +++ b/src/acpica/fwts_acpica.c @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args) } /* + * AcpiOsCreateSemaphore() + * Override ACPICA AcpiOsCreateSemaphore + */ +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits, + UINT32 InitialUnits, + ACPI_HANDLE *OutHandle) +{ + sem_t *sem; + + if (!OutHandle) + return AE_BAD_PARAMETER; + + sem = AcpiOsAllocate(sizeof(sem_t)); + if (!sem) + return AE_NO_MEMORY; + + if (sem_init(sem, 0, InitialUnits) == -1) { + AcpiOsFree(sem); + return AE_BAD_PARAMETER; + } + + *OutHandle = (ACPI_HANDLE)sem; + + return AE_OK; +} + +/* + * AcpiOsDeleteSemaphore() + * Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing + */ +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle) +{ + sem_t *sem = (sem_t *)Handle; + + if (!sem) + return AE_BAD_PARAMETER; + + if (sem_destroy(sem) == -1) + return AE_BAD_PARAMETER; + + AcpiOsFree(sem); + + return AE_OK; +} + + +/* * AcpiOsWaitSemaphore() * Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires * so that we can see if any methods are sloppy in their releases.