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

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

[Lin Ming]

    Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"
    
    This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18.
    
    The quick fix of bug 13620 would be to revert the change.
    http://bugzilla.kernel.org/show_bug.cgi?id=13620
    
    Also, see the commit df92e695998e1bc6e426a840eb86d6d1ee87e2a5
    "ACPI: track opregion names to avoid driver resource conflicts."
    
    But there are problems we need to address:
    
    1. We need to enhance the mechanism of avoiding driver resource conflicts
       to base a "resource" on Field definitions instead of Operation Region definitions.
    
    2. For dynamic region, we need an interface to call when an operation region (field) is deleted,
       in order to delete it from the resource list.
    
    3. If the same region is created and added to resource list over and over again,
       this is have the potential to be a memory leak by growing the list every time
    
    CC: Thomas Renninger <trenn@suse.de>
    Signed-off-by: Lin Ming <ming.m.lin@intel.com>
    Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
 drivers/acpi/acpica/acobject.h |    1 +
 drivers/acpi/acpica/dsopcode.c |   24 ++++++++++++++++++++++++
 drivers/acpi/acpica/exfldio.c  |    6 ++++++
 include/acpi/acpiosxf.h        |    4 ++++
 4 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h
index 544dcf8..eb6f038 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -97,6 +97,7 @@
 #define AOPOBJ_OBJECT_INITIALIZED   0x08
 #define AOPOBJ_SETUP_COMPLETE       0x10
 #define AOPOBJ_SINGLE_DATUM         0x20
