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

From: Lin Ming <ming.m.lin@intel.com>
To: Thomas Renninger <trenn@suse.de>
Cc: Len Brown <lenb@kernel.org>,
	"Moore, Robert" <robert.moore@intel.com>,
	"Zhao, Yakui" <yakui.zhao@intel.com>,
	linux-acpi <linux-acpi@vger.kernel.org>,
	"khali@linux-fr.org" <khali@linux-fr.org>,
	"alan-jenkins@tuffmail.co.uk" <alan-jenkins@tuffmail.co.uk>,
	"fboiteux@calistel.com" <fboiteux@calistel.com>
Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"
Date: Tue, 14 Jul 2009 10:28:56 +0800	[thread overview]
Message-ID: <1247538536.3850.25.camel@minggr.sh.intel.com> (raw)
In-Reply-To: <200907131736.42225.trenn@suse.de>

On Mon, 2009-07-13 at 23:36 +0800, Thomas Renninger wrote:
> 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"

Hi, Thomas,

Those are acpica private header files that should not be used out of
acpica.

See commit e2f7a7772880458edff1b1cc5a988947229fac26
"ACPICA: hide private headers"

> +
>  #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;

Same as above, acpi_namespace_node is an ACPICA internal used only
structure.

>  
> -	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);

Same as above, acpi_ns_map_handle_to_node is ACPICA internal used only
function. 

Try acpi_get_object_info(...) to get node type.

> +	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);

Same as above, acpi_ns_get_attached_object is ACPICA internal used only.

Thanks,
Lin Ming

> +	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;
>  }
>  
> 