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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
