ACPICA: Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"
Subsystems (2)
PATCH 1/1 ACPICA: Revert "ACPICA: Remove obsolete acpi_os_validate_address interface"
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
--- 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(-)
drivers/acpi/acpica/acobject.h +1 -0
@@ -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 */ /****************************************************************************** *
drivers/acpi/acpica/dsopcode.c +24 -0
@@ -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); }
drivers/acpi/acpica/exfldio.c +6 -0
@@ -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
include/acpi/acpiosxf.h +4 -0
@@ -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);
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.
Quoted 10 lines · On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
Revert "ACPICA: Remove obsolete
acpi_os_validate_addressinterface"This reverts commit
f9ca058430333c9a24c5ca926aa445125f88df18.The quick fix of bug 13620 would be to revert the change.
http://bugzilla.kernel.org/show_bug.cgi?id=13620Also, see the commit
df92e695998e1bc6e426a840eb86d6d1ee87e2a5"ACPI: track opregion names to avoid driver resource conflicts."
Quoted 6 lines
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.
Quoted 9 lines
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
Quoted 30 lines · 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.Quoted 10 lines · On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
Revert "ACPICA: Remove obsolete
acpi_os_validate_addressinterface"This reverts commit
f9ca058430333c9a24c5ca926aa445125f88df18.The quick fix of bug 13620 would be to revert the change.
http://bugzilla.kernel.org/show_bug.cgi?id=13620Also, see the commit
df92e695998e1bc6e426a840eb86d6d1ee87e2a5"ACPI: track opregion names to avoid driver resource conflicts."Quoted 6 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
Quoted 4 lines
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 timeRegion 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.
Quoted 2 lines
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
Quoted 4 lines
Thanks,
Thomas
Quoted 16 lines · On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
Quoted 15 lines · 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.Quoted 7 lines · On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
Revert "ACPICA: Remove obsolete
acpi_os_validate_addressinterface"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
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
definitions.
Good idea, this could avoid some false positive detected region conflicts.
Quoted 2 lines
2. For dynamic region, we need an interface to call when an
operation
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.Quoted 1 line
How can this happen, can you show a little ASL snippet or explain this a
bit
Quoted 10 lines
Quoted 1 line
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) ..... }
Quoted 5 lines
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
Quoted 74 lines
-----Original Message-----
From: Thomas Renninger [mailto:[email protected]]
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 obsoleteacpi_os_validate_addressinterface"Quoted 10 lines · On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
Quoted 9 lines · 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.Quoted 1 line · On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
Revert "ACPICA: Remove obsolete
acpi_os_validate_addressinterface"
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
This reverts commit f9ca058430333c9a24c5ca926aa445125f88df18. The quick fix of bug 13620 would be to revert the change. http://bugzilla.kernel.org/show_bug.cgi?id=13620Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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 OperationRegion
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
definitions.
Good idea, this could avoid some false positive detected region
conflicts.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
2. For dynamic region, we need an interface to call when an
operation
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
over again,
this is have the potential to be a memory leak by growing thelist
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.Quoted 1 line
How can this happen, can you show a little ASL snippet or explain this
a
bitQuoted 13 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 2 lines
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
Quoted 22 lines · Robert
Quoted 22 lines
-----Original Message-----
From: Thomas Renninger [mailto:[email protected]]
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 obsoleteacpi_os_validate_addressinterface"Quoted 10 lines · On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
Quoted 9 lines · 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.Quoted 1 line · On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
Revert "ACPICA: Remove obsolete
acpi_os_validate_addressinterface"
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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...
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
For example, the dynamic region which defined in a method,
Method(m000)
{ OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF) ..... }
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
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.
Quoted 25 lines
Quoted 4 lines
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_namespacefor 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.
Quoted 5 lines
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
could still detect most i2c/smbus/hwmon devices conflicts.
What exactly are these conflicts?
Quoted 128 lines
-----Original Message-----
From: Thomas Renninger [mailto:[email protected]]
Sent: Wednesday, July 01, 2009 2:20 PM
To: Moore, Robert
Cc: Lin, Ming M; Len Brown; Zhao, Yakui; linux-acpi; [email protected]
Subject: Re: [PATCH] ACPICA: Revert "ACPICA: Remove obsoleteacpi_os_validate_addressinterface"Quoted 22 lines · Robert
Quoted 22 lines
-----Original Message-----
From: Thomas Renninger [mailto:[email protected]]
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 obsoleteacpi_os_validate_addressinterface"Quoted 10 lines · On Wednesday 01 July 2009 11:23:38 Lin Ming wrote:
Quoted 9 lines · 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.Quoted 1 line · On Wednesday 01 July 2009 04:29:51 Lin Ming wrote:
Revert "ACPICA: Remove obsolete
acpi_os_validate_addressinterface"
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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 resourceconflicts."
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
< 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...Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
For example, the dynamic region which defined in a method,
Method(m000)
{ OperationRegion (xxxx, SystemMemory, 0xFED11000, 0xFF) ..... }Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
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 whereacpi_os_validate_addressis 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.Quoted 9 lines
Quoted 4 lines
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)Quoted 5 lines
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
Quoted 1 line
simply query the existing ACPI namespace when you need to? A
walk_namespaceQuoted 1 line
for field objects would give you the same information as the resource
list,
Quoted 6 lines
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
Quoted 4 lines
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
Quoted 2 lines
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
Quoted 1 line
these regions. The ACPI Global Lock is used when exclusive access cannot
be
Quoted 1 line
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/iomemand/proc/ioports, but I finally didn't see a way to do that.Thomas
Hi Robert,
Le jeudi 02 juillet 2009, Moore, Robert a écrit :
Quoted 3 lines
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.
Signature · 6 lines
Jean Delvare Suse L3 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Le mercredi 01 juillet 2009, Thomas Renninger a écrit :
Quoted 6 lines · Robert
Quoted 4 lines
Quoted 4 lines
-----Original Message-----
From: Thomas Renninger [mailto:[email protected]]
Ouch, I thought OperationRegions must be declared in global namespace. I agree, we have a memleak and this looks rather sever.
Quoted 7 lines
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 whereacpi_os_validate_addressis 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?
Quoted 18 lines
Quoted 18 lines
Quoted 4 lines
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_namespacefor field objects would give you the same information as the resource list, and it is automatically guaranteed to be current.
Quoted 6 lines
Quoted 6 lines
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.
Quoted 5 lines
Quoted 5 lines
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?
Signature · 6 lines
Jean Delvare Suse L3 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Len Brown
reviewerI 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
Quoted 5 lines · 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.
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.
drivers/acpi/acpica/utdelete.c +6 -0
@@ -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) {/*
drivers/acpi/osl.c +90 -3
122 lines not on this page · raw message
include/acpi/acpiosxf.h +4 -0
@@ -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);
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.
Quoted 33 lines
-----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 obsoleteacpi_os_validate_addressinterface"Quoted 5 lines · 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.
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.
Quoted 2 lines · Robert
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
Quoted 16 lines · On Thursday 02 July 2009 08:27:25 Lin Ming wrote:
Quoted 5 lines · 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.
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.
Quoted 5 lines
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
drivers/acpi/acpica/utdelete.c static void acpi_ut_delete_internal_obj(union
Quoted 10 lines
2) add a new function
acpi_os_invalidate_address, which is called when region is deleted.drivers/acpi/acpica/utdelete.c +0
-0@@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union
acpi_operand_object *object)
Quoted 101 lines
drivers/acpi/acpica/utdelete.c +6
-0ACPI_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) {/* drivers/acpi/osl.c +74
-0@@ -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 :
drivers/acpi/osl.c acpi_os_validate_interface (char *interface) · line 1419
Quoted 10 lines
drivers/acpi/osl.c +9
-0+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 ; +} +
/******************************************************************************
Quoted 49 lines
drivers/acpi/osl.c +7
-3* * 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 :include/acpi/acpiosxf.h +4
-0@@ -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);
Quoted 17 lines · Thomas Renninger
Quoted 16 lines · On Thursday 02 July 2009 08:27:25 Lin Ming wrote:
Quoted 5 lines · 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.
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.
Quoted 8 lines
Quoted 5 lines
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 andacpi_os_invalidate_addresswill 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
Quoted 178 lines
I like it. Thanks for looking into that!
Thomas
drivers/acpi/acpica/utdelete.c static void acpi_ut_delete_internal_obj(union
Quoted 10 lines
2) add a new function
acpi_os_invalidate_address, which is called when region is deleted.drivers/acpi/acpica/utdelete.c +0
-0@@ -215,6 +215,12 @@ static void acpi_ut_delete_internal_obj(union
acpi_operand_object*object)Quoted 101 lines
drivers/acpi/acpica/utdelete.c +6
-0ACPI_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) {/* drivers/acpi/osl.c +74
-0@@ -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 :drivers/acpi/osl.c acpi_os_validate_interface (char *interface) · line 1419
Quoted 10 lines
drivers/acpi/osl.c +9
-0+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 ; +} +/******************************************************************************
Quoted 49 lines
drivers/acpi/osl.c +7
-3* * 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 :include/acpi/acpiosxf.h +4
-0@@ -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);
Hi Lin and all others...
this is about:
http://bugzilla.kernel.org/show_bug.cgi?id=13620
Quoted 9 lines · On Thursday 02 July 2009 08:27:25 Lin Ming wrote:
Quoted 5 lines · 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.
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
--- 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(-)
drivers/acpi/acpica/aclocal.h +2 -0
@@ -44,6 +44,8 @@#ifndef __ACLOCAL_H__#define __ACLOCAL_H__ +#include "acconfig.h" +/* acpisrc:struct_defs -- for acpisrc conversion */ #define ACPI_SERIALIZED 0xFF
drivers/acpi/acpica/acnamesp.h +2 -0
@@ -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)
drivers/acpi/acpica/acobject.h +2 -0
@@ -45,6 +45,8 @@#ifndef _ACOBJECT_H#define _ACOBJECT_H +#include "aclocal.h" +/* acpisrc:struct_defs -- for acpisrc conversion */ /*
drivers/acpi/osl.c +78 -92
201 lines not on this page · raw message
Quoted 98 lines · Thomas Renninger
Hi Lin and all others...
this is about:
http://bugzilla.kernel.org/show_bug.cgi?id=13620Quoted 9 lines · On Thursday 02 July 2009 08:27:25 Lin Ming wrote:
Quoted 5 lines · 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.
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 -> structacpi_object_regionmapping
at the beginning of thewalk_namespacecallback 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.clist 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 opregionsSigned-off-by Thomas Renninger <[email protected]>--- 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
===================================================================drivers/acpi/acpica/aclocal.h +2
-0@@ -44,6 +44,8 @@#ifndef __ACLOCAL_H__#define __ACLOCAL_H__ +#include "acconfig.h" +/* acpisrc:struct_defs -- for acpisrc conversion */ #define ACPI_SERIALIZED 0xFFIndex:
linux-2.6.29/drivers/acpi/acpica/acnamesp.h
===================================================================drivers/acpi/acpica/acnamesp.h +2
-0@@ -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
===================================================================drivers/acpi/acpica/acobject.h +2
-0@@ -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
===================================================================
drivers/acpi/osl.c line 56
Quoted 6 lines
drivers/acpi/osl.c +3
-0@@ -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"
Quoted 33 lines
drivers/acpi/osl.c +4
-15+#define _COMPONENT ACPI_OS_SERVICESACPI_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)
drivers/acpi/osl.c static int __init acpi_enforce_resources · line 1096
Quoted 10 lines
drivers/acpi/osl.c +1
-7{ -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.
Quoted 8 lines
drivers/acpi/osl.c +4
-2- 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);
drivers/acpi/osl.c static int __init acpi_enforce_resources · line 1104
Quoted 10 lines
drivers/acpi/osl.c +2
-8-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.
drivers/acpi/osl.c static int __init acpi_enforce_resources · line 1110
Quoted 13 lines
drivers/acpi/osl.c +5
-7+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
Quoted 125 lines
drivers/acpi/osl.c +59
-53+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; }
Quoted 185 lines · Lin Ming
Quoted 101 lines · Thomas Renninger
Hi Lin and all others...
this is about:
http://bugzilla.kernel.org/show_bug.cgi?id=13620Quoted 9 lines · On Thursday 02 July 2009 08:27:25 Lin Ming wrote:
Quoted 5 lines · 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.
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 -> structacpi_object_regionmapping
at the beginning of thewalk_namespacecallback 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.clist 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 opregionsSigned-off-by Thomas Renninger <[email protected]>--- 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
===================================================================drivers/acpi/acpica/aclocal.h +2
-0@@ -44,6 +44,8 @@#ifndef __ACLOCAL_H__#define __ACLOCAL_H__ +#include "acconfig.h" +/* acpisrc:struct_defs -- for acpisrc conversion */ #define ACPI_SERIALIZED 0xFFIndex:
linux-2.6.29/drivers/acpi/acpica/acnamesp.h
===================================================================drivers/acpi/acpica/acnamesp.h +2
-0@@ -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
===================================================================drivers/acpi/acpica/acobject.h +2
-0@@ -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
===================================================================drivers/acpi/osl.c line 56
Quoted 6 lines
drivers/acpi/osl.c +3
-0@@ -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"Quoted 33 lines
drivers/acpi/osl.c +3
-15+#define _COMPONENT ACPI_OS_SERVICESACPI_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)drivers/acpi/osl.c static int __init acpi_enforce_resources · line 1096
Quoted 10 lines
drivers/acpi/osl.c +1
-7{ -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_nodeis an ACPICA internal used only structure.Quoted 7 lines
drivers/acpi/osl.c +4
-2- 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);drivers/acpi/osl.c static int __init acpi_enforce_resources · line 1104
Quoted 10 lines
drivers/acpi/osl.c +2
-8-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_nodeis ACPICA internal used only function.
Quoted 1 line
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
Quoted 143 lines
Quoted 4 lines
drivers/acpi/osl.c +2
-0+if (!node || node->type !=ACPI_TYPE_REGION ) +return AE_OK ;drivers/acpi/osl.c static int __init acpi_enforce_resources · line 1110
Quoted 10 lines
drivers/acpi/osl.c +3
-7-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_objectis ACPICA internal used only.Thanks,
Lin MingQuoted 123 lines
drivers/acpi/osl.c +59
-53+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; }
Quoted 2 lines · 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.
Quoted 2 lines
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
Quoted 2 lines
-----Original Message-----
From: Jean Delvare [mailto:[email protected]]
...
Quoted 5 lines
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/ioportsand/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
Quoted 23 lines
-----Original Message-----
From: Thomas Renninger [mailto:[email protected]]
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 obsoleteacpi_os_validate_addressinterface"Quoted 2 lines · 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.Quoted 2 lines
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
Quoted 10 lines · Robert
Quoted 2 lines
-----Original Message-----
From: Jean Delvare [mailto:[email protected]]...
Quoted 5 lines
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/ioportsand/proc/iomem.
Quoted 7 lines
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_addressinterface. 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_namespaceto 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..)
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 :
Quoted 11 lines · Robert
Quoted 10 lines
Quoted 2 lines
-----Original Message-----
From: Jean Delvare [mailto:[email protected]]...
Quoted 5 lines
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/ioportsand/proc/iomem.
Quoted 4 lines
Quoted 4 lines
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.
Quoted 4 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
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_addressinterface. 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.)
Quoted 7 lines
Quoted 7 lines
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.
Quoted 6 lines
Quoted 5 lines
As far as an immediate fix for the problem, I suggest that you think about using
acpi_walk_namespaceto 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.
Quoted 7 lines
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?
Signature · 2 lines
Jean Delvare Suse L3