Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"

[Thomas Renninger <trenn@suse.de>]

Hi Lin and all others...

this is about:
http://bugzilla.kernel.org/show_bug.cgi?id=13620

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.

This should provide the same functionality as we had before 2.6.30,
but goes the approach Bob suggested:
If a driver wants to check for ACPI OpRegion conflicts, walk the
namespace and check for conflicts against "active/defined" OpRegions.
It would be great if someone could comment on the:
handle -> struct acpi_object_region mapping
at the beginning of the walk_namespace callback function:
acpi_check_resource_conflict_wn(..)
This should be the most critical part.

On a conflict you should see rather the same output (from my little
test module):
ACPI: I/O resource sis5595 [0x80-0x85] conflicts with ACPI region DB80 [0x80-0x81]

This is more intrusive than Lin's approach (remove entries from the
OpRegion osl.c list when destroyed).
Not sure what is better (this should go into stable later...).
Let me know whether this is acceptable and I can write a more
detailed changelog. I can also rebase the patch then (this one is
against a 2.6.29 kernel).

Thanks,

   Thomas

-------
ACPI: Bring back resource conflict checking for hwmon drivers vs ACPI opregions


Signed-off-by: Thomas Renninger <trenn@suse.de>

---
 drivers/acpi/acpica/aclocal.h  |    2 
 drivers/acpi/acpica/acnamesp.h |    2 
 drivers/acpi/acpica/acobject.h |    2 
 drivers/acpi/osl.c             |  170 ++++++++++++++++++-----------------------
 4 files changed, 84 insertions(+), 92 deletions(-)

Index: linux-2.6.29/drivers/acpi/acpica/aclocal.h
===================================================================
--- linux-2.6.29.orig/drivers/acpi/acpica/aclocal.h
+++ linux-2.6.29/drivers/acpi/acpica/aclocal.h
@@ -44,6 +44,8 @@
 #ifndef __ACLOCAL_H__
 #define __ACLOCAL_H__
 
+#include "acconfig.h"
+
 /* acpisrc:struct_defs -- for acpisrc conversion */
 
 #define ACPI_SERIALIZED                 0xFF
Index: linux-2.6.29/drivers/acpi/acpica/acnamesp.h
===================================================================
--- linux-2.6.29.orig/drivers/acpi/acpica/acnamesp.h
+++ linux-2.6.29/drivers/acpi/acpica/acnamesp.h
@@ -44,6 +44,8 @@
 #ifndef __ACNAMESP_H__
 #define __ACNAMESP_H__
 
+#include "acstruct.h"
+
 /* To search the entire name space, pass this as search_base */
 
 #define ACPI_NS_ALL                 ((acpi_handle)0)
Index: linux-2.6.29/drivers/acpi/acpica/acobject.h
===================================================================
--- linux-2.6.29.orig/drivers/acpi/acpica/acobject.h
+++ linux-2.6.29/drivers/acpi/acpica/acobject.h
@@ -45,6 +45,8 @@
 #ifndef _ACOBJECT_H
 #define _ACOBJECT_H
 
+#include "aclocal.h"
+
 /* acpisrc:struct_defs -- for acpisrc conversion */
 
 /*
Index: linux-2.6.29/drivers/acpi/osl.c
===================================================================
--- linux-2.6.29.orig/drivers/acpi/osl.c
+++ linux-2.6.29/drivers/acpi/osl.c
@@ -51,6 +51,10 @@
 #include <acpi/acpi_bus.h>
 #include <acpi/processor.h>
 
+#include "acpica/acobject.h"
+#include "acpica/acnamesp.h"
+#include "acpica/acutils.h"
+
 #define _COMPONENT		ACPI_OS_SERVICES
 ACPI_MODULE_NAME("osl");
 #define PREFIX		"ACPI: "
@@ -80,18 +84,6 @@ static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
 
-struct acpi_res_list {
-	resource_size_t start;
-	resource_size_t end;
-	acpi_adr_space_type resource_type; /* IO port, System memory, ...*/
-	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;
-};
-
-static LIST_HEAD(resource_list_head);
-static DEFINE_SPINLOCK(acpi_res_lock);
-
 #define	OSI_STRING_LENGTH_MAX 64	/* arbitrary */
 static char osi_additional_string[OSI_STRING_LENGTH_MAX];
 
@@ -1096,57 +1088,87 @@ static int __init acpi_enforce_resources
 
 __setup("acpi_enforce_resources=", acpi_enforce_resources_setup);
 
