| Message ID | 1246516045.3326.0.camel@minggr.sh.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Before we start extending the existing mechanisms, I think we need to really think about a better solution. Since there already appears to be a workaround for the problem, then this should give us some time to analyze this. >-----Original Message----- >From: Lin, Ming M >Sent: Wednesday, July 01, 2009 11:27 PM >To: Len Brown >Cc: Thomas Renninger; Moore, Robert; Zhao, Yakui; linux-acpi >Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete >acpi_os_validate_address interface" > >On Thu, 2009-07-02 at 10:03 +0800, Len Brown wrote: >> I can't apply a patch that adds a known memory leak, >> even if it removes a conflict between drivers. >> >> The reason is that there is a workaround for the driver conflict, >> but no workaround for the memory leak. > >Here is a prototype simple patch, please review to see if it is the >right way to go. > >1) add a new field "count" to struct acpi_res_list. > > When inserting, if the region(addr, len) is already in the resource > list, we just increase "count", otherwise, the region is inserted > with count=1. > > When deleting, the "count" is decreased, if it's decreased to 0, > the region is deleted from the resource list. > > With "count", the region with same address and length can only be > inserted to the resource list once, so prevent potential memory leak. > >2) add a new function acpi_os_invalidate_address, which is called when > region is deleted. > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 02 July 2009 08:27:25 Lin Ming wrote: > On Thu, 2009-07-02 at 10:03 +0800, Len Brown wrote: > > I can't apply a patch that adds a known memory leak, > > even if it removes a conflict between drivers. > > > > The reason is that there is a workaround for the driver conflict, > > but no workaround for the memory leak. > > Here is a prototype simple patch, please review to see if it is the > right way to go. > > 1) add a new field "count" to struct acpi_res_list. > > When inserting, if the region(addr, len) is already in the resource > list, we just increase "count", otherwise, the region is inserted > with count=1. > > When deleting, the "count" is decreased, if it's decreased to 0, > the region is deleted from the resource list. > > With "count", the region with same address and length can only be > inserted to the resource list once, so prevent potential memory leak. Is the counting really needed? An OpRegion inside a method should get deleted when the function is exited and acpi_os_invalidate_address will get called. I like it. Thanks for looking into that! Thomas > > 2) add a new function acpi_os_invalidate_address, which is called when > region is deleted. > > > diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c > index bc17103..96e26e7 100644 > --- a/drivers/acpi/acpica/utdelete.c > +++ b/drivers/acpi/acpica/utdelete.c > @@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union acpi_operand_object *object) > ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS, > "***** Region %p\n", object)); > > + /* Invalidate the region address/length via the host OS */ > + > + acpi_os_invalidate_address(object->region.space_id, > + object->region.address, > + (acpi_size) object->region.length); > + > second_desc = acpi_ns_get_secondary_object(object); > if (second_desc) { > /* > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index 7167071..620a65a 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -88,6 +88,7 @@ struct acpi_res_list { > char name[5]; /* only can have a length of 4 chars, make use of this > one instead of res->name, no need to kalloc then */ > struct list_head resource_list; > + int count; > }; > > static LIST_HEAD(resource_list_head); > @@ -1333,6 +1334,88 @@ acpi_os_validate_interface (char *interface) > return AE_SUPPORT; > } > > +static inline int acpi_res_list_add(struct acpi_res_list *res) > +{ > + struct acpi_res_list *res_list_elem; > + > + list_for_each_entry(res_list_elem, &resource_list_head, > + resource_list) { > + > + if (res->resource_type == res_list_elem->resource_type && > + res->start == res_list_elem->start && > + res->end == res_list_elem->end) { > + > + /* > + * The Region(addr,len) already exist in the list, > + * just increase the count > + */ > + > + res_list_elem->count++; > + return 0; > + } > + } > + > + res->count = 1; > + list_add(&res->resource_list, &resource_list_head); > + return 1; > +} > + > +static inline void acpi_res_list_del(struct acpi_res_list *res) > +{ > + struct acpi_res_list *res_list_elem; > + > + list_for_each_entry(res_list_elem, &resource_list_head, > + resource_list) { > + > + if (res->resource_type == res_list_elem->resource_type && > + res->start == res_list_elem->start && > + res->end == res_list_elem->end) { > + > + /* > + * If the res count is decreased to 0, remove and free it > + */ > + > + if (--res_list_elem->count == 0) { > + list_del(&res_list_elem->resource_list); > + kfree(res_list_elem); > + } > + return; > + } > + } > +} > + > +acpi_status > +acpi_os_invalidate_address( > + u8 space_id, > + acpi_physical_address address, > + acpi_size length) > +{ > + struct acpi_res_list res; > + > + switch (space_id) { > + case ACPI_ADR_SPACE_SYSTEM_IO: > + case ACPI_ADR_SPACE_SYSTEM_MEMORY: > + /* Only interference checks against SystemIO and SytemMemory > + are needed */ > + res.start = address; > + res.end = address + length - 1; > + res.resource_type = space_id; > + spin_lock(&acpi_res_lock); > + acpi_res_list_del(&res); > + spin_unlock(&acpi_res_lock); > + break; > + case ACPI_ADR_SPACE_PCI_CONFIG: > + case ACPI_ADR_SPACE_EC: > + case ACPI_ADR_SPACE_SMBUS: > + case ACPI_ADR_SPACE_CMOS: > + case ACPI_ADR_SPACE_PCI_BAR_TARGET: > + case ACPI_ADR_SPACE_DATA_TABLE: > + case ACPI_ADR_SPACE_FIXED_HARDWARE: > + break; > + } > + return AE_OK; > +} > + > /****************************************************************************** > * > * FUNCTION: acpi_os_validate_address > @@ -1357,6 +1440,7 @@ acpi_os_validate_address ( > char *name) > { > struct acpi_res_list *res; > + int added; > if (acpi_enforce_resources == ENFORCE_RESOURCES_NO) > return AE_OK; > > @@ -1374,14 +1458,17 @@ acpi_os_validate_address ( > res->end = address + length - 1; > res->resource_type = space_id; > spin_lock(&acpi_res_lock); > - list_add(&res->resource_list, &resource_list_head); > + added = acpi_res_list_add(res); > spin_unlock(&acpi_res_lock); > - pr_debug("Added %s resource: start: 0x%llx, end: 0x%llx, " > - "name: %s\n", (space_id == ACPI_ADR_SPACE_SYSTEM_IO) > + pr_debug("%s %s resource: start: 0x%llx, end: 0x%llx, " > + "name: %s\n", added ? "Added" : "Already exist", > + (space_id == ACPI_ADR_SPACE_SYSTEM_IO) > ? "SystemIO" : "System Memory", > (unsigned long long)res->start, > (unsigned long long)res->end, > res->name); > + if (!added) > + kfree(res); > break; > case ACPI_ADR_SPACE_PCI_CONFIG: > case ACPI_ADR_SPACE_EC: > diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h > index ab0b85c..14d40bf 100644 > --- a/include/acpi/acpiosxf.h > +++ b/include/acpi/acpiosxf.h > @@ -246,6 +246,10 @@ acpi_status > acpi_os_validate_address(u8 space_id, acpi_physical_address address, > acpi_size length, char *name); > > +acpi_status > +acpi_os_invalidate_address(u8 space_id, acpi_physical_address address, > + acpi_size length); > + > u64 acpi_os_get_timer(void); > > acpi_status acpi_os_signal(u32 function, void *info); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 02 July 2009 08:42:38 Moore, Robert wrote: > Before we start extending the existing mechanisms, I think we need to really > think about a better solution. Yes, but we still need a fix for older stable kernels which are the ones in production. Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-07-02 at 18:12 +0800, Thomas Renninger wrote: > On Thursday 02 July 2009 08:27:25 Lin Ming wrote: > > On Thu, 2009-07-02 at 10:03 +0800, Len Brown wrote: > > > I can't apply a patch that adds a known memory leak, > > > even if it removes a conflict between drivers. > > > > > > The reason is that there is a workaround for the driver conflict, > > > but no workaround for the memory leak. > > > > Here is a prototype simple patch, please review to see if it is the > > right way to go. > > > > 1) add a new field "count" to struct acpi_res_list. > > > > When inserting, if the region(addr, len) is already in the resource > > list, we just increase "count", otherwise, the region is inserted > > with count=1. > > > > When deleting, the "count" is decreased, if it's decreased to 0, > > the region is deleted from the resource list. > > > > With "count", the region with same address and length can only be > > inserted to the resource list once, so prevent potential memory leak. > Is the counting really needed? > An OpRegion inside a method should get deleted when the function is exited > and acpi_os_invalidate_address will get called. I was considering maybe some strange BIOS defines a region in a method with the same address and length of the global region, as below OperationRegion (reg1, SystemMemory, 0xFED11000, 0xFF) Method(m000) { OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF) .... } Although, this is a really seldom case, but with the counting it would be more safe. Lin Ming > > I like it. Thanks for looking into that! > > Thomas > > > > 2) add a new function acpi_os_invalidate_address, which is called when > > region is deleted. > > > > > > diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c > > index bc17103..96e26e7 100644 > > --- a/drivers/acpi/acpica/utdelete.c > > +++ b/drivers/acpi/acpica/utdelete.c > > @@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union > acpi_operand_object *object) > > ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS, > > "***** Region %p\n", object)); > > > > + /* Invalidate the region address/length via the host OS */ > > + > > + acpi_os_invalidate_address(object->region.space_id, > > + object->region.address, > > + (acpi_size) object->region.length); > > + > > second_desc = acpi_ns_get_secondary_object(object); > > if (second_desc) { > > /* > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > > index 7167071..620a65a 100644 > > --- a/drivers/acpi/osl.c > > +++ b/drivers/acpi/osl.c > > @@ -88,6 +88,7 @@ struct acpi_res_list { > > char name[5]; /* only can have a length of 4 chars, make use of this > > one instead of res->name, no need to kalloc then */ > > struct list_head resource_list; > > + int count; > > }; > > > > static LIST_HEAD(resource_list_head); > > @@ -1333,6 +1334,88 @@ acpi_os_validate_interface (char *interface) > > return AE_SUPPORT; > > } > > > > +static inline int acpi_res_list_add(struct acpi_res_list *res) > > +{ > > + struct acpi_res_list *res_list_elem; > > + > > + list_for_each_entry(res_list_elem, &resource_list_head, > > + resource_list) { > > + > > + if (res->resource_type == res_list_elem->resource_type && > > + res->start == res_list_elem->start && > > + res->end == res_list_elem->end) { > > + > > + /* > > + * The Region(addr,len) already exist in the list, > > + * just increase the count > > + */ > > + > > + res_list_elem->count++; > > + return 0; > > + } > > + } > > + > > + res->count = 1; > > + list_add(&res->resource_list, &resource_list_head); > > + return 1; > > +} > > + > > +static inline void acpi_res_list_del(struct acpi_res_list *res) > > +{ > > + struct acpi_res_list *res_list_elem; > > + > > + list_for_each_entry(res_list_elem, &resource_list_head, > > + resource_list) { > > + > > + if (res->resource_type == res_list_elem->resource_type && > > + res->start == res_list_elem->start && > > + res->end == res_list_elem->end) { > > + > > + /* > > + * If the res count is decreased to 0, remove and free it > > + */ > > + > > + if (--res_list_elem->count == 0) { > > + list_del(&res_list_elem->resource_list); > > + kfree(res_list_elem); > > + } > > + return; > > + } > > + } > > +} > > + > > +acpi_status > > +acpi_os_invalidate_address( > > + u8 space_id, > > + acpi_physical_address address, > > + acpi_size length) > > +{ > > + struct acpi_res_list res; > > + > > + switch (space_id) { > > + case ACPI_ADR_SPACE_SYSTEM_IO: > > + case ACPI_ADR_SPACE_SYSTEM_MEMORY: > > + /* Only interference checks against SystemIO and SytemMemory > > + are needed */ > > + res.start = address; > > + res.end = address + length - 1; > > + res.resource_type = space_id; > > + spin_lock(&acpi_res_lock); > > + acpi_res_list_del(&res); > > + spin_unlock(&acpi_res_lock); > > + break; > > + case ACPI_ADR_SPACE_PCI_CONFIG: > > + case ACPI_ADR_SPACE_EC: > > + case ACPI_ADR_SPACE_SMBUS: > > + case ACPI_ADR_SPACE_CMOS: > > + case ACPI_ADR_SPACE_PCI_BAR_TARGET: > > + case ACPI_ADR_SPACE_DATA_TABLE: > > + case ACPI_ADR_SPACE_FIXED_HARDWARE: > > + break; > > + } > > + return AE_OK; > > +} > > + > > > /****************************************************************************** > > * > > * FUNCTION: acpi_os_validate_address > > @@ -1357,6 +1440,7 @@ acpi_os_validate_address ( > > char *name) > > { > > struct acpi_res_list *res; > > + int added; > > if (acpi_enforce_resources == ENFORCE_RESOURCES_NO) > > return AE_OK; > > > > @@ -1374,14 +1458,17 @@ acpi_os_validate_address ( > > res->end = address + length - 1; > > res->resource_type = space_id; > > spin_lock(&acpi_res_lock); > > - list_add(&res->resource_list, &resource_list_head); > > + added = acpi_res_list_add(res); > > spin_unlock(&acpi_res_lock); > > - pr_debug("Added %s resource: start: 0x%llx, end: 0x%llx, " > > - "name: %s\n", (space_id == ACPI_ADR_SPACE_SYSTEM_IO) > > + pr_debug("%s %s resource: start: 0x%llx, end: 0x%llx, " > > + "name: %s\n", added ? "Added" : "Already exist", > > + (space_id == ACPI_ADR_SPACE_SYSTEM_IO) > > ? "SystemIO" : "System Memory", > > (unsigned long long)res->start, > > (unsigned long long)res->end, > > res->name); > > + if (!added) > > + kfree(res); > > break; > > case ACPI_ADR_SPACE_PCI_CONFIG: > > case ACPI_ADR_SPACE_EC: > > diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h > > index ab0b85c..14d40bf 100644 > > --- a/include/acpi/acpiosxf.h > > +++ b/include/acpi/acpiosxf.h > > @@ -246,6 +246,10 @@ acpi_status > > acpi_os_validate_address(u8 space_id, acpi_physical_address address, > > acpi_size length, char *name); > > > > +acpi_status > > +acpi_os_invalidate_address(u8 space_id, acpi_physical_address address, > > + acpi_size length); > > + > > u64 acpi_os_get_timer(void); > > > > acpi_status acpi_os_signal(u32 function, void *info); > > > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c index bc17103..96e26e7 100644 --- a/drivers/acpi/acpica/utdelete.c +++ b/drivers/acpi/acpica/utdelete.c @@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union acpi_operand_object *object) ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS, "***** Region %p\n", object)); + /* Invalidate the region address/length via the host OS */ + + acpi_os_invalidate_address(object->region.space_id, + object->region.address, + (acpi_size) object->region.length); + second_desc = acpi_ns_get_secondary_object(object); if (second_desc) { /* diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 7167071..620a65a 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -88,6 +88,7 @@ struct acpi_res_list { char name[5]; /* only can have a length of 4 chars, make use of this one instead of res->name, no need to kalloc then */ struct list_head resource_list; + int count; }; static LIST_HEAD(resource_list_head); @@ -1333,6 +1334,88 @@ acpi_os_validate_interface (char *interface) return AE_SUPPORT; } +static inline int acpi_res_list_add(struct acpi_res_list *res) +{ + struct acpi_res_list *res_list_elem; + + list_for_each_entry(res_list_elem, &resource_list_head, + resource_list) { + + if (res->resource_type == res_list_elem->resource_type && + res->start == res_list_elem->start && + res->end == res_list_elem->end) { + + /* + * The Region(addr,len) already exist in the list, + * just increase the count + */ + + res_list_elem->count++; + return 0; + } + } + + res->count = 1; + list_add(&res->resource_list, &resource_list_head); + return 1; +} + +static inline void acpi_res_list_del(struct acpi_res_list *res) +{ + struct acpi_res_list *res_list_elem; + + list_for_each_entry(res_list_elem, &resource_list_head, + resource_list) { + + if (res->resource_type == res_list_elem->resource_type && + res->start == res_list_elem->start && + res->end == res_list_elem->end) { + + /* + * If the res count is decreased to 0, remove and free it + */ + + if (--res_list_elem->count == 0) { + list_del(&res_list_elem->resource_list); + kfree(res_list_elem); + } + return; + } + } +} + +acpi_status +acpi_os_invalidate_address( + u8 space_id, + acpi_physical_address address, + acpi_size length) +{ + struct acpi_res_list res; + + switch (space_id) { + case ACPI_ADR_SPACE_SYSTEM_IO: + case ACPI_ADR_SPACE_SYSTEM_MEMORY: + /* Only interference checks against SystemIO and SytemMemory + are needed */ + res.start = address; + res.end = address + length - 1; + res.resource_type = space_id; + spin_lock(&acpi_res_lock); + acpi_res_list_del(&res); + spin_unlock(&acpi_res_lock); + break; + case ACPI_ADR_SPACE_PCI_CONFIG: + case ACPI_ADR_SPACE_EC: + case ACPI_ADR_SPACE_SMBUS: + case ACPI_ADR_SPACE_CMOS: + case ACPI_ADR_SPACE_PCI_BAR_TARGET: + case ACPI_ADR_SPACE_DATA_TABLE: + case ACPI_ADR_SPACE_FIXED_HARDWARE: + break; + } + return AE_OK; +} + /****************************************************************************** * * FUNCTION: acpi_os_validate_address @@ -1357,6 +1440,7 @@ acpi_os_validate_address ( char *name) { struct acpi_res_list *res; + int added; if (acpi_enforce_resources == ENFORCE_RESOURCES_NO) return AE_OK; @@ -1374,14 +1458,17 @@ acpi_os_validate_address ( res->end = address + length - 1; res->resource_type = space_id; spin_lock(&acpi_res_lock); - list_add(&res->resource_list, &resource_list_head); + added = acpi_res_list_add(res); spin_unlock(&acpi_res_lock); - pr_debug("Added %s resource: start: 0x%llx, end: 0x%llx, " - "name: %s\n", (space_id == ACPI_ADR_SPACE_SYSTEM_IO) + pr_debug("%s %s resource: start: 0x%llx, end: 0x%llx, " + "name: %s\n", added ? "Added" : "Already exist", + (space_id == ACPI_ADR_SPACE_SYSTEM_IO) ? "SystemIO" : "System Memory", (unsigned long long)res->start, (unsigned long long)res->end, res->name); + if (!added) + kfree(res); break; case ACPI_ADR_SPACE_PCI_CONFIG: case ACPI_ADR_SPACE_EC: diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h index ab0b85c..14d40bf 100644 --- a/include/acpi/acpiosxf.h +++ b/include/acpi/acpiosxf.h @@ -246,6 +246,10 @@ acpi_status acpi_os_validate_address(u8 space_id, acpi_physical_address address, acpi_size length, char *name); +acpi_status +acpi_os_invalidate_address(u8 space_id, acpi_physical_address address, + acpi_size length); + u64 acpi_os_get_timer(void); acpi_status acpi_os_signal(u32 function, void *info);