* [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] @ 2011-11-28 20:53 Luca Tettamanti 2011-11-28 22:56 ` Moore, Robert 0 siblings, 1 reply; 13+ messages in thread From: Luca Tettamanti @ 2011-11-28 20:53 UTC (permalink / raw) To: Jean Delvare; +Cc: Nikolay Mikov, Thomas, Bob Moore, linux-acpi To give some context to the newcomers: while helping a user with a new hwmon chip we discovered that the native driver is loaded fine even though the IO ports are claimed by ACPI (OperationRegion) and acpi_enforce_resources is "strict". On Mon, Nov 28, 2011 at 4:48 PM, Luca Tettamanti <kronos.it@gmail.com> wrote: > On Mon, Nov 28, 2011 at 4:08 PM, Jean Delvare <khali@linux-fr.org> wrote: >> On Mon, 28 Nov 2011 14:02:51 +0100, Luca Tettamanti wrote: >>> The DSDT has this: >>> >>> Name (IOEB, 0x0290) >>> OperationRegion (SIOE, SystemIO, IOEB, 0x20) >>> Field (SIOE, ByteAcc, NoLock, Preserve) >>> { >>> Offset (0x05), >>> GIDX, 8, >>> GDAT, 8 >>> } >>> >>> so the requested area is well within what ACPI claims. [cut] >> A colleague of mine (hi Thomas!) suggests that Nikolay could boot with: >> >> ddebug="file osl.c +p > > This should be: > > ddebug_query="file osl.c +p" > > It should spam the log with all the resources found by ACPI parser, > assuming that CONFIG_DYNAMIC_DEBUG is enabled. Hum, the call to acpi_os_validate_address was removed between 2.6.38 and 2.6.39, with this commit by Bob More: commit 9ad19ac456a5f097f7cbbfef820b95297d6a934f Author: Bob Moore <robert.moore@intel.com> Date: Mon Feb 14 16:09:40 2011 +0800 ACPICA: Split large dsopcode and dsload.c files. which seems to be just moving code around (the culprit here is acpi_ds_get_region_arguments). I guess that the removal wasn't intentional since the corresponding call to acpi_os_invalidate_address was left in place. Bob should I put the call back in place? Am I missing something? Luca -- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-11-28 20:53 [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] Luca Tettamanti @ 2011-11-28 22:56 ` Moore, Robert 2011-11-29 1:48 ` Lin Ming 0 siblings, 1 reply; 13+ messages in thread From: Moore, Robert @ 2011-11-28 22:56 UTC (permalink / raw) To: Luca Tettamanti, Jean Delvare, Lin, Ming M Cc: Nikolay Mikov, Thomas, linux-acpi@vger.kernel.org I'm going to let Lin Ming respond to this, since the functions in question appear to be Linux extensions to ACPICA. > -----Original Message----- > From: Luca Tettamanti [mailto:kronos.it@gmail.com] > Sent: Monday, November 28, 2011 12:54 PM > To: Jean Delvare > Cc: Nikolay Mikov; Thomas; Moore, Robert; linux-acpi@vger.kernel.org > Subject: [BUG] ACPI resource validation not working [Was: Re: ITE > 8728F] > > To give some context to the newcomers: while helping a user with a new > hwmon chip we discovered that the native driver is loaded fine even > though the IO ports are claimed by ACPI (OperationRegion) and > acpi_enforce_resources is "strict". > > On Mon, Nov 28, 2011 at 4:48 PM, Luca Tettamanti <kronos.it@gmail.com> > wrote: > > On Mon, Nov 28, 2011 at 4:08 PM, Jean Delvare <khali@linux-fr.org> > wrote: > >> On Mon, 28 Nov 2011 14:02:51 +0100, Luca Tettamanti wrote: > >>> The DSDT has this: > >>> > >>> Name (IOEB, 0x0290) > >>> OperationRegion (SIOE, SystemIO, IOEB, 0x20) > >>> Field (SIOE, ByteAcc, NoLock, Preserve) > >>> { > >>> Offset (0x05), > >>> GIDX, 8, > >>> GDAT, 8 > >>> } > >>> > >>> so the requested area is well within what ACPI claims. > [cut] > >> A colleague of mine (hi Thomas!) suggests that Nikolay could boot > with: > >> > >> ddebug="file osl.c +p > > > > This should be: > > > > ddebug_query="file osl.c +p" > > > > It should spam the log with all the resources found by ACPI parser, > > assuming that CONFIG_DYNAMIC_DEBUG is enabled. > > Hum, the call to acpi_os_validate_address was removed between 2.6.38 > and 2.6.39, with this commit by Bob More: > > commit 9ad19ac456a5f097f7cbbfef820b95297d6a934f > Author: Bob Moore <robert.moore@intel.com> > Date: Mon Feb 14 16:09:40 2011 +0800 > > ACPICA: Split large dsopcode and dsload.c files. > > which seems to be just moving code around (the culprit here is > acpi_ds_get_region_arguments). I guess that the removal wasn't > intentional since the corresponding call to acpi_os_invalidate_address > was left in place. > Bob should I put the call back in place? Am I missing something? > > Luca ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-11-28 22:56 ` Moore, Robert @ 2011-11-29 1:48 ` Lin Ming 2011-11-29 13:18 ` Luca Tettamanti 0 siblings, 1 reply; 13+ messages in thread From: Lin Ming @ 2011-11-29 1:48 UTC (permalink / raw) To: Moore, Robert Cc: Luca Tettamanti, Jean Delvare, Nikolay Mikov, Thomas, linux-acpi@vger.kernel.org On Tue, 2011-11-29 at 06:56 +0800, Moore, Robert wrote: > I'm going to let Lin Ming respond to this, since the functions in question appear to be Linux extensions to ACPICA. > > > -----Original Message----- > > From: Luca Tettamanti [mailto:kronos.it@gmail.com] > > Sent: Monday, November 28, 2011 12:54 PM > > To: Jean Delvare > > Cc: Nikolay Mikov; Thomas; Moore, Robert; linux-acpi@vger.kernel.org > > Subject: [BUG] ACPI resource validation not working [Was: Re: ITE > > 8728F] > > > > To give some context to the newcomers: while helping a user with a new > > hwmon chip we discovered that the native driver is loaded fine even > > though the IO ports are claimed by ACPI (OperationRegion) and > > acpi_enforce_resources is "strict". > > > > On Mon, Nov 28, 2011 at 4:48 PM, Luca Tettamanti <kronos.it@gmail.com> > > wrote: > > > On Mon, Nov 28, 2011 at 4:08 PM, Jean Delvare <khali@linux-fr.org> > > wrote: > > >> On Mon, 28 Nov 2011 14:02:51 +0100, Luca Tettamanti wrote: > > >>> The DSDT has this: > > >>> > > >>> Name (IOEB, 0x0290) > > >>> OperationRegion (SIOE, SystemIO, IOEB, 0x20) > > >>> Field (SIOE, ByteAcc, NoLock, Preserve) > > >>> { > > >>> Offset (0x05), > > >>> GIDX, 8, > > >>> GDAT, 8 > > >>> } > > >>> > > >>> so the requested area is well within what ACPI claims. > > [cut] > > >> A colleague of mine (hi Thomas!) suggests that Nikolay could boot > > with: > > >> > > >> ddebug="file osl.c +p > > > > > > This should be: > > > > > > ddebug_query="file osl.c +p" > > > > > > It should spam the log with all the resources found by ACPI parser, > > > assuming that CONFIG_DYNAMIC_DEBUG is enabled. > > > > Hum, the call to acpi_os_validate_address was removed between 2.6.38 > > and 2.6.39, with this commit by Bob More: > > > > commit 9ad19ac456a5f097f7cbbfef820b95297d6a934f > > Author: Bob Moore <robert.moore@intel.com> > > Date: Mon Feb 14 16:09:40 2011 +0800 > > > > ACPICA: Split large dsopcode and dsload.c files. > > > > which seems to be just moving code around (the culprit here is > > acpi_ds_get_region_arguments). I guess that the removal wasn't > > intentional since the corresponding call to acpi_os_invalidate_address > > was left in place. > > Bob should I put the call back in place? Am I missing something? Yes, we should put the call back. diff --git a/drivers/acpi/acpica/dsargs.c b/drivers/acpi/acpica/dsargs.c index 8c7b997..42163d8 100644 --- a/drivers/acpi/acpica/dsargs.c +++ b/drivers/acpi/acpica/dsargs.c @@ -387,5 +387,29 @@ acpi_status acpi_ds_get_region_arguments(union acpi_operand_object *obj_desc) status = acpi_ds_execute_arguments(node, node->parent, extra_desc->extra.aml_length, extra_desc->extra.aml_start); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Validate the region address/length via the host OS */ + + status = acpi_os_validate_address(obj_desc->region.space_id, + obj_desc->region.address, + (acpi_size) obj_desc->region.length, + acpi_ut_get_node_name(node)); + + if (ACPI_FAILURE(status)) { + /* + * Invalid address/length. We will emit an error message and mark + * the region as invalid, so that it will cause an additional error if + * it is ever used. Then return AE_OK. + */ + ACPI_EXCEPTION((AE_INFO, status, + "During address validation of OpRegion [%4.4s]", + node->name.ascii)); + obj_desc->common.flags |= AOPOBJ_INVALID; + status = AE_OK; + } + return_ACPI_STATUS(status); } > > > > Luca ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-11-29 1:48 ` Lin Ming @ 2011-11-29 13:18 ` Luca Tettamanti 2011-11-29 13:44 ` Lin Ming 2011-12-09 2:01 ` Lin Ming 0 siblings, 2 replies; 13+ messages in thread From: Luca Tettamanti @ 2011-11-29 13:18 UTC (permalink / raw) To: Lin Ming Cc: Moore, Robert, Jean Delvare, Nikolay Mikov, Thomas, linux-acpi@vger.kernel.org On Tue, Nov 29, 2011 at 2:48 AM, Lin Ming <ming.m.lin@intel.com> wrote: > On Tue, 2011-11-29 at 06:56 +0800, Moore, Robert wrote: >> I'm going to let Lin Ming respond to this, since the functions in question appear to be Linux extensions to ACPICA. [...] > Yes, we should put the call back. Great, are you going to push this patch yourself? > diff --git a/drivers/acpi/acpica/dsargs.c b/drivers/acpi/acpica/dsargs.c > index 8c7b997..42163d8 100644 > --- a/drivers/acpi/acpica/dsargs.c > +++ b/drivers/acpi/acpica/dsargs.c > @@ -387,5 +387,29 @@ acpi_status acpi_ds_get_region_arguments(union acpi_operand_object *obj_desc) > status = acpi_ds_execute_arguments(node, node->parent, > extra_desc->extra.aml_length, > extra_desc->extra.aml_start); > + if (ACPI_FAILURE(status)) { > + return_ACPI_STATUS(status); > + } > + > + /* Validate the region address/length via the host OS */ > + > + status = acpi_os_validate_address(obj_desc->region.space_id, > + obj_desc->region.address, > + (acpi_size) obj_desc->region.length, > + acpi_ut_get_node_name(node)); > + > + if (ACPI_FAILURE(status)) { > + /* > + * Invalid address/length. We will emit an error message and mark > + * the region as invalid, so that it will cause an additional error if > + * it is ever used. Then return AE_OK. > + */ > + ACPI_EXCEPTION((AE_INFO, status, > + "During address validation of OpRegion [%4.4s]", > + node->name.ascii)); > + obj_desc->common.flags |= AOPOBJ_INVALID; > + status = AE_OK; > + } > + > return_ACPI_STATUS(status); > } thanks, Luca ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-11-29 13:18 ` Luca Tettamanti @ 2011-11-29 13:44 ` Lin Ming 2011-11-30 9:07 ` Jean Delvare 2011-12-09 2:01 ` Lin Ming 1 sibling, 1 reply; 13+ messages in thread From: Lin Ming @ 2011-11-29 13:44 UTC (permalink / raw) To: Luca Tettamanti Cc: Moore, Robert, Jean Delvare, Nikolay Mikov, Thomas, linux-acpi@vger.kernel.org On Tue, Nov 29, 2011 at 9:18 PM, Luca Tettamanti <kronos.it@gmail.com> wrote: > On Tue, Nov 29, 2011 at 2:48 AM, Lin Ming <ming.m.lin@intel.com> wrote: >> On Tue, 2011-11-29 at 06:56 +0800, Moore, Robert wrote: >>> I'm going to let Lin Ming respond to this, since the functions in question appear to be Linux extensions to ACPICA. > [...] >> Yes, we should put the call back. > > Great, are you going to push this patch yourself? Yes. I'll send it to Len. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-11-29 13:44 ` Lin Ming @ 2011-11-30 9:07 ` Jean Delvare 0 siblings, 0 replies; 13+ messages in thread From: Jean Delvare @ 2011-11-30 9:07 UTC (permalink / raw) To: Lin Ming Cc: Luca Tettamanti, Moore, Robert, Nikolay Mikov, Thomas, linux-acpi On Tue, 29 Nov 2011 21:44:08 +0800, Lin Ming wrote: > On Tue, Nov 29, 2011 at 9:18 PM, Luca Tettamanti <kronos.it@gmail.com> wrote: > > On Tue, Nov 29, 2011 at 2:48 AM, Lin Ming <ming.m.lin@intel.com> wrote: > >> Yes, we should put the call back. > > > > Great, are you going to push this patch yourself? > > Yes. I'll send it to Len. Thanks. Please make sure it goes to stable@kernel.org too, as this regression will cause trouble for many lm-sensors users. -- Jean Delvare ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-11-29 13:18 ` Luca Tettamanti 2011-11-29 13:44 ` Lin Ming @ 2011-12-09 2:01 ` Lin Ming 2011-12-12 9:37 ` Jean Delvare 2011-12-12 12:52 ` Luca Tettamanti 1 sibling, 2 replies; 13+ messages in thread From: Lin Ming @ 2011-12-09 2:01 UTC (permalink / raw) To: Luca Tettamanti Cc: Moore, Robert, Jean Delvare, Nikolay Mikov, Thomas, linux-acpi@vger.kernel.org, lenb On Tue, 2011-11-29 at 21:18 +0800, Luca Tettamanti wrote: > On Tue, Nov 29, 2011 at 2:48 AM, Lin Ming <ming.m.lin@intel.com> wrote: > > On Tue, 2011-11-29 at 06:56 +0800, Moore, Robert wrote: > >> I'm going to let Lin Ming respond to this, since the functions in question appear to be Linux extensions to ACPICA. > [...] > > Yes, we should put the call back. > > Great, are you going to push this patch yourself? Hi Luca, We have investigated this issue more and think that it's better to move the resource validation code into ACPICA core. Here is the new patch. The major changes include: - Remove acpi_os_validate_address/acpi_os_invalidate_address from osl.c. They are reimplemented in ACPICA core: acpi_ut_add_address_range/acpi_ut_remove_address_range. - Add new interface for drivers to check resource conflict: acpi_check_address_range Could you help to test it? Thanks, Lin Ming --- drivers/acpi/acpica/Makefile | 2 +- drivers/acpi/acpica/acconfig.h | 4 + drivers/acpi/acpica/acglobal.h | 2 + drivers/acpi/acpica/aclocal.h | 9 ++ drivers/acpi/acpica/acutils.h | 18 +++ drivers/acpi/acpica/dsargs.c | 7 + drivers/acpi/acpica/utaddress.c | 295 +++++++++++++++++++++++++++++++++++++++ drivers/acpi/acpica/utdelete.c | 13 +- drivers/acpi/acpica/utglobal.c | 6 + drivers/acpi/acpica/utinit.c | 1 + drivers/acpi/acpica/utxface.c | 38 +++++ drivers/acpi/osl.c | 202 ++------------------------- include/acpi/acpixf.h | 5 + 13 files changed, 406 insertions(+), 196 deletions(-) diff --git a/drivers/acpi/acpica/Makefile b/drivers/acpi/acpica/Makefile index 301bd2d..855785b 100644 --- a/drivers/acpi/acpica/Makefile +++ b/drivers/acpi/acpica/Makefile @@ -45,4 +45,4 @@ acpi-y += tbxface.o tbinstal.o tbutils.o tbfind.o tbfadt.o tbxfroot.o acpi-y += utalloc.o utdebug.o uteval.o utinit.o utmisc.o utxface.o \ utcopy.o utdelete.o utglobal.o utmath.o utobject.o \ utstate.o utmutex.o utobject.o utresrc.o utlock.o utids.o \ - utosi.o utxferror.o utdecode.o + utosi.o utxferror.o utdecode.o utaddress.o diff --git a/drivers/acpi/acpica/acconfig.h b/drivers/acpi/acpica/acconfig.h index f895a24..4dbd010 100644 --- a/drivers/acpi/acpica/acconfig.h +++ b/drivers/acpi/acpica/acconfig.h @@ -123,6 +123,10 @@ #define ACPI_MAX_SLEEP 2000 /* Two seconds */ +/* Address Range lists are per-space_id (Memory and I/O only) */ + +#define ACPI_ADDRESS_RANGE_MAX 2 + /****************************************************************************** * * ACPI Specification constants (Do not change unless the specification changes) diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h index 76dc02f..b7e7fbd 100644 --- a/drivers/acpi/acpica/acglobal.h +++ b/drivers/acpi/acpica/acglobal.h @@ -295,6 +295,8 @@ ACPI_EXTERN u8 acpi_gbl_acpi_hardware_present; ACPI_EXTERN u8 acpi_gbl_events_initialized; ACPI_EXTERN u8 acpi_gbl_osi_data; ACPI_EXTERN struct acpi_interface_info *acpi_gbl_supported_interfaces; +ACPI_EXTERN struct acpi_address_range + *acpi_gbl_address_range_list[ACPI_ADDRESS_RANGE_MAX]; #ifndef DEFINE_ACPI_GLOBALS diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h index 5552125..44a2164 100644 --- a/drivers/acpi/acpica/aclocal.h +++ b/drivers/acpi/acpica/aclocal.h @@ -625,6 +625,15 @@ union acpi_generic_state { typedef acpi_status(*ACPI_EXECUTE_OP) (struct acpi_walk_state * walk_state); +/* Address Range info block */ + +struct acpi_address_range { + struct acpi_address_range *next; + struct acpi_namespace_node *region_node; + acpi_physical_address start_address; + acpi_physical_address end_address; +}; + /***************************************************************************** * * Parser typedefs and structs diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h index 99c140d..dfea11d 100644 --- a/drivers/acpi/acpica/acutils.h +++ b/drivers/acpi/acpica/acutils.h @@ -579,6 +579,24 @@ acpi_ut_create_list(char *list_name, #endif /* ACPI_DBG_TRACK_ALLOCATIONS */ /* + * utaddress - address range check + */ +acpi_status +acpi_ut_add_address_range(acpi_adr_space_type space_id, + acpi_physical_address address, + u32 length, struct acpi_namespace_node *region_node); + +void +acpi_ut_remove_address_range(acpi_adr_space_type space_id, + struct acpi_namespace_node *region_node); + +u32 +acpi_ut_check_address_range(acpi_adr_space_type space_id, + acpi_physical_address address, u32 length, u8 warn); + +void acpi_ut_delete_address_lists(void); + +/* * utxferror - various error/warning output functions */ void ACPI_INTERNAL_VAR_XFACE diff --git a/drivers/acpi/acpica/dsargs.c b/drivers/acpi/acpica/dsargs.c index 8c7b997..0e62474 100644 --- a/drivers/acpi/acpica/dsargs.c +++ b/drivers/acpi/acpica/dsargs.c @@ -387,5 +387,12 @@ acpi_status acpi_ds_get_region_arguments(union acpi_operand_object *obj_desc) status = acpi_ds_execute_arguments(node, node->parent, extra_desc->extra.aml_length, extra_desc->extra.aml_start); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + status = acpi_ut_add_address_range(obj_desc->region.space_id, + obj_desc->region.address, + obj_desc->region.length, node); return_ACPI_STATUS(status); } diff --git a/drivers/acpi/acpica/utaddress.c b/drivers/acpi/acpica/utaddress.c new file mode 100644 index 0000000..6ef7649 --- /dev/null +++ b/drivers/acpi/acpica/utaddress.c @@ -0,0 +1,295 @@ +/****************************************************************************** + * + * Module Name: utaddress - op_region address range check + * + *****************************************************************************/ + +/* + * Copyright (C) 2000 - 2011, Intel Corp. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions, and the following disclaimer, + * without modification. + * 2. Redistributions in binary form must reproduce at minimum a disclaimer + * substantially similar to the "NO WARRANTY" disclaimer below + * ("Disclaimer") and any redistribution must be conditioned upon + * including a substantially similar Disclaimer requirement for further + * binary redistribution. + * 3. Neither the names of the above-listed copyright holders nor the names + * of any contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + * + * NO WARRANTY + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING + * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGES. + */ + +#include <acpi/acpi.h> +#include "accommon.h" +#include "acnamesp.h" + +#define _COMPONENT ACPI_UTILITIES +ACPI_MODULE_NAME("utaddress") + +/******************************************************************************* + * + * FUNCTION: acpi_ut_add_address_range + * + * PARAMETERS: space_id - Address space ID + * Address - op_region start address + * Length - op_region length + * region_node - op_region namespace node + * + * RETURN: Status + * + * DESCRIPTION: Add the Operation Region address range to the global list. + * The only supported Space IDs are Memory and I/O. Called when + * the op_region address/length operands are fully evaluated. + * + * MUTEX: Locks the namespace + * + * NOTE: Because this interface is only called when an op_region argument + * list is evaluated, there cannot be any duplicate region_nodes. + * Duplicate Address/Length values are allowed, however, so that multiple + * address conflicts can be detected. + * + ******************************************************************************/ +acpi_status +acpi_ut_add_address_range(acpi_adr_space_type space_id, + acpi_physical_address address, + u32 length, struct acpi_namespace_node *region_node) +{ + struct acpi_address_range *range_info; + acpi_status status; + + ACPI_FUNCTION_TRACE(ut_add_address_range); + + if ((space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) && + (space_id != ACPI_ADR_SPACE_SYSTEM_IO)) { + return_ACPI_STATUS(AE_OK); + } + + status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Allocate/init a new info block, add it to the appropriate list */ + + range_info = ACPI_ALLOCATE(sizeof(struct acpi_address_range)); + if (!range_info) { + status = AE_NO_MEMORY; + goto exit; + } + + range_info->start_address = address; + range_info->end_address = (address + length - 1); + range_info->region_node = region_node; + + range_info->next = acpi_gbl_address_range_list[space_id]; + acpi_gbl_address_range_list[space_id] = range_info; + + ACPI_DEBUG_PRINT((ACPI_DB_NAMES, + "\nAdded [%4.4s] address range: 0x%p-0x%p\n", + acpi_ut_get_node_name(range_info->region_node), + ACPI_CAST_PTR(void, address), + ACPI_CAST_PTR(void, range_info->end_address))); + + exit: + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); + return_ACPI_STATUS(status); +} + +/******************************************************************************* + * + * FUNCTION: acpi_ut_remove_address_range + * + * PARAMETERS: space_id - Address space ID + * region_node - op_region namespace node + * + * RETURN: None + * + * DESCRIPTION: Remove the Operation Region from the global list. The only + * supported Space IDs are Memory and I/O. Called when an + * op_region is deleted. + * + * MUTEX: Assumes the namespace is locked + * + ******************************************************************************/ + +void +acpi_ut_remove_address_range(acpi_adr_space_type space_id, + struct acpi_namespace_node *region_node) +{ + struct acpi_address_range *range_info; + struct acpi_address_range *prev; + + ACPI_FUNCTION_TRACE(ut_remove_address_range); + + if ((space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) && + (space_id != ACPI_ADR_SPACE_SYSTEM_IO)) { + return_VOID; + } + + /* Get the appropriate list head and check the list */ + + range_info = prev = acpi_gbl_address_range_list[space_id]; + while (range_info) { + if (range_info->region_node == region_node) { + if (range_info == prev) { /* Found at list head */ + acpi_gbl_address_range_list[space_id] = + range_info->next; + } else { + prev->next = range_info->next; + } + + ACPI_DEBUG_PRINT((ACPI_DB_NAMES, + "\nRemoved [%4.4s] address range: 0x%p-0x%p\n", + acpi_ut_get_node_name(range_info-> + region_node), + ACPI_CAST_PTR(void, + range_info-> + start_address), + ACPI_CAST_PTR(void, + range_info-> + end_address))); + + ACPI_FREE(range_info); + return_VOID; + } + + prev = range_info; + range_info = range_info->next; + } + + return_VOID; +} + +/******************************************************************************* + * + * FUNCTION: acpi_ut_check_address_range + * + * PARAMETERS: space_id - Address space ID + * Address - Start address + * Length - Length of address range + * Warn - TRUE if warning on overlap desired + * + * RETURN: Count of the number of conflicts detected. Zero is always + * returned for Space IDs other than Memory or I/O. + * + * DESCRIPTION: Check if the input address range overlaps any of the + * ASL operation region address ranges. The only supported + * Space IDs are Memory and I/O. + * + * MUTEX: Assumes the namespace is locked. + * + ******************************************************************************/ + +u32 +acpi_ut_check_address_range(acpi_adr_space_type space_id, + acpi_physical_address address, u32 length, u8 warn) +{ + struct acpi_address_range *range_info; + acpi_physical_address end_address; + char *pathname; + u32 overlap_count = 0; + + ACPI_FUNCTION_TRACE(ut_check_address_range); + + if ((space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) && + (space_id != ACPI_ADR_SPACE_SYSTEM_IO)) { + return_UINT32(0); + } + + range_info = acpi_gbl_address_range_list[space_id]; + end_address = address + length - 1; + + /* Check entire list for all possible conflicts */ + + while (range_info) { + /* + * Check if the requested Address/Length overlaps this address_range. + * Four cases to consider: + * + * 1) Input address/length is contained completely in the address range + * 2) Input address/length overlaps range at the range start + * 3) Input address/length overlaps range at the range end + * 4) Input address/length completely encompasses the range + */ + if ((address <= range_info->end_address) && + (end_address >= range_info->start_address)) { + + /* Found an address range overlap */ + + overlap_count++; + if (warn) { /* Optional warning message */ + pathname = + acpi_ns_get_external_pathname(range_info-> + region_node); + + ACPI_WARNING((AE_INFO, + "0x%p-0x%p %s conflicts with Region %s %d", + ACPI_CAST_PTR(void, address), + ACPI_CAST_PTR(void, end_address), + acpi_ut_get_region_name(space_id), + pathname, overlap_count)); + ACPI_FREE(pathname); + } + } + + range_info = range_info->next; + } + + return_UINT32(overlap_count); +} + +/******************************************************************************* + * + * FUNCTION: acpi_ut_delete_address_lists + * + * PARAMETERS: None + * + * RETURN: None + * + * DESCRIPTION: Delete all global address range lists (called during + * subsystem shutdown). + * + ******************************************************************************/ + +void acpi_ut_delete_address_lists(void) +{ + struct acpi_address_range *next; + struct acpi_address_range *range_info; + int i; + + /* Delete all elements in all address range lists */ + + for (i = 0; i < ACPI_ADDRESS_RANGE_MAX; i++) { + next = acpi_gbl_address_range_list[i]; + + while (next) { + range_info = next; + next = range_info->next; + ACPI_FREE(range_info); + } + + acpi_gbl_address_range_list[i] = NULL; + } +} diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c index 31f5a78..93ec06b 100644 --- a/drivers/acpi/acpica/utdelete.c +++ b/drivers/acpi/acpica/utdelete.c @@ -215,11 +215,14 @@ 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); + /* + * Update address_range list. However, only permanent regions + * are installed in this list. (Not created within a method) + */ + if (!(object->region.node->flags & ANOBJ_TEMPORARY)) { + acpi_ut_remove_address_range(object->region.space_id, + object->region.node); + } second_desc = acpi_ns_get_secondary_object(object); if (second_desc) { diff --git a/drivers/acpi/acpica/utglobal.c b/drivers/acpi/acpica/utglobal.c index ffba0a3..3cf758b 100644 --- a/drivers/acpi/acpica/utglobal.c +++ b/drivers/acpi/acpica/utglobal.c @@ -264,6 +264,12 @@ acpi_status acpi_ut_init_globals(void) return_ACPI_STATUS(status); } + /* Address Range lists */ + + for (i = 0; i < ACPI_ADDRESS_RANGE_MAX; i++) { + acpi_gbl_address_range_list[i] = NULL; + } + /* Mutex locked flags */ for (i = 0; i < ACPI_NUM_MUTEX; i++) { diff --git a/drivers/acpi/acpica/utinit.c b/drivers/acpi/acpica/utinit.c index 191b682..cab61a0 100644 --- a/drivers/acpi/acpica/utinit.c +++ b/drivers/acpi/acpica/utinit.c @@ -92,6 +92,7 @@ static void acpi_ut_terminate(void) gpe_xrupt_info = next_gpe_xrupt_info; } + acpi_ut_delete_address_lists(); return_VOID; } diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c index 420ebfe..15cbdde 100644 --- a/drivers/acpi/acpica/utxface.c +++ b/drivers/acpi/acpica/utxface.c @@ -48,6 +48,7 @@ #include "acnamesp.h" #include "acdebug.h" #include "actables.h" +#include "acinterp.h" #define _COMPONENT ACPI_UTILITIES ACPI_MODULE_NAME("utxface") @@ -640,4 +641,41 @@ acpi_status acpi_install_interface_handler(acpi_interface_handler handler) } ACPI_EXPORT_SYMBOL(acpi_install_interface_handler) + +/***************************************************************************** + * + * FUNCTION: acpi_check_address_range + * + * PARAMETERS: space_id - Address space ID + * Address - Start address + * Length - Length + * Warn - TRUE if warning on overlap desired + * + * RETURN: Count of the number of conflicts detected. + * + * DESCRIPTION: Check if the input address range overlaps any of the + * ASL operation region address ranges. + * + ****************************************************************************/ +u32 +acpi_check_address_range(acpi_adr_space_type space_id, + acpi_physical_address address, + acpi_size length, u8 warn) +{ + u32 overlaps; + acpi_status status; + + status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); + if (ACPI_FAILURE(status)) { + return (0); + } + + overlaps = acpi_ut_check_address_range(space_id, address, + (u32)length, warn); + + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); + return (overlaps); +} + +ACPI_EXPORT_SYMBOL(acpi_check_address_range) #endif /* !ACPI_ASL_COMPILER */ diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index f31c5c5..3e57fbd 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -83,19 +83,6 @@ static struct workqueue_struct *kacpi_notify_wq; struct workqueue_struct *kacpi_hotplug_wq; EXPORT_SYMBOL(kacpi_hotplug_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; - int count; -}; - -static LIST_HEAD(resource_list_head); -static DEFINE_SPINLOCK(acpi_res_lock); - /* * This list of permanent mappings is for memory that may be accessed from * interrupt context, where we can't do the ioremap(). @@ -1278,44 +1265,28 @@ __setup("acpi_enforce_resources=", acpi_enforce_resources_setup); * drivers */ int acpi_check_resource_conflict(const struct resource *res) { - struct acpi_res_list *res_list_elem; - int ioport = 0, clash = 0; + acpi_adr_space_type space_id; + acpi_size length; + u8 warn = 0; + int clash = 0; if (acpi_enforce_resources == ENFORCE_RESOURCES_NO) return 0; if (!(res->flags & IORESOURCE_IO) && !(res->flags & IORESOURCE_MEM)) return 0; - ioport = res->flags & IORESOURCE_IO; - - 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; + if (res->flags & IORESOURCE_IO) + space_id = ACPI_ADR_SPACE_SYSTEM_IO; + else + space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY; - if (res->end < res_list_elem->start - || res_list_elem->end < res->start) - continue; - clash = 1; - break; - } - spin_unlock(&acpi_res_lock); + length = res->end - res->start + 1; + if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) + warn = 1; + clash = acpi_check_address_range(space_id, res->start, length, warn); if (clash) { if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) { - printk(KERN_WARNING "ACPI: resource %s %pR" - " conflicts with ACPI region %s " - "[%s 0x%zx-0x%zx]\n", - res->name, res, res_list_elem->name, - (res_list_elem->resource_type == - ACPI_ADR_SPACE_SYSTEM_IO) ? "io" : "mem", - (size_t) res_list_elem->start, - (size_t) res_list_elem->end); if (acpi_enforce_resources == ENFORCE_RESOURCES_LAX) printk(KERN_NOTICE "ACPI: This conflict may" " cause random problems and system" @@ -1467,155 +1438,6 @@ acpi_status acpi_os_release_object(acpi_cache_t * cache, void *object) kmem_cache_free(cache, object); return (AE_OK); } - -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 SystemMemory - 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 - * - * PARAMETERS: space_id - ACPI space ID - * address - Physical address - * length - Address length - * - * RETURN: AE_OK if address/length is valid for the space_id. Otherwise, - * should return AE_AML_ILLEGAL_ADDRESS. - * - * DESCRIPTION: Validate a system address via the host OS. Used to validate - * the addresses accessed by AML operation regions. - * - *****************************************************************************/ - -acpi_status -acpi_os_validate_address ( - u8 space_id, - acpi_physical_address address, - acpi_size length, - char *name) -{ - struct acpi_res_list *res; - int added; - 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 SystemMemory - 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); - added = acpi_res_list_add(res); - spin_unlock(&acpi_res_lock); - 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: - 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; -} #endif acpi_status __init acpi_os_initialize(void) diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index f554a93..7d7df17 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -111,6 +111,11 @@ acpi_status acpi_install_interface(acpi_string interface_name); acpi_status acpi_remove_interface(acpi_string interface_name); +u32 +acpi_check_address_range(acpi_adr_space_type space_id, + acpi_physical_address address, + acpi_size length, u8 warn); + /* * ACPI Memory management */ > > > diff --git a/drivers/acpi/acpica/dsargs.c b/drivers/acpi/acpica/dsargs.c > > index 8c7b997..42163d8 100644 > > --- a/drivers/acpi/acpica/dsargs.c > > +++ b/drivers/acpi/acpica/dsargs.c > > @@ -387,5 +387,29 @@ acpi_status acpi_ds_get_region_arguments(union acpi_operand_object *obj_desc) > > status = acpi_ds_execute_arguments(node, node->parent, > > extra_desc->extra.aml_length, > > extra_desc->extra.aml_start); > > + if (ACPI_FAILURE(status)) { > > + return_ACPI_STATUS(status); > > + } > > + > > + /* Validate the region address/length via the host OS */ > > + > > + status = acpi_os_validate_address(obj_desc->region.space_id, > > + obj_desc->region.address, > > + (acpi_size) obj_desc->region.length, > > + acpi_ut_get_node_name(node)); > > + > > + if (ACPI_FAILURE(status)) { > > + /* > > + * Invalid address/length. We will emit an error message and mark > > + * the region as invalid, so that it will cause an additional error if > > + * it is ever used. Then return AE_OK. > > + */ > > + ACPI_EXCEPTION((AE_INFO, status, > > + "During address validation of OpRegion [%4.4s]", > > + node->name.ascii)); > > + obj_desc->common.flags |= AOPOBJ_INVALID; > > + status = AE_OK; > > + } > > + > > return_ACPI_STATUS(status); > > } > > thanks, > Luca ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-12-09 2:01 ` Lin Ming @ 2011-12-12 9:37 ` Jean Delvare 2011-12-12 12:42 ` Lin Ming 2011-12-12 12:52 ` Luca Tettamanti 1 sibling, 1 reply; 13+ messages in thread From: Jean Delvare @ 2011-12-12 9:37 UTC (permalink / raw) To: Lin Ming Cc: Luca Tettamanti, Moore, Robert, Nikolay Mikov, Thomas, linux-acpi, lenb Hi Lin, On Fri, 09 Dec 2011 10:01:49 +0800, Lin Ming wrote: > On Tue, 2011-11-29 at 21:18 +0800, Luca Tettamanti wrote: > > On Tue, Nov 29, 2011 at 2:48 AM, Lin Ming <ming.m.lin@intel.com> wrote: > > > Yes, we should put the call back. > > > > Great, are you going to push this patch yourself? > > We have investigated this issue more and think that it's better to move > the resource validation code into ACPICA core. > > Here is the new patch. > > The major changes include: > - Remove acpi_os_validate_address/acpi_os_invalidate_address from osl.c. > They are reimplemented in ACPICA core: > acpi_ut_add_address_range/acpi_ut_remove_address_range. > > - Add new interface for drivers to check resource conflict: > acpi_check_address_range > > Could you help to test it? I have no objection for an upstream patch, but the main problem we have at the moment is with already released kernels. Versions 2.6.39, 3.0 and 3.1 currently have a regression as the ACPI resource conflict checks are inefficient, and this allows conflicting drivers to be loaded together. So you are free to reimplement things differently in version 3.2 and later, but for these 3 older versions we need the smallest possible patch, so that it is accepted in stable branches. In other words, I would like two patches, one just adding back the code that was accidentally dropped, and a second one moving things around if you think it makes sense (and I tend to agree.) That way we can easily backport only the first patch to kernel versions 2.6.39 to 3.1. Thanks, Jean > Thanks, > Lin Ming > --- > drivers/acpi/acpica/Makefile | 2 +- > drivers/acpi/acpica/acconfig.h | 4 + > drivers/acpi/acpica/acglobal.h | 2 + > drivers/acpi/acpica/aclocal.h | 9 ++ > drivers/acpi/acpica/acutils.h | 18 +++ > drivers/acpi/acpica/dsargs.c | 7 + > drivers/acpi/acpica/utaddress.c | 295 +++++++++++++++++++++++++++++++++++++++ > drivers/acpi/acpica/utdelete.c | 13 +- > drivers/acpi/acpica/utglobal.c | 6 + > drivers/acpi/acpica/utinit.c | 1 + > drivers/acpi/acpica/utxface.c | 38 +++++ > drivers/acpi/osl.c | 202 ++------------------------- > include/acpi/acpixf.h | 5 + > 13 files changed, 406 insertions(+), 196 deletions(-) > (...) -- Jean Delvare ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-12-12 9:37 ` Jean Delvare @ 2011-12-12 12:42 ` Lin Ming 2011-12-12 20:12 ` Jean Delvare 0 siblings, 1 reply; 13+ messages in thread From: Lin Ming @ 2011-12-12 12:42 UTC (permalink / raw) To: Jean Delvare Cc: Luca Tettamanti, Moore, Robert, Nikolay Mikov, Thomas, linux-acpi, lenb On Mon, Dec 12, 2011 at 5:37 PM, Jean Delvare <khali@linux-fr.org> wrote: > Hi Lin, > > On Fri, 09 Dec 2011 10:01:49 +0800, Lin Ming wrote: >> On Tue, 2011-11-29 at 21:18 +0800, Luca Tettamanti wrote: >> > On Tue, Nov 29, 2011 at 2:48 AM, Lin Ming <ming.m.lin@intel.com> wrote: >> > > Yes, we should put the call back. >> > >> > Great, are you going to push this patch yourself? >> >> We have investigated this issue more and think that it's better to move >> the resource validation code into ACPICA core. >> >> Here is the new patch. >> >> The major changes include: >> - Remove acpi_os_validate_address/acpi_os_invalidate_address from osl.c. >> They are reimplemented in ACPICA core: >> acpi_ut_add_address_range/acpi_ut_remove_address_range. >> >> - Add new interface for drivers to check resource conflict: >> acpi_check_address_range >> >> Could you help to test it? > > I have no objection for an upstream patch, but the main problem we have > at the moment is with already released kernels. Versions 2.6.39, 3.0 > and 3.1 currently have a regression as the ACPI resource conflict > checks are inefficient, and this allows conflicting drivers to be > loaded together. So you are free to reimplement things differently in > version 3.2 and later, but for these 3 older versions we need the > smallest possible patch, so that it is accepted in stable branches. > > In other words, I would like two patches, one just adding back the code > that was accidentally dropped, and a second one moving things around if > you think it makes sense (and I tend to agree.) That way we can easily > backport only the first patch to kernel versions 2.6.39 to 3.1. Sure. I have send out the patch. [PATCH] ACPICA: Put back the call to acpi_os_validate_address http://marc.info/?l=linux-acpi&m=132257617527119&w=2 Regards, Lin Ming -- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-12-12 12:42 ` Lin Ming @ 2011-12-12 20:12 ` Jean Delvare 2012-01-11 13:01 ` Jean Delvare 0 siblings, 1 reply; 13+ messages in thread From: Jean Delvare @ 2011-12-12 20:12 UTC (permalink / raw) To: Lin Ming Cc: Luca Tettamanti, Moore, Robert, Nikolay Mikov, Thomas, linux-acpi, lenb On Mon, 12 Dec 2011 20:42:50 +0800, Lin Ming wrote: > On Mon, Dec 12, 2011 at 5:37 PM, Jean Delvare <khali@linux-fr.org> wrote: > > I have no objection for an upstream patch, but the main problem we have > > at the moment is with already released kernels. Versions 2.6.39, 3.0 > > and 3.1 currently have a regression as the ACPI resource conflict > > checks are inefficient, and this allows conflicting drivers to be > > loaded together. So you are free to reimplement things differently in > > version 3.2 and later, but for these 3 older versions we need the > > smallest possible patch, so that it is accepted in stable branches. > > > > In other words, I would like two patches, one just adding back the code > > that was accidentally dropped, and a second one moving things around if > > you think it makes sense (and I tend to agree.) That way we can easily > > backport only the first patch to kernel versions 2.6.39 to 3.1. > > Sure. > > I have send out the patch. > > [PATCH] ACPICA: Put back the call to acpi_os_validate_address > http://marc.info/?l=linux-acpi&m=132257617527119&w=2 Perfect, thank you! -- Jean Delvare ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-12-12 20:12 ` Jean Delvare @ 2012-01-11 13:01 ` Jean Delvare 0 siblings, 0 replies; 13+ messages in thread From: Jean Delvare @ 2012-01-11 13:01 UTC (permalink / raw) To: Lin Ming, Len Brown Cc: Luca Tettamanti, Moore, Robert, Nikolay Mikov, Thomas, linux-acpi On Mon, 12 Dec 2011 21:12:45 +0100, Jean Delvare wrote: > On Mon, 12 Dec 2011 20:42:50 +0800, Lin Ming wrote: > > On Mon, Dec 12, 2011 at 5:37 PM, Jean Delvare <khali@linux-fr.org> wrote: > > > I have no objection for an upstream patch, but the main problem we have > > > at the moment is with already released kernels. Versions 2.6.39, 3.0 > > > and 3.1 currently have a regression as the ACPI resource conflict > > > checks are inefficient, and this allows conflicting drivers to be > > > loaded together. So you are free to reimplement things differently in > > > version 3.2 and later, but for these 3 older versions we need the > > > smallest possible patch, so that it is accepted in stable branches. > > > > > > In other words, I would like two patches, one just adding back the code > > > that was accidentally dropped, and a second one moving things around if > > > you think it makes sense (and I tend to agree.) That way we can easily > > > backport only the first patch to kernel versions 2.6.39 to 3.1. > > > > Sure. > > > > I have send out the patch. > > > > [PATCH] ACPICA: Put back the call to acpi_os_validate_address > > http://marc.info/?l=linux-acpi&m=132257617527119&w=2 > > Perfect, thank you! I still don't see this important fix upstream. Len, can you please get it there quickly, so that it can propagate to stable kernels from there? Thanks, -- Jean Delvare ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-12-09 2:01 ` Lin Ming 2011-12-12 9:37 ` Jean Delvare @ 2011-12-12 12:52 ` Luca Tettamanti 2011-12-13 2:01 ` Lin Ming 1 sibling, 1 reply; 13+ messages in thread From: Luca Tettamanti @ 2011-12-12 12:52 UTC (permalink / raw) To: Lin Ming Cc: Moore, Robert, Jean Delvare, Nikolay Mikov, Thomas, linux-acpi@vger.kernel.org, lenb On Fri, Dec 9, 2011 at 3:01 AM, Lin Ming <ming.m.lin@intel.com> wrote: > Hi Luca, > > We have investigated this issue more and think that it's better to move > the resource validation code into ACPICA core. > > Here is the new patch. > > The major changes include: > - Remove acpi_os_validate_address/acpi_os_invalidate_address from osl.c. > They are reimplemented in ACPICA core: > acpi_ut_add_address_range/acpi_ut_remove_address_range. > > - Add new interface for drivers to check resource conflict: > acpi_check_address_range > > Could you help to test it? Sure, works fine: Tested-by: Luca Tettamanti <kronos.it@gmail.com> > > Thanks, > Lin Ming > --- > drivers/acpi/acpica/Makefile | 2 +- > drivers/acpi/acpica/acconfig.h | 4 + > drivers/acpi/acpica/acglobal.h | 2 + > drivers/acpi/acpica/aclocal.h | 9 ++ > drivers/acpi/acpica/acutils.h | 18 +++ > drivers/acpi/acpica/dsargs.c | 7 + > drivers/acpi/acpica/utaddress.c | 295 +++++++++++++++++++++++++++++++++++++++ > drivers/acpi/acpica/utdelete.c | 13 +- > drivers/acpi/acpica/utglobal.c | 6 + > drivers/acpi/acpica/utinit.c | 1 + > drivers/acpi/acpica/utxface.c | 38 +++++ > drivers/acpi/osl.c | 202 ++------------------------- > include/acpi/acpixf.h | 5 + > 13 files changed, 406 insertions(+), 196 deletions(-) > > diff --git a/drivers/acpi/acpica/Makefile b/drivers/acpi/acpica/Makefile > index 301bd2d..855785b 100644 > --- a/drivers/acpi/acpica/Makefile > +++ b/drivers/acpi/acpica/Makefile > @@ -45,4 +45,4 @@ acpi-y += tbxface.o tbinstal.o tbutils.o tbfind.o tbfadt.o tbxfroot.o > acpi-y += utalloc.o utdebug.o uteval.o utinit.o utmisc.o utxface.o \ > utcopy.o utdelete.o utglobal.o utmath.o utobject.o \ > utstate.o utmutex.o utobject.o utresrc.o utlock.o utids.o \ > - utosi.o utxferror.o utdecode.o > + utosi.o utxferror.o utdecode.o utaddress.o > diff --git a/drivers/acpi/acpica/acconfig.h b/drivers/acpi/acpica/acconfig.h > index f895a24..4dbd010 100644 > --- a/drivers/acpi/acpica/acconfig.h > +++ b/drivers/acpi/acpica/acconfig.h > @@ -123,6 +123,10 @@ > > #define ACPI_MAX_SLEEP 2000 /* Two seconds */ > > +/* Address Range lists are per-space_id (Memory and I/O only) */ > + > +#define ACPI_ADDRESS_RANGE_MAX 2 > + > /****************************************************************************** > * > * ACPI Specification constants (Do not change unless the specification changes) > diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h > index 76dc02f..b7e7fbd 100644 > --- a/drivers/acpi/acpica/acglobal.h > +++ b/drivers/acpi/acpica/acglobal.h > @@ -295,6 +295,8 @@ ACPI_EXTERN u8 acpi_gbl_acpi_hardware_present; > ACPI_EXTERN u8 acpi_gbl_events_initialized; > ACPI_EXTERN u8 acpi_gbl_osi_data; > ACPI_EXTERN struct acpi_interface_info *acpi_gbl_supported_interfaces; > +ACPI_EXTERN struct acpi_address_range > + *acpi_gbl_address_range_list[ACPI_ADDRESS_RANGE_MAX]; > > #ifndef DEFINE_ACPI_GLOBALS > > diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h > index 5552125..44a2164 100644 > --- a/drivers/acpi/acpica/aclocal.h > +++ b/drivers/acpi/acpica/aclocal.h > @@ -625,6 +625,15 @@ union acpi_generic_state { > > typedef acpi_status(*ACPI_EXECUTE_OP) (struct acpi_walk_state * walk_state); > > +/* Address Range info block */ > + > +struct acpi_address_range { > + struct acpi_address_range *next; > + struct acpi_namespace_node *region_node; > + acpi_physical_address start_address; > + acpi_physical_address end_address; > +}; > + > /***************************************************************************** > * > * Parser typedefs and structs > diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h > index 99c140d..dfea11d 100644 > --- a/drivers/acpi/acpica/acutils.h > +++ b/drivers/acpi/acpica/acutils.h > @@ -579,6 +579,24 @@ acpi_ut_create_list(char *list_name, > #endif /* ACPI_DBG_TRACK_ALLOCATIONS */ > > /* > + * utaddress - address range check > + */ > +acpi_status > +acpi_ut_add_address_range(acpi_adr_space_type space_id, > + acpi_physical_address address, > + u32 length, struct acpi_namespace_node *region_node); > + > +void > +acpi_ut_remove_address_range(acpi_adr_space_type space_id, > + struct acpi_namespace_node *region_node); > + > +u32 > +acpi_ut_check_address_range(acpi_adr_space_type space_id, > + acpi_physical_address address, u32 length, u8 warn); > + > +void acpi_ut_delete_address_lists(void); > + > +/* > * utxferror - various error/warning output functions > */ > void ACPI_INTERNAL_VAR_XFACE > diff --git a/drivers/acpi/acpica/dsargs.c b/drivers/acpi/acpica/dsargs.c > index 8c7b997..0e62474 100644 > --- a/drivers/acpi/acpica/dsargs.c > +++ b/drivers/acpi/acpica/dsargs.c > @@ -387,5 +387,12 @@ acpi_status acpi_ds_get_region_arguments(union acpi_operand_object *obj_desc) > status = acpi_ds_execute_arguments(node, node->parent, > extra_desc->extra.aml_length, > extra_desc->extra.aml_start); > + if (ACPI_FAILURE(status)) { > + return_ACPI_STATUS(status); > + } > + > + status = acpi_ut_add_address_range(obj_desc->region.space_id, > + obj_desc->region.address, > + obj_desc->region.length, node); > return_ACPI_STATUS(status); > } > diff --git a/drivers/acpi/acpica/utaddress.c b/drivers/acpi/acpica/utaddress.c > new file mode 100644 > index 0000000..6ef7649 > --- /dev/null > +++ b/drivers/acpi/acpica/utaddress.c > @@ -0,0 +1,295 @@ > +/****************************************************************************** > + * > + * Module Name: utaddress - op_region address range check > + * > + *****************************************************************************/ > + > +/* > + * Copyright (C) 2000 - 2011, Intel Corp. > + * All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions, and the following disclaimer, > + * without modification. > + * 2. Redistributions in binary form must reproduce at minimum a disclaimer > + * substantially similar to the "NO WARRANTY" disclaimer below > + * ("Disclaimer") and any redistribution must be conditioned upon > + * including a substantially similar Disclaimer requirement for further > + * binary redistribution. > + * 3. Neither the names of the above-listed copyright holders nor the names > + * of any contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * Alternatively, this software may be distributed under the terms of the > + * GNU General Public License ("GPL") version 2 as published by the Free > + * Software Foundation. > + * > + * NO WARRANTY > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, > + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING > + * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > + * POSSIBILITY OF SUCH DAMAGES. > + */ > + > +#include <acpi/acpi.h> > +#include "accommon.h" > +#include "acnamesp.h" > + > +#define _COMPONENT ACPI_UTILITIES > +ACPI_MODULE_NAME("utaddress") > + > +/******************************************************************************* > + * > + * FUNCTION: acpi_ut_add_address_range > + * > + * PARAMETERS: space_id - Address space ID > + * Address - op_region start address > + * Length - op_region length > + * region_node - op_region namespace node > + * > + * RETURN: Status > + * > + * DESCRIPTION: Add the Operation Region address range to the global list. > + * The only supported Space IDs are Memory and I/O. Called when > + * the op_region address/length operands are fully evaluated. > + * > + * MUTEX: Locks the namespace > + * > + * NOTE: Because this interface is only called when an op_region argument > + * list is evaluated, there cannot be any duplicate region_nodes. > + * Duplicate Address/Length values are allowed, however, so that multiple > + * address conflicts can be detected. > + * > + ******************************************************************************/ > +acpi_status > +acpi_ut_add_address_range(acpi_adr_space_type space_id, > + acpi_physical_address address, > + u32 length, struct acpi_namespace_node *region_node) > +{ > + struct acpi_address_range *range_info; > + acpi_status status; > + > + ACPI_FUNCTION_TRACE(ut_add_address_range); > + > + if ((space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) && > + (space_id != ACPI_ADR_SPACE_SYSTEM_IO)) { > + return_ACPI_STATUS(AE_OK); > + } > + > + status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); > + if (ACPI_FAILURE(status)) { > + return_ACPI_STATUS(status); > + } > + > + /* Allocate/init a new info block, add it to the appropriate list */ > + > + range_info = ACPI_ALLOCATE(sizeof(struct acpi_address_range)); > + if (!range_info) { > + status = AE_NO_MEMORY; > + goto exit; > + } > + > + range_info->start_address = address; > + range_info->end_address = (address + length - 1); > + range_info->region_node = region_node; > + > + range_info->next = acpi_gbl_address_range_list[space_id]; > + acpi_gbl_address_range_list[space_id] = range_info; > + > + ACPI_DEBUG_PRINT((ACPI_DB_NAMES, > + "\nAdded [%4.4s] address range: 0x%p-0x%p\n", > + acpi_ut_get_node_name(range_info->region_node), > + ACPI_CAST_PTR(void, address), > + ACPI_CAST_PTR(void, range_info->end_address))); > + > + exit: > + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); > + return_ACPI_STATUS(status); > +} > + > +/******************************************************************************* > + * > + * FUNCTION: acpi_ut_remove_address_range > + * > + * PARAMETERS: space_id - Address space ID > + * region_node - op_region namespace node > + * > + * RETURN: None > + * > + * DESCRIPTION: Remove the Operation Region from the global list. The only > + * supported Space IDs are Memory and I/O. Called when an > + * op_region is deleted. > + * > + * MUTEX: Assumes the namespace is locked > + * > + ******************************************************************************/ > + > +void > +acpi_ut_remove_address_range(acpi_adr_space_type space_id, > + struct acpi_namespace_node *region_node) > +{ > + struct acpi_address_range *range_info; > + struct acpi_address_range *prev; > + > + ACPI_FUNCTION_TRACE(ut_remove_address_range); > + > + if ((space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) && > + (space_id != ACPI_ADR_SPACE_SYSTEM_IO)) { > + return_VOID; > + } > + > + /* Get the appropriate list head and check the list */ > + > + range_info = prev = acpi_gbl_address_range_list[space_id]; > + while (range_info) { > + if (range_info->region_node == region_node) { > + if (range_info == prev) { /* Found at list head */ > + acpi_gbl_address_range_list[space_id] = > + range_info->next; > + } else { > + prev->next = range_info->next; > + } > + > + ACPI_DEBUG_PRINT((ACPI_DB_NAMES, > + "\nRemoved [%4.4s] address range: 0x%p-0x%p\n", > + acpi_ut_get_node_name(range_info-> > + region_node), > + ACPI_CAST_PTR(void, > + range_info-> > + start_address), > + ACPI_CAST_PTR(void, > + range_info-> > + end_address))); > + > + ACPI_FREE(range_info); > + return_VOID; > + } > + > + prev = range_info; > + range_info = range_info->next; > + } > + > + return_VOID; > +} > + > +/******************************************************************************* > + * > + * FUNCTION: acpi_ut_check_address_range > + * > + * PARAMETERS: space_id - Address space ID > + * Address - Start address > + * Length - Length of address range > + * Warn - TRUE if warning on overlap desired > + * > + * RETURN: Count of the number of conflicts detected. Zero is always > + * returned for Space IDs other than Memory or I/O. > + * > + * DESCRIPTION: Check if the input address range overlaps any of the > + * ASL operation region address ranges. The only supported > + * Space IDs are Memory and I/O. > + * > + * MUTEX: Assumes the namespace is locked. > + * > + ******************************************************************************/ > + > +u32 > +acpi_ut_check_address_range(acpi_adr_space_type space_id, > + acpi_physical_address address, u32 length, u8 warn) > +{ > + struct acpi_address_range *range_info; > + acpi_physical_address end_address; > + char *pathname; > + u32 overlap_count = 0; > + > + ACPI_FUNCTION_TRACE(ut_check_address_range); > + > + if ((space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) && > + (space_id != ACPI_ADR_SPACE_SYSTEM_IO)) { > + return_UINT32(0); > + } > + > + range_info = acpi_gbl_address_range_list[space_id]; > + end_address = address + length - 1; > + > + /* Check entire list for all possible conflicts */ > + > + while (range_info) { > + /* > + * Check if the requested Address/Length overlaps this address_range. > + * Four cases to consider: > + * > + * 1) Input address/length is contained completely in the address range > + * 2) Input address/length overlaps range at the range start > + * 3) Input address/length overlaps range at the range end > + * 4) Input address/length completely encompasses the range > + */ > + if ((address <= range_info->end_address) && > + (end_address >= range_info->start_address)) { > + > + /* Found an address range overlap */ > + > + overlap_count++; > + if (warn) { /* Optional warning message */ > + pathname = > + acpi_ns_get_external_pathname(range_info-> > + region_node); > + > + ACPI_WARNING((AE_INFO, > + "0x%p-0x%p %s conflicts with Region %s %d", > + ACPI_CAST_PTR(void, address), > + ACPI_CAST_PTR(void, end_address), > + acpi_ut_get_region_name(space_id), > + pathname, overlap_count)); > + ACPI_FREE(pathname); > + } > + } > + > + range_info = range_info->next; > + } > + > + return_UINT32(overlap_count); > +} > + > +/******************************************************************************* > + * > + * FUNCTION: acpi_ut_delete_address_lists > + * > + * PARAMETERS: None > + * > + * RETURN: None > + * > + * DESCRIPTION: Delete all global address range lists (called during > + * subsystem shutdown). > + * > + ******************************************************************************/ > + > +void acpi_ut_delete_address_lists(void) > +{ > + struct acpi_address_range *next; > + struct acpi_address_range *range_info; > + int i; > + > + /* Delete all elements in all address range lists */ > + > + for (i = 0; i < ACPI_ADDRESS_RANGE_MAX; i++) { > + next = acpi_gbl_address_range_list[i]; > + > + while (next) { > + range_info = next; > + next = range_info->next; > + ACPI_FREE(range_info); > + } > + > + acpi_gbl_address_range_list[i] = NULL; > + } > +} > diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c > index 31f5a78..93ec06b 100644 > --- a/drivers/acpi/acpica/utdelete.c > +++ b/drivers/acpi/acpica/utdelete.c > @@ -215,11 +215,14 @@ 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); > + /* > + * Update address_range list. However, only permanent regions > + * are installed in this list. (Not created within a method) > + */ > + if (!(object->region.node->flags & ANOBJ_TEMPORARY)) { > + acpi_ut_remove_address_range(object->region.space_id, > + object->region.node); > + } > > second_desc = acpi_ns_get_secondary_object(object); > if (second_desc) { > diff --git a/drivers/acpi/acpica/utglobal.c b/drivers/acpi/acpica/utglobal.c > index ffba0a3..3cf758b 100644 > --- a/drivers/acpi/acpica/utglobal.c > +++ b/drivers/acpi/acpica/utglobal.c > @@ -264,6 +264,12 @@ acpi_status acpi_ut_init_globals(void) > return_ACPI_STATUS(status); > } > > + /* Address Range lists */ > + > + for (i = 0; i < ACPI_ADDRESS_RANGE_MAX; i++) { > + acpi_gbl_address_range_list[i] = NULL; > + } > + > /* Mutex locked flags */ > > for (i = 0; i < ACPI_NUM_MUTEX; i++) { > diff --git a/drivers/acpi/acpica/utinit.c b/drivers/acpi/acpica/utinit.c > index 191b682..cab61a0 100644 > --- a/drivers/acpi/acpica/utinit.c > +++ b/drivers/acpi/acpica/utinit.c > @@ -92,6 +92,7 @@ static void acpi_ut_terminate(void) > gpe_xrupt_info = next_gpe_xrupt_info; > } > > + acpi_ut_delete_address_lists(); > return_VOID; > } > > diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c > index 420ebfe..15cbdde 100644 > --- a/drivers/acpi/acpica/utxface.c > +++ b/drivers/acpi/acpica/utxface.c > @@ -48,6 +48,7 @@ > #include "acnamesp.h" > #include "acdebug.h" > #include "actables.h" > +#include "acinterp.h" > > #define _COMPONENT ACPI_UTILITIES > ACPI_MODULE_NAME("utxface") > @@ -640,4 +641,41 @@ acpi_status acpi_install_interface_handler(acpi_interface_handler handler) > } > > ACPI_EXPORT_SYMBOL(acpi_install_interface_handler) > + > +/***************************************************************************** > + * > + * FUNCTION: acpi_check_address_range > + * > + * PARAMETERS: space_id - Address space ID > + * Address - Start address > + * Length - Length > + * Warn - TRUE if warning on overlap desired > + * > + * RETURN: Count of the number of conflicts detected. > + * > + * DESCRIPTION: Check if the input address range overlaps any of the > + * ASL operation region address ranges. > + * > + ****************************************************************************/ > +u32 > +acpi_check_address_range(acpi_adr_space_type space_id, > + acpi_physical_address address, > + acpi_size length, u8 warn) > +{ > + u32 overlaps; > + acpi_status status; > + > + status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); > + if (ACPI_FAILURE(status)) { > + return (0); > + } > + > + overlaps = acpi_ut_check_address_range(space_id, address, > + (u32)length, warn); > + > + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); > + return (overlaps); > +} > + > +ACPI_EXPORT_SYMBOL(acpi_check_address_range) > #endif /* !ACPI_ASL_COMPILER */ > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index f31c5c5..3e57fbd 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -83,19 +83,6 @@ static struct workqueue_struct *kacpi_notify_wq; > struct workqueue_struct *kacpi_hotplug_wq; > EXPORT_SYMBOL(kacpi_hotplug_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; > - int count; > -}; > - > -static LIST_HEAD(resource_list_head); > -static DEFINE_SPINLOCK(acpi_res_lock); > - > /* > * This list of permanent mappings is for memory that may be accessed from > * interrupt context, where we can't do the ioremap(). > @@ -1278,44 +1265,28 @@ __setup("acpi_enforce_resources=", acpi_enforce_resources_setup); > * drivers */ > int acpi_check_resource_conflict(const struct resource *res) > { > - struct acpi_res_list *res_list_elem; > - int ioport = 0, clash = 0; > + acpi_adr_space_type space_id; > + acpi_size length; > + u8 warn = 0; > + int clash = 0; > > if (acpi_enforce_resources == ENFORCE_RESOURCES_NO) > return 0; > if (!(res->flags & IORESOURCE_IO) && !(res->flags & IORESOURCE_MEM)) > return 0; > > - ioport = res->flags & IORESOURCE_IO; > - > - 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; > + if (res->flags & IORESOURCE_IO) > + space_id = ACPI_ADR_SPACE_SYSTEM_IO; > + else > + space_id = ACPI_ADR_SPACE_SYSTEM_MEMORY; > > - if (res->end < res_list_elem->start > - || res_list_elem->end < res->start) > - continue; > - clash = 1; > - break; > - } > - spin_unlock(&acpi_res_lock); > + length = res->end - res->start + 1; > + if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) > + warn = 1; > + clash = acpi_check_address_range(space_id, res->start, length, warn); > > if (clash) { > if (acpi_enforce_resources != ENFORCE_RESOURCES_NO) { > - printk(KERN_WARNING "ACPI: resource %s %pR" > - " conflicts with ACPI region %s " > - "[%s 0x%zx-0x%zx]\n", > - res->name, res, res_list_elem->name, > - (res_list_elem->resource_type == > - ACPI_ADR_SPACE_SYSTEM_IO) ? "io" : "mem", > - (size_t) res_list_elem->start, > - (size_t) res_list_elem->end); > if (acpi_enforce_resources == ENFORCE_RESOURCES_LAX) > printk(KERN_NOTICE "ACPI: This conflict may" > " cause random problems and system" > @@ -1467,155 +1438,6 @@ acpi_status acpi_os_release_object(acpi_cache_t * cache, void *object) > kmem_cache_free(cache, object); > return (AE_OK); > } > - > -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 SystemMemory > - 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 > - * > - * PARAMETERS: space_id - ACPI space ID > - * address - Physical address > - * length - Address length > - * > - * RETURN: AE_OK if address/length is valid for the space_id. Otherwise, > - * should return AE_AML_ILLEGAL_ADDRESS. > - * > - * DESCRIPTION: Validate a system address via the host OS. Used to validate > - * the addresses accessed by AML operation regions. > - * > - *****************************************************************************/ > - > -acpi_status > -acpi_os_validate_address ( > - u8 space_id, > - acpi_physical_address address, > - acpi_size length, > - char *name) > -{ > - struct acpi_res_list *res; > - int added; > - 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 SystemMemory > - 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); > - added = acpi_res_list_add(res); > - spin_unlock(&acpi_res_lock); > - 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: > - 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; > -} > #endif > > acpi_status __init acpi_os_initialize(void) > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h > index f554a93..7d7df17 100644 > --- a/include/acpi/acpixf.h > +++ b/include/acpi/acpixf.h > @@ -111,6 +111,11 @@ acpi_status acpi_install_interface(acpi_string interface_name); > > acpi_status acpi_remove_interface(acpi_string interface_name); > > +u32 > +acpi_check_address_range(acpi_adr_space_type space_id, > + acpi_physical_address address, > + acpi_size length, u8 warn); > + > /* > * ACPI Memory management > */ > > >> >> > diff --git a/drivers/acpi/acpica/dsargs.c b/drivers/acpi/acpica/dsargs.c >> > index 8c7b997..42163d8 100644 >> > --- a/drivers/acpi/acpica/dsargs.c >> > +++ b/drivers/acpi/acpica/dsargs.c >> > @@ -387,5 +387,29 @@ acpi_status acpi_ds_get_region_arguments(union acpi_operand_object *obj_desc) >> > status = acpi_ds_execute_arguments(node, node->parent, >> > extra_desc->extra.aml_length, >> > extra_desc->extra.aml_start); >> > + if (ACPI_FAILURE(status)) { >> > + return_ACPI_STATUS(status); >> > + } >> > + >> > + /* Validate the region address/length via the host OS */ >> > + >> > + status = acpi_os_validate_address(obj_desc->region.space_id, >> > + obj_desc->region.address, >> > + (acpi_size) obj_desc->region.length, >> > + acpi_ut_get_node_name(node)); >> > + >> > + if (ACPI_FAILURE(status)) { >> > + /* >> > + * Invalid address/length. We will emit an error message and mark >> > + * the region as invalid, so that it will cause an additional error if >> > + * it is ever used. Then return AE_OK. >> > + */ >> > + ACPI_EXCEPTION((AE_INFO, status, >> > + "During address validation of OpRegion [%4.4s]", >> > + node->name.ascii)); >> > + obj_desc->common.flags |= AOPOBJ_INVALID; >> > + status = AE_OK; >> > + } >> > + >> > return_ACPI_STATUS(status); >> > } >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] 2011-12-12 12:52 ` Luca Tettamanti @ 2011-12-13 2:01 ` Lin Ming 0 siblings, 0 replies; 13+ messages in thread From: Lin Ming @ 2011-12-13 2:01 UTC (permalink / raw) To: Luca Tettamanti Cc: Moore, Robert, Jean Delvare, Nikolay Mikov, Thomas, linux-acpi@vger.kernel.org, lenb On Mon, 2011-12-12 at 20:52 +0800, Luca Tettamanti wrote: > On Fri, Dec 9, 2011 at 3:01 AM, Lin Ming <ming.m.lin@intel.com> wrote: > > Hi Luca, > > > > We have investigated this issue more and think that it's better to move > > the resource validation code into ACPICA core. > > > > Here is the new patch. > > > > The major changes include: > > - Remove acpi_os_validate_address/acpi_os_invalidate_address from osl.c. > > They are reimplemented in ACPICA core: > > acpi_ut_add_address_range/acpi_ut_remove_address_range. > > > > - Add new interface for drivers to check resource conflict: > > acpi_check_address_range > > > > Could you help to test it? > > Sure, works fine: > Tested-by: Luca Tettamanti <kronos.it@gmail.com> Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-01-11 13:01 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-28 20:53 [BUG] ACPI resource validation not working [Was: Re: ITE 8728F] Luca Tettamanti 2011-11-28 22:56 ` Moore, Robert 2011-11-29 1:48 ` Lin Ming 2011-11-29 13:18 ` Luca Tettamanti 2011-11-29 13:44 ` Lin Ming 2011-11-30 9:07 ` Jean Delvare 2011-12-09 2:01 ` Lin Ming 2011-12-12 9:37 ` Jean Delvare 2011-12-12 12:42 ` Lin Ming 2011-12-12 20:12 ` Jean Delvare 2012-01-11 13:01 ` Jean Delvare 2011-12-12 12:52 ` Luca Tettamanti 2011-12-13 2:01 ` Lin Ming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).