-/* Check for resource conflicts between ACPI OperationRegions and native
- * drivers */
-int acpi_check_resource_conflict(struct resource *res)
+static acpi_status
+acpi_check_resource_conflict_wn(acpi_handle handle, u32 level, void *context,
+			     void **retval)
 {
-	struct acpi_res_list *res_list_elem;
-	int ioport;
-	int clash = 0;
 
-	if (acpi_enforce_resources == ENFORCE_RESOURCES_NO)
-		return 0;
-	if (!(res->flags & IORESOURCE_IO) && !(res->flags & IORESOURCE_MEM))
-		return 0;
+	struct acpi_namespace_node *node;
 
-	ioport = res->flags & IORESOURCE_IO;
+	struct resource *res = context;
+	struct acpi_object_region *region_desc;
+	unsigned long long reg_start;
+	unsigned long long reg_end;
 
-	spin_lock(&acpi_res_lock);
-	list_for_each_entry(res_list_elem, &resource_list_head,
-			    resource_list) {
-		if (ioport && (res_list_elem->resource_type
-			       != ACPI_ADR_SPACE_SYSTEM_IO))
-			continue;
-		if (!ioport && (res_list_elem->resource_type
-				!= ACPI_ADR_SPACE_SYSTEM_MEMORY))
-			continue;
+	/* Convert and validate the device handle */
+	node = acpi_ns_map_handle_to_node(handle);
+	if (!node || node->type != ACPI_TYPE_REGION)
+		return AE_OK;
 
-		if (res->end < res_list_elem->start
-		    || res_list_elem->end < res->start)
-			continue;
-		clash = 1;
-		break;
-	}
-	spin_unlock(&acpi_res_lock);
+	/* Check for an existing internal object */
+	region_desc = (struct acpi_object_region *)
+		acpi_ns_get_attached_object(node);
+	if (!region_desc || region_desc->type != ACPI_TYPE_REGION)
+		return AE_OK;
 
-	if (clash) {
-		if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) {
-			printk("%sACPI: %s resource %s [0x%llx-0x%llx]"
-			       " conflicts with ACPI region %s"
-			       " [0x%llx-0x%llx]\n",
-			       acpi_enforce_resources == ENFORCE_RESOURCES_LAX
-			       ? KERN_WARNING : KERN_ERR,
-			       ioport ? "I/O" : "Memory", res->name,
-			       (long long) res->start, (long long) res->end,
-			       res_list_elem->name,
-			       (long long) res_list_elem->start,
-			       (long long) res_list_elem->end);
-			printk(KERN_INFO "ACPI: Device needs an ACPI driver\n");
-		}
-		if (acpi_enforce_resources == ENFORCE_RESOURCES_STRICT)
-			return -EBUSY;
+	if (region_desc->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY
+	    && region_desc->space_id != ACPI_ADR_SPACE_SYSTEM_IO)
+		return AE_OK;
+
+	/* Only check IO vs IO and Mem vs Mem regions/resources */
+	if ((region_desc->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+	     !(res->flags & IORESOURCE_MEM)) ||
+	    (region_desc->space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
+	     !(res->flags & IORESOURCE_IO)))
+		return AE_OK;
+
+	reg_start = region_desc->address;
+	reg_end   = region_desc->address + region_desc->length;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION, "Checking %s Region: ACPI:"
+			  " 0x%llX - 0x%llX\n",
+			  region_desc->space_id == ACPI_ADR_SPACE_SYSTEM_IO ?
+			  "IO" : "Memory", reg_start, reg_end));
+
+	if (res->end < reg_start || reg_end < res->start)
+		return AE_OK;
+
+	/* Conflict! */
+	if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) {
+		printk("%sACPI: %s resource %s [0x%llx-0x%llx] conflicts with "
+		       "ACPI region %s [0x%llx-0x%llx]\n",
+		       acpi_enforce_resources == ENFORCE_RESOURCES_LAX
+		       ? KERN_WARNING : KERN_ERR,
+		       region_desc->space_id == ACPI_ADR_SPACE_SYSTEM_IO
+		       ? "I/O" : "Memory", res->name,
+		       (long long) res->start, (long long) res->end,
+		       acpi_ut_get_node_name(node),
+		       reg_start,
+		       reg_end);
+		printk(KERN_INFO "ACPI: Device needs an ACPI driver\n");
 	}
-	return 0;
+	if (acpi_enforce_resources == ENFORCE_RESOURCES_STRICT)
+		return AE_ERROR;
+
+	return AE_OK;
+}
+
+/* Check for resource conflicts between ACPI OperationRegions and native
+ * drivers
+ * Returns zero if there is no conflict
+ */
+int acpi_check_resource_conflict(struct resource *res)
+{
+	acpi_status status;
+
+	status = acpi_walk_namespace(ACPI_TYPE_REGION, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX,
+				     acpi_check_resource_conflict_wn,
+				     res, NULL);
+	if (status == AE_ERROR)
+		return 1;
+	else
+		return 0;
 }
 EXPORT_SYMBOL(acpi_check_resource_conflict);
 
@@ -1340,42 +1362,6 @@ acpi_os_validate_address (
     acpi_size               length,
     char *name)
 {
-	struct acpi_res_list *res;
-	if (acpi_enforce_resources == ENFORCE_RESOURCES_NO)
-		return AE_OK;
-
-	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 = kzalloc(sizeof(struct acpi_res_list), GFP_KERNEL);
-		if (!res)
-			return AE_OK;
-		/* ACPI names are fixed to 4 bytes, still better use strlcpy */
-		strlcpy(res->name, name, 5);
-		res->start = address;
-		res->end = address + length - 1;
-		res->resource_type = space_id;
-		spin_lock(&acpi_res_lock);
-		list_add(&res->resource_list, &resource_list_head);
-		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)
-			 ? "SystemIO" : "System Memory",
-			 (unsigned long long)res->start,
-			 (unsigned long long)res->end,
-			 res->name);
-		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;
 }