+#define AOPOBJ_INVALID              0x40	/* Used if host OS won't allow an op_region address */
 
 /******************************************************************************
  *
diff --git a/drivers/acpi/acpica/dsopcode.c b/drivers/acpi/acpica/dsopcode.c
index 584d766..b79978f 100644
--- a/drivers/acpi/acpica/dsopcode.c
+++ b/drivers/acpi/acpica/dsopcode.c
@@ -397,6 +397,30 @@ acpi_status acpi_ds_get_region_arguments(union acpi_operand_object *obj_desc)
 	status = acpi_ds_execute_arguments(node, acpi_ns_get_parent_node(node),
 					   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);
 }
 
diff --git a/drivers/acpi/acpica/exfldio.c b/drivers/acpi/acpica/exfldio.c
index d4075b8..6687be1 100644
--- a/drivers/acpi/acpica/exfldio.c
+++ b/drivers/acpi/acpica/exfldio.c
@@ -113,6 +113,12 @@ acpi_ex_setup_region(union acpi_operand_object *obj_desc,
 		}
 	}
 
+	/* Exit if Address/Length have been disallowed by the host OS */
+
+	if (rgn_desc->common.flags & AOPOBJ_INVALID) {
+		return_ACPI_STATUS(AE_AML_ILLEGAL_ADDRESS);
+	}
+
 	/*
 	 * Exit now for SMBus address space, it has a non-linear address space
 	 * and the request cannot be directly validated
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 3e79859..ab0b85c 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -242,6 +242,10 @@ acpi_os_derive_pci_id(acpi_handle rhandle,
 acpi_status acpi_os_validate_interface(char *interface);
 acpi_status acpi_osi_invalidate(char* interface);
 
+acpi_status
+acpi_os_validate_address(u8 space_id, acpi_physical_address address,
+			 acpi_size length, char *name);
+
 u64 acpi_os_get_timer(void);
 
 acpi_status acpi_os_signal(u32 function, void *info);

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

[Thomas Renninger]

Hi Lin,

thanks for adding me.
This is not "that" sever, but IMO this one should also be submitted
to 2.6.30 stable kernels as it is a riskless revert of a patch
fixing a regression.

On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
>     Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"
>     
>     This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18.
>     
>     The quick fix of bug 13620 would be to revert the change.
>     http://bugzilla.kernel.org/show_bug.cgi?id=13620
>     
>     Also, see the commit df92e695998e1bc6e426a840eb86d6d1ee87e2a5
>     "ACPI: track opregion names to avoid driver resource conflicts."
>     
>     But there are problems we need to address:
>     
>     1. We need to enhance the mechanism of avoiding driver resource
>     conflicts
>        to base a "resource" on Field definitions instead of Operation Region
>        definitions.
Good idea, this could avoid some false positive detected region conflicts.
>     
>     2. For dynamic region, we need an interface to call when an operation
>     region (field) is deleted,
>        in order to delete it from the resource list.
>     
>     3. If the same region is created and added to resource list over and
>     over again,
>        this is have the potential to be a memory leak by growing the list
>        every time
Region or field or both?
How can this happen, can you show a little ASL snippet or explain this a bit 
more detailed, please. I do not fully understand what is meant in 3.

Thanks,

  Thomas

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

[Lin Ming]

On Wed, 2009-07-01 at 16:56 +0800, Thomas Renninger wrote:
> Hi Lin,
> 
> thanks for adding me.
> This is not "that" sever, but IMO this one should also be submitted
> to 2.6.30 stable kernels as it is a riskless revert of a patch
> fixing a regression.
> 
> On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
> >     Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"
> >     
> >     This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18.
> >     
> >     The quick fix of bug 13620 would be to revert the change.
> >     http://bugzilla.kernel.org/show_bug.cgi?id=13620
> >     
> >     Also, see the commit df92e695998e1bc6e426a840eb86d6d1ee87e2a5
> >     "ACPI: track opregion names to avoid driver resource conflicts."
> >     
> >     But there are problems we need to address:
> >     
> >     1. We need to enhance the mechanism of avoiding driver resource
> >     conflicts
> >        to base a "resource" on Field definitions instead of Operation Region
> >        definitions.
> Good idea, this could avoid some false positive detected region conflicts.
> >     
> >     2. For dynamic region, we need an interface to call when an operation
> >     region (field) is deleted,
> >        in order to delete it from the resource list.
> >     
> >     3. If the same region is created and added to resource list over and
> >     over again,
> >        this is have the potential to be a memory leak by growing the list
> >        every time
> Region or field or both?

Currently, it's region.
If we change the mechanism to base on "Field", as item 1 above, then
it's field.

> How can this happen, can you show a little ASL snippet or explain this a bit 
> more detailed, please. I do not fully understand what is meant in 3.

For example, the dynamic region which defined in a method,

Method(m000)
{
	OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF)
	.....
}

If method m000 is called multiple times, then the region xxxx will be
inserted to the resource list again and again.

So we need item 2 above to delete the dynamic region from the resource
list.

Thanks for comments.

Lin Ming

> 
> Thanks,
> 
>   Thomas

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

[Thomas Renninger]

On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
> On Wed, 2009-07-01 at 16:56 +0800, Thomas Renninger wrote:
> > Hi Lin,
> > 
> > thanks for adding me.
> > This is not "that" sever, but IMO this one should also be submitted
> > to 2.6.30 stable kernels as it is a riskless revert of a patch
> > fixing a regression.
> > 
> > On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
> > >     Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"
> > >     
> > >     This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18.
> > >     
> > >     The quick fix of bug 13620 would be to revert the change.
> > >     http://bugzilla.kernel.org/show_bug.cgi?id=13620
> > >     
> > >     Also, see the commit df92e695998e1bc6e426a840eb86d6d1ee87e2a5
> > >     "ACPI: track opregion names to avoid driver resource conflicts."
> > >     
> > >     But there are problems we need to address:
> > >     
> > >     1. We need to enhance the mechanism of avoiding driver resource
> > >     conflicts
> > >        to base a "resource" on Field definitions instead of Operation 
Region
> > >        definitions.
> > Good idea, this could avoid some false positive detected region conflicts.
> > >     
> > >     2. For dynamic region, we need an interface to call when an 
operation
> > >     region (field) is deleted,
> > >        in order to delete it from the resource list.
> > >     
> > >     3. If the same region is created and added to resource list over and
> > >     over again,
> > >        this is have the potential to be a memory leak by growing the 
list
> > >        every time
> > Region or field or both?
> 
> Currently, it's region.
> If we change the mechanism to base on "Field", as item 1 above, then
> it's field.
> 
> > How can this happen, can you show a little ASL snippet or explain this a 
bit 
> > more detailed, please. I do not fully understand what is meant in 3.
> 
> For example, the dynamic region which defined in a method,
> 
> Method(m000)
> {
> 	OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF)
> 	.....
> }
> 
> If method m000 is called multiple times, then the region xxxx will be
> inserted to the resource list again and again.
> 
> So we need item 2 above to delete the dynamic region from the resource
> list.
Ouch, I thought OperationRegions must be declared in global namespace.
I agree, we have a memleak and this looks rather sever.
Grmpfl, that makes the resource conflict checking even more ugly.
Thanks for spotting this,

     Thomas

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

[Moore, Robert]

>-----Original Message-----
>From: Thomas Renninger [mailto:trenn@suse.de]
>Sent: Wednesday, July 01, 2009 2:35 AM
>To: Lin, Ming M
>Cc: Len Brown; Moore, Robert; Zhao, Yakui; linux-acpi
>Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete
>acpi_os_validate_address interface"
>
>On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
>> On Wed, 2009-07-01 at 16:56 +0800, Thomas Renninger wrote:
>> > Hi Lin,
>> >
>> > thanks for adding me.
>> > This is not "that" sever, but IMO this one should also be submitted
>> > to 2.6.30 stable kernels as it is a riskless revert of a patch
>> > fixing a regression.
>> >
>> > On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
>> > >     Revert "ACPICA: Remove obsolete acpi_os_validate_address
>interface"
>> > >
>> > >     This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18.
>> > >
>> > >     The quick fix of bug 13620 would be to revert the change.
>> > >     http://bugzilla.kernel.org/show_bug.cgi?id=13620
>> > >
>> > >     Also, see the commit df92e695998e1bc6e426a840eb86d6d1ee87e2a5
>> > >     "ACPI: track opregion names to avoid driver resource conflicts."
>> > >
>> > >     But there are problems we need to address:
>> > >
>> > >     1. We need to enhance the mechanism of avoiding driver resource
>> > >     conflicts
>> > >        to base a "resource" on Field definitions instead of Operation
>Region
>> > >        definitions.
>> > Good idea, this could avoid some false positive detected region
>conflicts.
>> > >
>> > >     2. For dynamic region, we need an interface to call when an
>operation
>> > >     region (field) is deleted,
>> > >        in order to delete it from the resource list.
>> > >
>> > >     3. If the same region is created and added to resource list over
>and
>> > >     over again,
>> > >        this is have the potential to be a memory leak by growing the
>list
>> > >        every time
>> > Region or field or both?
>>
>> Currently, it's region.
>> If we change the mechanism to base on "Field", as item 1 above, then
>> it's field.
>>
>> > How can this happen, can you show a little ASL snippet or explain this
>a
>bit
>> > more detailed, please. I do not fully understand what is meant in 3.
>>
>> For example, the dynamic region which defined in a method,
>>
>> Method(m000)
>> {
>> 	OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF)
>> 	.....
>> }
>>
>> If method m000 is called multiple times, then the region xxxx will be
>> inserted to the resource list again and again.
>>
>> So we need item 2 above to delete the dynamic region from the resource
>> list.
>Ouch, I thought OperationRegions must be declared in global namespace.
>I agree, we have a memleak and this looks rather sever.
>Grmpfl, that makes the resource conflict checking even more ugly.
>Thanks for spotting this,
>
>     Thomas


OperationRegions/Fields can be dynamically created/deleted in two ways:
1) Control method execution
2) Dynamic ACPI table load/unload (for example, hotplug)

So, in order to track this, the resource list must also be dynamic, with the ability to add and delete entries.

It begins to sound like the resource list is becoming a "mini-namespace" that consists of only ACPI field objects. Might it not be more efficient to simply query the existing ACPI namespace when you need to? A walk_namespace for field objects would give you the same information as the resource list, and it is automatically guaranteed to be current.

However, the may be some more fundamental issues. I have not looked at exactly how the resource list is used, but since the list is dynamic, if the worry concerns address collisions between a driver and the AML interpreter, it may not be enough to simply check for this at the time the driver is loaded. You would really need to check before *every* I/O or memory access. Which does not sound feasible.

I guess that I would like to understand the exact issue(s) that are behind the creation of the resource list in the first place. AFAIK, any AML code that reads/writes to operation regions usually assumes exclusive access to these regions. The ACPI Global Lock is used when exclusive access cannot be assumed.

Bob

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

[Thomas Renninger]

On Wednesday 01 July 2009 05:29:57 pm Moore, Robert wrote:
> >-----Original Message-----
> >From: Thomas Renninger [mailto:trenn@suse.de]
> >Sent: Wednesday, July 01, 2009 2:35 AM
> >To: Lin, Ming M
> >Cc: Len Brown; Moore, Robert; Zhao, Yakui; linux-acpi
> >Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete
> >acpi_os_validate_address interface"
> >
> >On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
> >> On Wed, 2009-07-01 at 16:56 +0800, Thomas Renninger wrote:
> >> > Hi Lin,
> >> >
> >> > thanks for adding me.
> >> > This is not "that" sever, but IMO this one should also be submitted
> >> > to 2.6.30 stable kernels as it is a riskless revert of a patch
> >> > fixing a regression.
> >> >
> >> > On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
> >> > >     Revert "ACPICA: Remove obsolete acpi_os_validate_address
> >
> >interface"
> >
> >> > >     This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18.
> >> > >
> >> > >     The quick fix of bug 13620 would be to revert the change.
> >> > >     http://bugzilla.kernel.org/show_bug.cgi?id=13620
> >> > >
> >> > >     Also, see the commit df92e695998e1bc6e426a840eb86d6d1ee87e2a5
> >> > >     "ACPI: track opregion names to avoid driver resource conflicts."
> >> > >

< Cut out some badly formatted text >

Jean, for you info: We have two problems here, not sure you saw this:
1) The acpica method to tell the os which IO addresses might get used got reverted,
     thus the acpi_enforce_resources=strict/lax/no functionality does not exist any more.
2) Our, or say may part of the acpi_enforce_resources= implementation introduces a
     sever memleak. Opregion declarations can be inside of functions and can be called
     again and again. The list I introduced may increase forever if often called ACPI functions
     include OpRegion declarations...
> >> For example, the dynamic region which defined in a method,
> >>
> >> Method(m000)
> >> {
> >> 	OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF)
> >> 	.....
> >> }
> >>
> >> If method m000 is called multiple times, then the region xxxx will be
> >> inserted to the resource list again and again.
> >>
> >> So we need item 2 above to delete the dynamic region from the resource
> >> list.
> >
> >Ouch, I thought OperationRegions must be declared in global namespace.
> >I agree, we have a memleak and this looks rather sever.

An idea for a quick fix which may be suitable for stable kernels (without
checking acpica code..):
Is it easily possible to check whether the Opregion is declared in global
namespace or inside a method at the place where acpi_os_validate_address is called
from acpica?
In the latter case it should not be called and we avoid the memleak and
could still detect most i2c/smbus/hwmon devices conflicts.

> >Grmpfl, that makes the resource conflict checking even more ugly.
> >Thanks for spotting this,
> >
> >     Thomas
>
> OperationRegions/Fields can be dynamically created/deleted in two ways:
> 1) Control method execution
> 2) Dynamic ACPI table load/unload (for example, hotplug)
>
> So, in order to track this, the resource list must also be dynamic, with
> the ability to add and delete entries.
>
> It begins to sound like the resource list is becoming a "mini-namespace"
> that consists of only ACPI field objects. Might it not be more efficient to
> simply query the existing ACPI namespace when you need to? A walk_namespace
> for field objects would give you the same information as the resource list,
> and it is automatically guaranteed to be current.
>
> However, the may be some more fundamental issues. I have not looked at
> exactly how the resource list is used, but since the list is dynamic, if
> the worry concerns address collisions between a driver and the AML
> interpreter, it may not be enough to simply check for this at the time the
> driver is loaded. You would really need to check before *every* I/O or
> memory access. Which does not sound feasible.
>
> I guess that I would like to understand the exact issue(s) that are behind
> the creation of the resource list in the first place. AFAIK, any AML code
> that reads/writes to operation regions usually assumes exclusive access to
> these regions. The ACPI Global Lock is used when exclusive access cannot be
> assumed.

Thinking a bit more about this problem is probably a good idea.
The resource checking  always was a workaround which may avoid loading of
drivers because Opregion declarations may never get used.
When I looked at DSDTs I had the problem that BIOSes declare all kind of
things like overlapping and double declarations. I initially wanted to
add things somehow to:
/proc/iomem and /proc/ioports, but I finally didn't see a way to do that.

    Thomas

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

[Moore, Robert]

could still detect most i2c/smbus/hwmon devices conflicts.

What exactly are these conflicts?


>-----Original Message-----
>From: Thomas Renninger [mailto:trenn@suse.de]
>Sent: Wednesday, July 01, 2009 2:20 PM
>To: Moore, Robert
>Cc: Lin, Ming M; Len Brown; Zhao, Yakui; linux-acpi; jdelvare@suse.de
>Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete
>acpi_os_validate_address interface"
>
>On Wednesday 01 July 2009 05:29:57 pm Moore, Robert wrote:
>> >-----Original Message-----
>> >From: Thomas Renninger [mailto:trenn@suse.de]
>> >Sent: Wednesday, July 01, 2009 2:35 AM
>> >To: Lin, Ming M
>> >Cc: Len Brown; Moore, Robert; Zhao, Yakui; linux-acpi
>> >Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete
>> >acpi_os_validate_address interface"
>> >
>> >On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
>> >> On Wed, 2009-07-01 at 16:56 +0800, Thomas Renninger wrote:
>> >> > Hi Lin,
>> >> >
>> >> > thanks for adding me.
>> >> > This is not "that" sever, but IMO this one should also be submitted
>> >> > to 2.6.30 stable kernels as it is a riskless revert of a patch
>> >> > fixing a regression.
>> >> >
>> >> > On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
>> >> > >     Revert "ACPICA: Remove obsolete acpi_os_validate_address
>> >
>> >interface"
>> >
>> >> > >     This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18.
>> >> > >
>> >> > >     The quick fix of bug 13620 would be to revert the change.
>> >> > >     http://bugzilla.kernel.org/show_bug.cgi?id=13620
>> >> > >
>> >> > >     Also, see the commit df92e695998e1bc6e426a840eb86d6d1ee87e2a5
>> >> > >     "ACPI: track opregion names to avoid driver resource
>conflicts."
>> >> > >
>
>< Cut out some badly formatted text >
>
>Jean, for you info: We have two problems here, not sure you saw this:
>1) The acpica method to tell the os which IO addresses might get used got
>reverted,
>     thus the acpi_enforce_resources=strict/lax/no functionality does not
>exist any more.
>2) Our, or say may part of the acpi_enforce_resources= implementation
>introduces a
>     sever memleak. Opregion declarations can be inside of functions and
>can be called
>     again and again. The list I introduced may increase forever if often
>called ACPI functions
>     include OpRegion declarations...
>> >> For example, the dynamic region which defined in a method,
>> >>
>> >> Method(m000)
>> >> {
>> >> 	OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF)
>> >> 	.....
>> >> }
>> >>
>> >> If method m000 is called multiple times, then the region xxxx will be
>> >> inserted to the resource list again and again.
>> >>
>> >> So we need item 2 above to delete the dynamic region from the resource
>> >> list.
>> >
>> >Ouch, I thought OperationRegions must be declared in global namespace.
>> >I agree, we have a memleak and this looks rather sever.
>
>An idea for a quick fix which may be suitable for stable kernels (without
>checking acpica code..):
>Is it easily possible to check whether the Opregion is declared in global
>namespace or inside a method at the place where acpi_os_validate_address is
>called
>from acpica?
>In the latter case it should not be called and we avoid the memleak and
>could still detect most i2c/smbus/hwmon devices conflicts.
>
>> >Grmpfl, that makes the resource conflict checking even more ugly.
>> >Thanks for spotting this,
>> >
>> >     Thomas
>>
>> OperationRegions/Fields can be dynamically created/deleted in two ways:
>> 1) Control method execution
>> 2) Dynamic ACPI table load/unload (for example, hotplug)
>>
>> So, in order to track this, the resource list must also be dynamic, with
>> the ability to add and delete entries.
>>
>> It begins to sound like the resource list is becoming a "mini-namespace"
>> that consists of only ACPI field objects. Might it not be more efficient
>to
>> simply query the existing ACPI namespace when you need to? A
>walk_namespace
>> for field objects would give you the same information as the resource
>list,
>> and it is automatically guaranteed to be current.
>>
>> However, the may be some more fundamental issues. I have not looked at
>> exactly how the resource list is used, but since the list is dynamic, if
>> the worry concerns address collisions between a driver and the AML
>> interpreter, it may not be enough to simply check for this at the time
>the
>> driver is loaded. You would really need to check before *every* I/O or
>> memory access. Which does not sound feasible.
>>
>> I guess that I would like to understand the exact issue(s) that are
>behind
>> the creation of the resource list in the first place. AFAIK, any AML code
>> that reads/writes to operation regions usually assumes exclusive access
>to
>> these regions. The ACPI Global Lock is used when exclusive access cannot
>be
>> assumed.
>
>Thinking a bit more about this problem is probably a good idea.
>The resource checking  always was a workaround which may avoid loading of
>drivers because Opregion declarations may never get used.
>When I looked at DSDTs I had the problem that BIOSes declare all kind of
>things like overlapping and double declarations. I initially wanted to
>add things somehow to:
>/proc/iomem and /proc/ioports, but I finally didn't see a way to do that.
>
>    Thomas

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

[Len Brown]

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.

thanks,
Len Brown, Intel Open Source Technology Center

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

[Lin Ming]

On Thu, 2009-07-02 at 10:03 +0800, Len Brown wrote: 
> I can't apply a patch that adds a known memory leak,
> even if it removes a conflict between drivers.
> 
> The reason is that there is a workaround for the driver conflict,
> but no workaround for the memory leak.

Here is a prototype simple patch, please review to see if it is the
right way to go.

1) add a new field "count" to struct acpi_res_list.

   When inserting, if the region(addr, len) is already in the resource 
   list, we just increase "count", otherwise, the region is inserted 
   with count=1.

   When deleting, the "count" is decreased, if it's decreased to 0,
   the region is deleted from the resource list.

   With "count", the region with same address and length can only be 
   inserted to the resource list once, so prevent potential memory leak.

2) add a new function acpi_os_invalidate_address, which is called when 
   region is deleted. 


diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c
index bc17103..96e26e7 100644
--- a/drivers/acpi/acpica/utdelete.c
+++ b/drivers/acpi/acpica/utdelete.c
@@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union acpi_operand_object *object)
 		ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS,
 				  "***** Region %p\n", object));
 
+		/* Invalidate the region address/length via the host OS */
+
+		acpi_os_invalidate_address(object->region.space_id,
+					  object->region.address,
+					  (acpi_size) object->region.length);
+
 		second_desc = acpi_ns_get_secondary_object(object);
 		if (second_desc) {
 			/*
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 7167071..620a65a 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -88,6 +88,7 @@ struct acpi_res_list {
 	char name[5];   /* only can have a length of 4 chars, make use of this
 			   one instead of res->name, no need to kalloc then */
 	struct list_head resource_list;
+	int count;
 };
 
 static LIST_HEAD(resource_list_head);
@@ -1333,6 +1334,88 @@ acpi_os_validate_interface (char *interface)
 	return AE_SUPPORT;
 }
 
+static inline int acpi_res_list_add(struct acpi_res_list *res)
+{
+	struct acpi_res_list *res_list_elem;
+
+	list_for_each_entry(res_list_elem, &resource_list_head,
+			    resource_list) {
+
+		if (res->resource_type == res_list_elem->resource_type &&
+		    res->start == res_list_elem->start &&
+		    res->end == res_list_elem->end) {
+
+			/*
+			 * The Region(addr,len) already exist in the list,
+			 * just increase the count
+			 */
+
+			res_list_elem->count++;
+			return 0;
+		}
+	}
+
+	res->count = 1;
+	list_add(&res->resource_list, &resource_list_head);
+	return 1;
+}
+
+static inline void acpi_res_list_del(struct acpi_res_list *res)
+{
+	struct acpi_res_list *res_list_elem;
+
+	list_for_each_entry(res_list_elem, &resource_list_head,
+			    resource_list) {
+
+		if (res->resource_type == res_list_elem->resource_type &&
+		    res->start == res_list_elem->start &&
+		    res->end == res_list_elem->end) {
+
+			/*
+			 * If the res count is decreased to 0, remove and free it
+			 */
+
+			if (--res_list_elem->count == 0) {
+				list_del(&res_list_elem->resource_list);
+				kfree(res_list_elem);
+			}
+			return;
+		}
+	}
+}
+
+acpi_status
+acpi_os_invalidate_address(
+    u8                   space_id,
+    acpi_physical_address   address,
+    acpi_size               length)
+{
+	struct acpi_res_list res;
+
+	switch (space_id) {
+	case ACPI_ADR_SPACE_SYSTEM_IO:
+	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
+		/* Only interference checks against SystemIO and SytemMemory
+		   are needed */
+		res.start = address;
+		res.end = address + length - 1;
+		res.resource_type = space_id;
+		spin_lock(&acpi_res_lock);
+		acpi_res_list_del(&res);
+		spin_unlock(&acpi_res_lock);
+		break;
+	case ACPI_ADR_SPACE_PCI_CONFIG:
+	case ACPI_ADR_SPACE_EC:
+	case ACPI_ADR_SPACE_SMBUS:
+	case ACPI_ADR_SPACE_CMOS:
+	case ACPI_ADR_SPACE_PCI_BAR_TARGET:
+	case ACPI_ADR_SPACE_DATA_TABLE:
+	case ACPI_ADR_SPACE_FIXED_HARDWARE:
+		break;
+	}
+	return AE_OK;
+}
+
 /******************************************************************************
  *
  * FUNCTION:    acpi_os_validate_address
@@ -1357,6 +1440,7 @@ acpi_os_validate_address (
     char *name)
 {
 	struct acpi_res_list *res;
+	int added;
 	if (acpi_enforce_resources == ENFORCE_RESOURCES_NO)
 		return AE_OK;
 
@@ -1374,14 +1458,17 @@ acpi_os_validate_address (
 		res->end = address + length - 1;
 		res->resource_type = space_id;
 		spin_lock(&acpi_res_lock);
-		list_add(&res->resource_list, &resource_list_head);
+		added = acpi_res_list_add(res);
 		spin_unlock(&acpi_res_lock);
-		pr_debug("Added %s resource: start: 0x%llx, end: 0x%llx, "
-			 "name: %s\n", (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
+		pr_debug("%s %s resource: start: 0x%llx, end: 0x%llx, "
+			 "name: %s\n", added ? "Added" : "Already exist",
+			 (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
 			 ? "SystemIO" : "System Memory",
 			 (unsigned long long)res->start,
 			 (unsigned long long)res->end,
 			 res->name);
+		if (!added)
+			kfree(res);
 		break;
 	case ACPI_ADR_SPACE_PCI_CONFIG:
 	case ACPI_ADR_SPACE_EC:
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index ab0b85c..14d40bf 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -246,6 +246,10 @@ acpi_status
 acpi_os_validate_address(u8 space_id, acpi_physical_address address,
 			 acpi_size length, char *name);
 
+acpi_status
+acpi_os_invalidate_address(u8 space_id, acpi_physical_address address,
+			 acpi_size length);
+
 u64 acpi_os_get_timer(void);
 
 acpi_status acpi_os_signal(u32 function, void *info);

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

[Moore, Robert]

Before we start extending the existing mechanisms, I think we need to really think about a better solution.

Since there already appears to be a workaround for the problem, then this should give us some time to analyze this.

>-----Original Message-----
>From: Lin, Ming M
>Sent: Wednesday, July 01, 2009 11:27 PM
>To: Len Brown
>Cc: Thomas Renninger; Moore, Robert; Zhao, Yakui; linux-acpi
>Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete
>acpi_os_validate_address interface"
>
>On Thu, 2009-07-02 at 10:03 +0800, Len Brown wrote:
>> I can't apply a patch that adds a known memory leak,
>> even if it removes a conflict between drivers.
>>
>> The reason is that there is a workaround for the driver conflict,
>> but no workaround for the memory leak.
>
>Here is a prototype simple patch, please review to see if it is the
>right way to go.
>
>1) add a new field "count" to struct acpi_res_list.
>
>   When inserting, if the region(addr, len) is already in the resource
>   list, we just increase "count", otherwise, the region is inserted
>   with count=1.
>
>   When deleting, the "count" is decreased, if it's decreased to 0,
>   the region is deleted from the resource list.
>
>   With "count", the region with same address and length can only be
>   inserted to the resource list once, so prevent potential memory leak.
>
>2) add a new function acpi_os_invalidate_address, which is called when
>   region is deleted.
>

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

[Jean Delvare]

Hi Robert,

Le jeudi 02 juillet 2009, Moore, Robert a écrit :
> could still detect most i2c/smbus/hwmon devices conflicts.
> 
> What exactly are these conflicts?

Native Linux drivers trying to access devices which ACPI also wants
to access. Most frequently these are SMBus master drivers or hardware
monitoring drivers (for Super I/O chips) but virtually any other
device type could be affected, due to the fact that ACPI I/O
resources do not show up in /proc/ioports and /proc/iomem.

The workaround written by Thomas would let the native drivers which
are more frequently affected by the problem ask the ACPI subsystem,
on a voluntary basis, if a given I/O range is safe to use. While not
perfect, it worked reasonably well in practice.

Your upstream commit broke this safety mechanism, and now all the
past bugs resulting of native driver vs. ACPI conflicts show up
again, so you will have to fix it now. Either by reverting your
commit, or by coming up with a better safety. I don't know much about
ACPI so I can't really comment on a technical perspective. All I care
about is the functionality: either concurrent access from native
drivers and ACPI must be doable safely, or it must be prevented
altogether (which Thomas' code was doing.)

I will be happy to answer your questions about the native Linux
drivers for SMBus master devices and hardware monitoring devices.
I can also help with testing if you want.

-- 
Jean Delvare
Suse L3
--
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

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

[Jean Delvare]

Le mercredi 01 juillet 2009, Thomas Renninger a écrit :
> On Wednesday 01 July 2009 05:29:57 pm Moore, Robert wrote:
> > >-----Original Message-----
> > >From: Thomas Renninger [mailto:trenn@suse.de]
> > >Ouch, I thought OperationRegions must be declared in global namespace.
> > >I agree, we have a memleak and this looks rather sever.
> 
> An idea for a quick fix which may be suitable for stable kernels (without
> checking acpica code..):
> Is it easily possible to check whether the Opregion is declared in global
> namespace or inside a method at the place where acpi_os_validate_address is called
> from acpica?
> In the latter case it should not be called and we avoid the memleak and
> could still detect most i2c/smbus/hwmon devices conflicts.

Can't you just walk the list before you add an entry, and if the
entry is already present, do not add it again?

> > >Grmpfl, that makes the resource conflict checking even more ugly.
> > >Thanks for spotting this,
> > >
> > >     Thomas
> >
> > OperationRegions/Fields can be dynamically created/deleted in two ways:
> > 1) Control method execution
> > 2) Dynamic ACPI table load/unload (for example, hotplug)
> >
> > So, in order to track this, the resource list must also be dynamic, with
> > the ability to add and delete entries.
> >
> > It begins to sound like the resource list is becoming a "mini-namespace"
> > that consists of only ACPI field objects. Might it not be more efficient to
> > simply query the existing ACPI namespace when you need to? A walk_namespace
> > for field objects would give you the same information as the resource list,
> > and it is automatically guaranteed to be current.
> >
> > However, the may be some more fundamental issues. I have not looked at
> > exactly how the resource list is used, but since the list is dynamic, if
> > the worry concerns address collisions between a driver and the AML
> > interpreter, it may not be enough to simply check for this at the time the
> > driver is loaded. You would really need to check before *every* I/O or
> > memory access. Which does not sound feasible.

This is indeed not feasible, the cost would be much too high. Not to
mention it would still be racy by design.

Please remember that Thomas' code was meant to solve real-world
problems which had been there for long and nobody else was able to
solve so far. It wasn't meant to be bullet-proof, it was not perfect,
but it did help in practice.

> > I guess that I would like to understand the exact issue(s) that are behind
> > the creation of the resource list in the first place. AFAIK, any AML code
> > that reads/writes to operation regions usually assumes exclusive access to
> > these regions. The ACPI Global Lock is used when exclusive access cannot be
> > assumed.

I could change the affected native drivers to take the ACPI Global
Lock if needed. But what guarantees that the ACPI code which accesses
the device in question is taking the ACPI Global Lock too? It can
only work if everybody plays the game. My understanding was that most
ACPI implementation did not take the ACPI Global Lock when accessing
the devices and thus this approach wouldn't work in practice. Was I
wrong?

-- 
Jean Delvare
Suse L3
--
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

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

[Thomas Renninger]

On Thursday 02 July 2009 08:27:25 Lin Ming wrote:
> On Thu, 2009-07-02 at 10:03 +0800, Len Brown wrote: 
> > I can't apply a patch that adds a known memory leak,
> > even if it removes a conflict between drivers.
> > 
> > The reason is that there is a workaround for the driver conflict,
> > but no workaround for the memory leak.
> 
> Here is a prototype simple patch, please review to see if it is the
> right way to go.
> 
> 1) add a new field "count" to struct acpi_res_list.
> 
>    When inserting, if the region(addr, len) is already in the resource 
>    list, we just increase "count", otherwise, the region is inserted 
>    with count=1.
> 
>    When deleting, the "count" is decreased, if it's decreased to 0,
>    the region is deleted from the resource list.
> 
>    With "count", the region with same address and length can only be 
>    inserted to the resource list once, so prevent potential memory leak.
Is the counting really needed?
An OpRegion inside a method should get deleted when the function is exited
and acpi_os_invalidate_address will get called.

I like it. Thanks for looking into that!

   Thomas
> 
> 2) add a new function acpi_os_invalidate_address, which is called when 
>    region is deleted. 
> 
> 
> diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c
> index bc17103..96e26e7 100644
> --- a/drivers/acpi/acpica/utdelete.c
> +++ b/drivers/acpi/acpica/utdelete.c
> @@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union 
acpi_operand_object *object)
>  		ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS,
>  				  "***** Region %p\n", object));
>  
> +		/* Invalidate the region address/length via the host OS */
> +
> +		acpi_os_invalidate_address(object->region.space_id,
> +					  object->region.address,
> +					  (acpi_size) object->region.length);
> +
>  		second_desc = acpi_ns_get_secondary_object(object);
>  		if (second_desc) {
>  			/*
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 7167071..620a65a 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -88,6 +88,7 @@ struct acpi_res_list {
>  	char name[5];   /* only can have a length of 4 chars, make use of this
>  			   one instead of res->name, no need to kalloc then */
>  	struct list_head resource_list;
> +	int count;
>  };
>  
>  static LIST_HEAD(resource_list_head);
> @@ -1333,6 +1334,88 @@ acpi_os_validate_interface (char *interface)
>  	return AE_SUPPORT;
>  }
>  
> +static inline int acpi_res_list_add(struct acpi_res_list *res)
> +{
> +	struct acpi_res_list *res_list_elem;
> +
> +	list_for_each_entry(res_list_elem, &resource_list_head,
> +			    resource_list) {
> +
> +		if (res->resource_type == res_list_elem->resource_type &&
> +		    res->start == res_list_elem->start &&
> +		    res->end == res_list_elem->end) {
> +
> +			/*
> +			 * The Region(addr,len) already exist in the list,
> +			 * just increase the count
> +			 */
> +
> +			res_list_elem->count++;
> +			return 0;
> +		}
> +	}
> +
> +	res->count = 1;
> +	list_add(&res->resource_list, &resource_list_head);
> +	return 1;
> +}
> +
> +static inline void acpi_res_list_del(struct acpi_res_list *res)
> +{
> +	struct acpi_res_list *res_list_elem;
> +
> +	list_for_each_entry(res_list_elem, &resource_list_head,
> +			    resource_list) {
> +
> +		if (res->resource_type == res_list_elem->resource_type &&
> +		    res->start == res_list_elem->start &&
> +		    res->end == res_list_elem->end) {
> +
> +			/*
> +			 * If the res count is decreased to 0, remove and free it
> +			 */
> +
> +			if (--res_list_elem->count == 0) {
> +				list_del(&res_list_elem->resource_list);
> +				kfree(res_list_elem);
> +			}
> +			return;
> +		}
> +	}
> +}
> +
> +acpi_status
> +acpi_os_invalidate_address(
> +    u8                   space_id,
> +    acpi_physical_address   address,
> +    acpi_size               length)
> +{
> +	struct acpi_res_list res;
> +
> +	switch (space_id) {
> +	case ACPI_ADR_SPACE_SYSTEM_IO:
> +	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> +		/* Only interference checks against SystemIO and SytemMemory
> +		   are needed */
> +		res.start = address;
> +		res.end = address + length - 1;
> +		res.resource_type = space_id;
> +		spin_lock(&acpi_res_lock);
> +		acpi_res_list_del(&res);
> +		spin_unlock(&acpi_res_lock);
> +		break;
> +	case ACPI_ADR_SPACE_PCI_CONFIG:
> +	case ACPI_ADR_SPACE_EC:
> +	case ACPI_ADR_SPACE_SMBUS:
> +	case ACPI_ADR_SPACE_CMOS:
> +	case ACPI_ADR_SPACE_PCI_BAR_TARGET:
> +	case ACPI_ADR_SPACE_DATA_TABLE:
> +	case ACPI_ADR_SPACE_FIXED_HARDWARE:
> +		break;
> +	}
> +	return AE_OK;
> +}
> +
>  
/******************************************************************************
>   *
>   * FUNCTION:    acpi_os_validate_address
> @@ -1357,6 +1440,7 @@ acpi_os_validate_address (
>      char *name)
>  {
>  	struct acpi_res_list *res;
> +	int added;
>  	if (acpi_enforce_resources == ENFORCE_RESOURCES_NO)
>  		return AE_OK;
>  
> @@ -1374,14 +1458,17 @@ acpi_os_validate_address (
>  		res->end = address + length - 1;
>  		res->resource_type = space_id;
>  		spin_lock(&acpi_res_lock);
> -		list_add(&res->resource_list, &resource_list_head);
> +		added = acpi_res_list_add(res);
>  		spin_unlock(&acpi_res_lock);
> -		pr_debug("Added %s resource: start: 0x%llx, end: 0x%llx, "
> -			 "name: %s\n", (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> +		pr_debug("%s %s resource: start: 0x%llx, end: 0x%llx, "
> +			 "name: %s\n", added ? "Added" : "Already exist",
> +			 (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
>  			 ? "SystemIO" : "System Memory",
>  			 (unsigned long long)res->start,
>  			 (unsigned long long)res->end,
>  			 res->name);
> +		if (!added)
> +			kfree(res);
>  		break;
>  	case ACPI_ADR_SPACE_PCI_CONFIG:
>  	case ACPI_ADR_SPACE_EC:
> diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
> index ab0b85c..14d40bf 100644
> --- a/include/acpi/acpiosxf.h
> +++ b/include/acpi/acpiosxf.h
> @@ -246,6 +246,10 @@ acpi_status
>  acpi_os_validate_address(u8 space_id, acpi_physical_address address,
>  			 acpi_size length, char *name);
>  
> +acpi_status
> +acpi_os_invalidate_address(u8 space_id, acpi_physical_address address,
> +			 acpi_size length);
> +
>  u64 acpi_os_get_timer(void);
>  
>  acpi_status acpi_os_signal(u32 function, void *info);
> 
> 
> 

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

[Thomas Renninger]

On Thursday 02 July 2009 08:42:38 Moore, Robert wrote:
> Before we start extending the existing mechanisms, I think we need to really
> think about a better solution.
Yes, but we still need a fix for older stable kernels which are the ones in
production.

   Thomas

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

[Thomas Renninger]

On Thursday 02 July 2009 04:03:55 Len Brown wrote:
> I can't apply a patch that adds a known memory leak,
> even if it removes a conflict between drivers.
Can you revert it with Lin's added on top (after discussing it), please.
It's always a good idea (or a must) to have a stable patch in mainline first.
I doubt we find a proper solution for 2.6.31.

> The reason is that there is a workaround for the driver conflict,
> but no workaround for the memory leak.
There is one now.

Thanks,

   Thomas

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

[Moore, Robert]

>-----Original Message-----
>From: Jean Delvare [mailto:jdelvare@suse.de]
...
>Native Linux drivers trying to access devices which ACPI also wants
>to access. Most frequently these are SMBus master drivers or hardware
>monitoring drivers (for Super I/O chips) but virtually any other
>device type could be affected, due to the fact that ACPI I/O
>resources do not show up in /proc/ioports and /proc/iomem.


I have some concern with this. The implication is that there is a serious hole in ACPI that can only be solved by monitoring exactly what the AML code is doing, because access to resources may conflict with device drivers. I have to think that if this were truly the case, the operating system vendors and the ACPI contributors would have pushed a fix for this problem into the ACPI specification by now (in the 13 years since the first release of ACPI).

The SMBus driver issue is very interesting. I have not heard of any conflicts between AML code and SMBus drivers on other operating systems, nor have we received any other complaints about removing the acpi_os_validate_address interface. I wonder if the Linux SMBus driver is doing something that it should not be doing.

I'd like to see a concrete example of the problem. If someone can send me a DSDT that contains such an example, I would appreciate it. I want to see the exact ASL code that is causing the problem along with the driver code that conflicts with it. This information will help me understand the problem in relation to the ACPICA code, and I can also present this to other ACPI and BIOS experts for their opinions.

As far as an immediate fix for the problem, I suggest that you think about using acpi_walk_namespace to retrieve all objects of type Region (or field). It seems a waste to build an entire list of objects that duplicates information that already exists in the namespace. 

Bob

>-----Original Message-----
>From: Thomas Renninger [mailto:trenn@suse.de]
>Sent: Thursday, July 02, 2009 3:22 AM
>To: Len Brown
>Cc: Lin, Ming M; Moore, Robert; Zhao, Yakui; linux-acpi
>Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsolete
>acpi_os_validate_address interface"
>
>On Thursday 02 July 2009 04:03:55 Len Brown wrote:
>> I can't apply a patch that adds a known memory leak,
>> even if it removes a conflict between drivers.
>Can you revert it with Lin's added on top (after discussing it), please.
>It's always a good idea (or a must) to have a stable patch in mainline
>first.
>I doubt we find a proper solution for 2.6.31.
>
>> The reason is that there is a workaround for the driver conflict,
>> but no workaround for the memory leak.
>There is one now.
>
>Thanks,
>
>   Thomas

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

[Lin Ming]

On Thu, 2009-07-02 at 18:12 +0800, Thomas Renninger wrote:
> On Thursday 02 July 2009 08:27:25 Lin Ming wrote:
> > On Thu, 2009-07-02 at 10:03 +0800, Len Brown wrote: 
> > > I can't apply a patch that adds a known memory leak,
> > > even if it removes a conflict between drivers.
> > > 
> > > The reason is that there is a workaround for the driver conflict,
> > > but no workaround for the memory leak.
> > 
> > Here is a prototype simple patch, please review to see if it is the
> > right way to go.
> > 
> > 1) add a new field "count" to struct acpi_res_list.
> > 
> >    When inserting, if the region(addr, len) is already in the resource 
> >    list, we just increase "count", otherwise, the region is inserted 
> >    with count=1.
> > 
> >    When deleting, the "count" is decreased, if it's decreased to 0,
> >    the region is deleted from the resource list.
> > 
> >    With "count", the region with same address and length can only be 
> >    inserted to the resource list once, so prevent potential memory leak.
> Is the counting really needed?
> An OpRegion inside a method should get deleted when the function is exited
> and acpi_os_invalidate_address will get called.

I was considering maybe some strange BIOS defines a region in a method
with the same address and length of the global region, as below

OperationRegion (reg1, SystemMemory, 0xFED11000, 0xFF)

Method(m000)
{
    OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF)
    ....
}

Although, this is a really seldom case, 
but with the counting it would be more safe.

Lin Ming

> 
> I like it. Thanks for looking into that!
> 
>    Thomas
> > 
> > 2) add a new function acpi_os_invalidate_address, which is called when 
> >    region is deleted. 
> > 
> > 
> > diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c
> > index bc17103..96e26e7 100644
> > --- a/drivers/acpi/acpica/utdelete.c
> > +++ b/drivers/acpi/acpica/utdelete.c
> > @@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union 
> acpi_operand_object *object)
> >  		ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS,
> >  				  "***** Region %p\n", object));
> >  
> > +		/* Invalidate the region address/length via the host OS */
> > +
> > +		acpi_os_invalidate_address(object->region.space_id,
> > +					  object->region.address,
> > +					  (acpi_size) object->region.length);
> > +
> >  		second_desc = acpi_ns_get_secondary_object(object);
> >  		if (second_desc) {
> >  			/*
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 7167071..620a65a 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -88,6 +88,7 @@ struct acpi_res_list {
> >  	char name[5];   /* only can have a length of 4 chars, make use of this
> >  			   one instead of res->name, no need to kalloc then */
> >  	struct list_head resource_list;
> > +	int count;
> >  };
> >  
> >  static LIST_HEAD(resource_list_head);
> > @@ -1333,6 +1334,88 @@ acpi_os_validate_interface (char *interface)
> >  	return AE_SUPPORT;
> >  }
> >  
> > +static inline int acpi_res_list_add(struct acpi_res_list *res)
> > +{
> > +	struct acpi_res_list *res_list_elem;
> > +
> > +	list_for_each_entry(res_list_elem, &resource_list_head,
> > +			    resource_list) {
> > +
> > +		if (res->resource_type == res_list_elem->resource_type &&
> > +		    res->start == res_list_elem->start &&
> > +		    res->end == res_list_elem->end) {
> > +
> > +			/*
> > +			 * The Region(addr,len) already exist in the list,
> > +			 * just increase the count
> > +			 */
> > +
> > +			res_list_elem->count++;
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	res->count = 1;
> > +	list_add(&res->resource_list, &resource_list_head);
> > +	return 1;
> > +}
> > +
> > +static inline void acpi_res_list_del(struct acpi_res_list *res)
> > +{
> > +	struct acpi_res_list *res_list_elem;
> > +
> > +	list_for_each_entry(res_list_elem, &resource_list_head,
> > +			    resource_list) {
> > +
> > +		if (res->resource_type == res_list_elem->resource_type &&
> > +		    res->start == res_list_elem->start &&
> > +		    res->end == res_list_elem->end) {
> > +
> > +			/*
> > +			 * If the res count is decreased to 0, remove and free it
> > +			 */
> > +
> > +			if (--res_list_elem->count == 0) {
> > +				list_del(&res_list_elem->resource_list);
> > +				kfree(res_list_elem);
> > +			}
> > +			return;
> > +		}
> > +	}
> > +}
> > +
> > +acpi_status
> > +acpi_os_invalidate_address(
> > +    u8                   space_id,
> > +    acpi_physical_address   address,
> > +    acpi_size               length)
> > +{
> > +	struct acpi_res_list res;
> > +
> > +	switch (space_id) {
> > +	case ACPI_ADR_SPACE_SYSTEM_IO:
> > +	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> > +		/* Only interference checks against SystemIO and SytemMemory
> > +		   are needed */
> > +		res.start = address;
> > +		res.end = address + length - 1;
> > +		res.resource_type = space_id;
> > +		spin_lock(&acpi_res_lock);
> > +		acpi_res_list_del(&res);
> > +		spin_unlock(&acpi_res_lock);
> > +		break;
> > +	case ACPI_ADR_SPACE_PCI_CONFIG:
> > +	case ACPI_ADR_SPACE_EC:
> > +	case ACPI_ADR_SPACE_SMBUS:
> > +	case ACPI_ADR_SPACE_CMOS:
> > +	case ACPI_ADR_SPACE_PCI_BAR_TARGET:
> > +	case ACPI_ADR_SPACE_DATA_TABLE:
> > +	case ACPI_ADR_SPACE_FIXED_HARDWARE:
> > +		break;
> > +	}
> > +	return AE_OK;
> > +}
> > +
> >  
> /******************************************************************************
> >   *
> >   * FUNCTION:    acpi_os_validate_address
> > @@ -1357,6 +1440,7 @@ acpi_os_validate_address (
> >      char *name)
> >  {
> >  	struct acpi_res_list *res;
> > +	int added;
> >  	if (acpi_enforce_resources == ENFORCE_RESOURCES_NO)
> >  		return AE_OK;
> >  
> > @@ -1374,14 +1458,17 @@ acpi_os_validate_address (
> >  		res->end = address + length - 1;
> >  		res->resource_type = space_id;
> >  		spin_lock(&acpi_res_lock);
> > -		list_add(&res->resource_list, &resource_list_head);
> > +		added = acpi_res_list_add(res);
> >  		spin_unlock(&acpi_res_lock);
> > -		pr_debug("Added %s resource: start: 0x%llx, end: 0x%llx, "
> > -			 "name: %s\n", (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> > +		pr_debug("%s %s resource: start: 0x%llx, end: 0x%llx, "
> > +			 "name: %s\n", added ? "Added" : "Already exist",
> > +			 (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> >  			 ? "SystemIO" : "System Memory",
> >  			 (unsigned long long)res->start,
> >  			 (unsigned long long)res->end,
> >  			 res->name);
> > +		if (!added)
> > +			kfree(res);
> >  		break;
> >  	case ACPI_ADR_SPACE_PCI_CONFIG:
> >  	case ACPI_ADR_SPACE_EC:
> > diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
> > index ab0b85c..14d40bf 100644
> > --- a/include/acpi/acpiosxf.h
> > +++ b/include/acpi/acpiosxf.h
> > @@ -246,6 +246,10 @@ acpi_status
> >  acpi_os_validate_address(u8 space_id, acpi_physical_address address,
> >  			 acpi_size length, char *name);
> >  
> > +acpi_status
> > +acpi_os_invalidate_address(u8 space_id, acpi_physical_address address,
> > +			 acpi_size length);
> > +
> >  u64 acpi_os_get_timer(void);
> >  
> >  acpi_status acpi_os_signal(u32 function, void *info);
> > 
> > 
> > 
> 
> 

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

[Robert Hancock]

On 07/02/2009 09:49 AM, Moore, Robert wrote:
>> -----Original Message-----
>> From: Jean Delvare [mailto:jdelvare@suse.de]
> ...
>> Native Linux drivers trying to access devices which ACPI also wants
>> to access. Most frequently these are SMBus master drivers or hardware
>> monitoring drivers (for Super I/O chips) but virtually any other
>> device type could be affected, due to the fact that ACPI I/O
>> resources do not show up in /proc/ioports and /proc/iomem.
>
>
> I have some concern with this. The implication is that there is a serious hole in ACPI that can only be solved by monitoring exactly what the AML code is doing, because access to resources may conflict with device drivers. I have to think that if this were truly the case, the operating system vendors and the ACPI contributors would have pushed a fix for this problem into the ACPI specification by now (in the 13 years since the first release of ACPI).
>
> The SMBus driver issue is very interesting. I have not heard of any conflicts between AML code and SMBus drivers on other operating systems, nor have we received any other complaints about removing the acpi_os_validate_address interface. I wonder if the Linux SMBus driver is doing something that it should not be doing.
>
> I'd like to see a concrete example of the problem. If someone can send me a DSDT that contains such an example, I would appreciate it. I want to see the exact ASL code that is causing the problem along with the driver code that conflicts with it. This information will help me understand the problem in relation to the ACPICA code, and I can also present this to other ACPI and BIOS experts for their opinions.
>
> As far as an immediate fix for the problem, I suggest that you think about using acpi_walk_namespace to retrieve all objects of type Region (or field). It seems a waste to build an entire list of objects that duplicates information that already exists in the namespace.

My guess is that exactly the same thing will happen if you install 
add-on hardware monitoring software on Windows that loads drivers that 
access SMBus hardware directly on such systems where ACPI accesses the 
same devices. The ACPI code and the native driver will likely stomp on 
each other's hardware accesses. Likely on Linux it's more frequent that 
this kind of native hardware monitoring driver gets used (in some setups 
it seems they will auto-load by default..)

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

[Thomas Renninger]

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

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

[Lin Ming]

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

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

[Thomas Renninger]

On Tuesday 14 July 2009 04:28:56 am Lin Ming wrote:
> 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.
Ok, I see.
I thought it's ok to use it in drivers/acpi/*.
While I could get the type from acpi_get_object_info(...), I have no idea how
I could obtain the region info (address/length) without internal functions.
Hints appreciated.

I also still think your approach (delete list entries on OpRegion 
invalidation) is the better one. At least for:
  - fixing the memleak in older kernels
  - handle the reported regression(s) in .30 and .31.
Len?

Thanks,

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

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

[Jean Delvare]

[-- Attachment #1: Type: text/plain, Size: 9037 bytes --]

Hi Robert, Robert, all,

Sorry for the late answer. Busy at work then long vacation and here
I am again.

Le samedi 04 juillet 2009, Robert Hancock a écrit :
> On 07/02/2009 09:49 AM, Moore, Robert wrote:
> >> -----Original Message-----
> >> From: Jean Delvare [mailto:jdelvare@suse.de]
> > ...
> >> Native Linux drivers trying to access devices which ACPI also wants
> >> to access. Most frequently these are SMBus master drivers or hardware
> >> monitoring drivers (for Super I/O chips) but virtually any other
> >> device type could be affected, due to the fact that ACPI I/O
> >> resources do not show up in /proc/ioports and /proc/iomem.
> >
> >
> > I have some concern with this. The implication is that there is a
> > serious hole in ACPI that can only be solved by monitoring
> > exactly what the AML code is doing, because access to resources
> > may conflict with device drivers.

This is the basic idea, yes. That being said, if I/O resources used
by ACPI are properly declared, then the problem is partly solved:
ACPI will take care of the devices (either through a dedicated driver
or through a generic ACPI driver such as "thermal") and native
drivers will not attempt to access the device.

Problems arise when:

* ACPI declares I/O regions just because they exist, but doesn't make
use of them. This will prevent native drivers from binding to the
device, leaving the user without the functionality.

* ACPI accesses I/O regions without declaring them. Native drivers
will access them too, causing havoc.

While the two issues above could be worked around by analyzing the
AML code as you suggested, a better fix is to get BIOSes out there
right. I know it's an on-going, never-ending battle, but it is still
preferable to fix the bug where it really is, isn't it?

Then there are two corner cases:

* The ACPI implementation does offer the possibility to write an ACPI
driver for hardware monitoring on a given machine, but we do not yet
have written such a driver. This has been the case for Asus' ATK0110
driver for quite some time. This is a minor issue IMHO, not different
from lack of support of other, hardware devices.

* ACPI does access the hardware monitoring chip (or SMBus controller)
and does something useful with it, but this is only a small subset of
what a native driver would do. A typical example of this is
mainboards implementing ACPI thermal zones out of a full-featured
hardware monitoring chip. I have such a board here (Jetway K8M8MS).
The ACPI "thermal" driver displays one temperature. The native driver
(f71805f) displays 9 voltages, 3 fan speeds and 3 temperature values,
all settable min and max limits, and their associated alarm flags.
The ACPI "thermal" driver will load by default, while the native
driver will be blocked, leaving the user with very reduced
functionality. Me, I have configured by system to blacklist the
thermal driver and load the f71805f driver instead, I _think_ I am
safe doing this, but I'm not even sure.

> > I have to think that if this were truly the case, the operating
> > system vendors and the ACPI contributors would have pushed a fix
> > for this problem into the ACPI specification by now (in the 13
> > years since the first release of ACPI).       

If they did care, yes they would have. But see below.

> > The SMBus driver issue is very interesting. I have not heard of
> > any conflicts between AML code and SMBus drivers on other
> > operating systems, nor have we received any other complaints
> > about removing the acpi_os_validate_address interface. I wonder
> > if the Linux SMBus driver is doing something that it should not
> > be doing.     

I don't think it is. The Linux SMBus drivers (note the plural, we
have one driver for each separate PC SMBus implementation, that's
about a dozen of them) do exactly what they have to, that is: provide
the rest of the kernel with a standard API to access devices
connected to the SMBus.

Of course you can claim that, on ACPI systems, the OS should never
attempt to access thermal-management-related devices. This solves
the resource conflict problem at once, at the price of a loss of
functionality for most users out there (because on most desktop and
server systems, ACPI doesn't care about thermal management at all.)

> > I'd like to see a concrete example of the problem. If someone can
> > send me a DSDT that contains such an example, I would appreciate
> > it. I want to see the exact ASL code that is causing the problem
> > along with the driver code that conflicts with it. This
> > information will help me understand the problem in relation to
> > the ACPICA code, and I can also present this to other ACPI and
> > BIOS experts for their opinions.      

Here you go, attached. This is the only machine I have at home with
an ACPI vs. native driver resource conflict. This is the mainboard I
mentioned above already: Jetway K8M8MS. Conflict is for I/O region
0x295-0x296.

> > As far as an immediate fix for the problem, I suggest that you
> > think about using acpi_walk_namespace to retrieve all objects of
> > type Region (or field). It seems a waste to build an entire list
> > of objects that duplicates information that already exists in the
> > namespace.    
> 
> My guess is that exactly the same thing will happen if you install 
> add-on hardware monitoring software on Windows that loads drivers that 
> access SMBus hardware directly on such systems where ACPI accesses the 
> same devices. The ACPI code and the native driver will likely stomp on 
> each other's hardware accesses. Likely on Linux it's more frequent that 
> this kind of native hardware monitoring driver gets used (in some setups 
> it seems they will auto-load by default..)

This is a very good point, although not totally exact. The key reason
why the problem wasn't solved yet is because Microsoft doesn't give a
damn. Last time I checked (several years ago admittedly) Windows did
not offer support for hardware monitoring at all. Basically the user
must choose between a vendor-provided monitoring applications (most
PC mainboard vendors have one by now) or 3rd party applications such
as MBM (discontinued), SpeedFan or hmonitor.

With vendor applications, the problem doesn't exist, because the
vendor has additional knowledge. They only support their own
products, and they know what their ACPI implementation is doing or
not. So they can easily access custom ACPI code if needed, use
custom locking mechanism if needed, etc.

With 3rd party applications, the problem is exactly the same as with
Linux: this is best effort. The big advantage is a unified hardware
monitoring handling across all vendors and models, but at the price
of a lack of knowledge about system specifics, which means lot of
trouble on some systems.

As you are talking about improving the ACPI standard, here are ideas
of what could be done:

* ACPI could define a standard interface to access hardware
monitoring chips. Something similar in essence to the thermal zones,
but that would cover all the standard features of hardware monitoring
chips (voltages, temperatures, fans, including limits, and if
possible fan speed control.) Vendors would implement that interface
in their BIOSes, and OSes would simply need one driver to support all
systems out there.

* ACPI could define a flag the OS would check, basically declaring
whether thermal management / hardware monitoring should be handled
by ACPI or by the OS on every given system. Then all we have to do
is educate all drivers to check this flag and decide whether to load
or not.

* ACPI could offer a way to share I/O regions between AML code and
OS code. Then both ACPI and native drivers could access the same
device. Of course, driver code would have to be very robust (i.e.
slow) because it couldn't assume _anything_ about register values
and device state between accesses. But that would still be better
than the current situation.

Lastly, one more thought about SMBus-based hardware monitoring chips:
blocking I/O access to them basically means blocking I/O access to
_all_ devices connected to the SMBus. In many cases this means SPD
EEPROMs, which are fun to access at run-time but we can definitely
live without them. But there can be other devices on the SMBus,
for example some FSC systems have a custom chip on the SMBus to
handle custom function keys. If you want to let the OS drive the
chip, then the best (only?) approach is to let ACPI handle the
SMBus for both itself and the OS. That is, have an ACPI driver for
the SMBus implementation, which must support ACPI accesses and OS
accesses alike, gracefully. I'm pretty sure that ACPI _does_ define
a standard SMBus API which would make writing such a driver
possible... I even seem to remember that said driver did exist at
some point in the past. I'm curious why it disappeared?

-- 
Jean Delvare
Suse L3

[-- Attachment #2: DSDT --]
[-- Type: application/octet-stream, Size: 19667 bytes --]