[PATCH] ACPI: add "auto" to acpi_enforce_resources

[PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

(This is an improved version of my earlier patch, I've reworked deviced
detection to simply check for the desired HID)

The following patch adds "auto" option to "acpi_enforce_resource"; this value
is also the new default.
"auto" enforces strict resource checking - disallowing access by native drivers
to IO ports and memory regions claimed by ACPI firmware - when a device with a
known ACPI driver is found (currently only ASUS ATK0110 is checked), and
reverts to lax checking otherwise.

The patch is mainly aimed to block native hwmon drivers from touching
monitoring chips that ACPI thinks it own.

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
---
New revision, now it simply checks the HID.

 Documentation/kernel-parameters.txt |   19 ++++++++++++++++
 drivers/acpi/osl.c                  |   41 +++++++++++++++++++++++++++++++++---
 2 files changed, 57 insertions(+), 3 deletions(-)

Index: b/Documentation/kernel-parameters.txt
===================================================================
--- a/Documentation/kernel-parameters.txt	2009-01-17 21:22:49.218882286 +0100
+++ b/Documentation/kernel-parameters.txt	2009-01-21 23:25:41.262478018 +0100
@@ -258,6 +258,25 @@
 			to assume that this machine's pmtimer latches its value
 			and always returns good values.
 
+	acpi_enforce_resources=	[ACPI]
+			{ strict, auto, lax, no }
+			Check for resource conflicts between native drivers
+			and ACPI OperationRegions (SystemIO and System Memory
+			only). IO ports and memory declared in ACPI might be
+			used by the ACPI subsystem in arbitrary AML code and
+			can interfere with legacy drivers.
+			strict: access to resources claimed by ACPI is denied;
+			legacy drivers trying to access reserved resources
+			will fail to load.
+			auto (default): try and detect ACPI devices with known
+			ACPI drivers; escalates the setting to "strict" if such
+			a device is found, otherwise behaves like "lax".
+			lax: access to resources claimed by ACPI is allowed;
+			legacy drivers trying to access reserved resources
+			will load and a warning message is logged.
+			no: ACPI OperationRegions are not marked as reserved,
+			no further checks are performed.
+
 	agp=		[AGP]
 			{ off | try_unsupported }
 			off: disable AGP support
Index: b/drivers/acpi/osl.c
===================================================================
--- a/drivers/acpi/osl.c	2009-01-17 21:22:49.190882303 +0100
+++ b/drivers/acpi/osl.c	2009-01-25 22:04:30.759209000 +0100
@@ -1063,7 +1063,10 @@
  * in arbitrary AML code and can interfere with legacy drivers.
  * acpi_enforce_resources= can be set to:
  *
- *   - strict           (2)
+ *   - auto             (2)
+ *     -> detect possible conflicts with ACPI drivers and switch to
+ *     strict if needed, otherwise act like lax
+ *   - strict           (3)
  *     -> further driver trying to access the resources will not load
  *   - lax (default)    (1)
  *     -> further driver trying to access the resources will load, but you
@@ -1073,11 +1076,12 @@
  *     -> ACPI Operation Region resources will not be registered
  *
  */
-#define ENFORCE_RESOURCES_STRICT 2
+#define ENFORCE_RESOURCES_STRICT 3
+#define ENFORCE_RESOURCES_AUTO   2
 #define ENFORCE_RESOURCES_LAX    1
 #define ENFORCE_RESOURCES_NO     0
 
-static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
+static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
 
 static int __init acpi_enforce_resources_setup(char *str)
 {
@@ -1086,6 +1090,8 @@
 
 	if (!strcmp("strict", str))
 		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
+	else if (!strcmp("auto", str))
+		acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
 	else if (!strcmp("lax", str))
 		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
 	else if (!strcmp("no", str))
@@ -1096,6 +1102,35 @@
 
 __setup("acpi_enforce_resources=", acpi_enforce_resources_setup);
 
+static acpi_status acpi_detect_asus_atk_cb(acpi_handle obj_handle,
+		u32 nesting_level, void *context, void **return_value)
+{
+	*((bool *)return_value) = true;
+	return AE_CTRL_TERMINATE;
+}
+
+static int __init acpi_detect_asus_atk(void)
+{
+	acpi_status ret;
+	bool found = false;
+
+	if (acpi_enforce_resources != ENFORCE_RESOURCES_AUTO)
+		return 0;
+
+	ret = acpi_get_devices("ATK0110", acpi_detect_asus_atk_cb,
+			NULL, (void **)&found);
+
+	if (ret == AE_OK && found) {
+		printk(KERN_DEBUG "Asus ATK0110 interface detected: "
+				"enforcing resource checking.\n");
+		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
+	} else
+		acpi_enforce_resources =  ENFORCE_RESOURCES_LAX;
+
+	return 0;
+}
+fs_initcall(acpi_detect_asus_atk);
+
 /* Check for resource conflicts between ACPI OperationRegions and native
  * drivers */
 int acpi_check_resource_conflict(struct resource *res)


Luca
-- 
La vasca da bagno fu inventata nel 1850, il telefono nel 1875.
Se fossi vissuto nel 1850, avrei potuto restare in vasca per 25 anni
senza sentir squillare il telefono

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Hans de Goede]

Luca Tettamanti wrote:
> (This is an improved version of my earlier patch, I've reworked deviced
> detection to simply check for the desired HID)
> 
> The following patch adds "auto" option to "acpi_enforce_resource"; this value
> is also the new default.
> "auto" enforces strict resource checking - disallowing access by native drivers
> to IO ports and memory regions claimed by ACPI firmware - when a device with a
> known ACPI driver is found (currently only ASUS ATK0110 is checked), and
> reverts to lax checking otherwise.
> 
> The patch is mainly aimed to block native hwmon drivers from touching
> monitoring chips that ACPI thinks it own.
> 
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>

This looks very good!

ACPI team, any chance we can get this in the mainline?

We want to add a driver (for the asus atk0110 ACPI interface) to the hwmon 
subsys which talks to hwmon IC's through ACPI, but before we can do that we 
must make sure that potentially conflicting native IC drivers do not load.

Thanks & Regards,

Hans

> ---
> New revision, now it simply checks the HID.
> 
>  Documentation/kernel-parameters.txt |   19 ++++++++++++++++
>  drivers/acpi/osl.c                  |   41 +++++++++++++++++++++++++++++++++---
>  2 files changed, 57 insertions(+), 3 deletions(-)
> 
> Index: b/Documentation/kernel-parameters.txt
> ===================================================================
> --- a/Documentation/kernel-parameters.txt	2009-01-17 21:22:49.218882286 +0100
> +++ b/Documentation/kernel-parameters.txt	2009-01-21 23:25:41.262478018 +0100
> @@ -258,6 +258,25 @@
>  			to assume that this machine's pmtimer latches its value
>  			and always returns good values.
>  
> +	acpi_enforce_resources=	[ACPI]
> +			{ strict, auto, lax, no }
> +			Check for resource conflicts between native drivers
> +			and ACPI OperationRegions (SystemIO and System Memory
> +			only). IO ports and memory declared in ACPI might be
> +			used by the ACPI subsystem in arbitrary AML code and
> +			can interfere with legacy drivers.
> +			strict: access to resources claimed by ACPI is denied;
> +			legacy drivers trying to access reserved resources
> +			will fail to load.
> +			auto (default): try and detect ACPI devices with known
> +			ACPI drivers; escalates the setting to "strict" if such
> +			a device is found, otherwise behaves like "lax".
> +			lax: access to resources claimed by ACPI is allowed;
> +			legacy drivers trying to access reserved resources
> +			will load and a warning message is logged.
> +			no: ACPI OperationRegions are not marked as reserved,
> +			no further checks are performed.
> +
>  	agp=		[AGP]
>  			{ off | try_unsupported }
>  			off: disable AGP support
> Index: b/drivers/acpi/osl.c
> ===================================================================
> --- a/drivers/acpi/osl.c	2009-01-17 21:22:49.190882303 +0100
> +++ b/drivers/acpi/osl.c	2009-01-25 22:04:30.759209000 +0100
> @@ -1063,7 +1063,10 @@
>   * in arbitrary AML code and can interfere with legacy drivers.
>   * acpi_enforce_resources= can be set to:
>   *
> - *   - strict           (2)
> + *   - auto             (2)
> + *     -> detect possible conflicts with ACPI drivers and switch to
> + *     strict if needed, otherwise act like lax
> + *   - strict           (3)
>   *     -> further driver trying to access the resources will not load
>   *   - lax (default)    (1)
>   *     -> further driver trying to access the resources will load, but you
> @@ -1073,11 +1076,12 @@
>   *     -> ACPI Operation Region resources will not be registered
>   *
>   */
> -#define ENFORCE_RESOURCES_STRICT 2
> +#define ENFORCE_RESOURCES_STRICT 3
> +#define ENFORCE_RESOURCES_AUTO   2
>  #define ENFORCE_RESOURCES_LAX    1
>  #define ENFORCE_RESOURCES_NO     0
>  
> -static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>  
>  static int __init acpi_enforce_resources_setup(char *str)
>  {
> @@ -1086,6 +1090,8 @@
>  
>  	if (!strcmp("strict", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
> +	else if (!strcmp("auto", str))
> +		acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>  	else if (!strcmp("lax", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
>  	else if (!strcmp("no", str))
> @@ -1096,6 +1102,35 @@
>  
>  __setup("acpi_enforce_resources=", acpi_enforce_resources_setup);
>  
> +static acpi_status acpi_detect_asus_atk_cb(acpi_handle obj_handle,
> +		u32 nesting_level, void *context, void **return_value)
> +{
> +	*((bool *)return_value) = true;
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static int __init acpi_detect_asus_atk(void)
> +{
> +	acpi_status ret;
> +	bool found = false;
> +
> +	if (acpi_enforce_resources != ENFORCE_RESOURCES_AUTO)
> +		return 0;
> +
> +	ret = acpi_get_devices("ATK0110", acpi_detect_asus_atk_cb,
> +			NULL, (void **)&found);
> +
> +	if (ret == AE_OK && found) {
> +		printk(KERN_DEBUG "Asus ATK0110 interface detected: "
> +				"enforcing resource checking.\n");
> +		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
> +	} else
> +		acpi_enforce_resources =  ENFORCE_RESOURCES_LAX;
> +
> +	return 0;
> +}
> +fs_initcall(acpi_detect_asus_atk);
> +
>  /* Check for resource conflicts between ACPI OperationRegions and native
>   * drivers */
>  int acpi_check_resource_conflict(struct resource *res)
> 
> 
> Luca

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Thomas Renninger]

On Sunday 25 January 2009 22:05:20 Luca Tettamanti wrote:
> (This is an improved version of my earlier patch, I've reworked deviced
> detection to simply check for the desired HID)
>
> The following patch adds "auto" option to "acpi_enforce_resource"; this
> value is also the new default.
> "auto" enforces strict resource checking - disallowing access by native
> drivers to IO ports and memory regions claimed by ACPI firmware - when a
> device with a known ACPI driver is found (currently only ASUS ATK0110 is
> checked), and reverts to lax checking otherwise.
>
> The patch is mainly aimed to block native hwmon drivers from touching
> monitoring chips that ACPI thinks it own.
Having this in the core ACPI code is ugly.
Either this should be more generic (instead of hardcoded "ATK0110" device,
use a list to add further specific thermal ACPI drivers). Hmm, maybe it's
only ASUS violating the spec? Thinkpads seem to read out extra thermal
data from EC and shouldn't interfere with hwmon drivers. hp-wmi seem to
read hdd temp through wmi, don't know whether this could interfere with
hwmon, I expect not?  Are there other known, model specific ACPI devices, 
accessing thermal IOs directly which could interfere with hwmon, then it
might be worth it.

If not, on these ASUS there should be a specific hwmon driver interfering
with the ATK0110 device?
Can't you just add:

interfering_hwmon_driver.c:

#ifdef ACPI
acpi_search_for_ATK0110(){
    /* put code from below in here */
}
#endif

interfering_hwmon_driver_init/add(){
...
#ifdef ACPI
    if (!acpi_disabled)
      if (acpi_search_for_ATK0110()) {
        printk ("Do not use this hwmon driver, use asus_acpi to read the "
                "extra sensor.\n");
        return -1;
      else
        err = acpi_check_resource_conflict(&res);
    }
#endif

    Thomas

> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> ---
> New revision, now it simply checks the HID.
>
>  Documentation/kernel-parameters.txt |   19 ++++++++++++++++
>  drivers/acpi/osl.c                  |   41
> +++++++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 3
> deletions(-)
>
> Index: b/Documentation/kernel-parameters.txt
> ===================================================================
> --- a/Documentation/kernel-parameters.txt	2009-01-17 21:22:49.218882286
> +0100 +++ b/Documentation/kernel-parameters.txt	2009-01-21
> 23:25:41.262478018 +0100 @@ -258,6 +258,25 @@
>  			to assume that this machine's pmtimer latches its value
>  			and always returns good values.
>
> +	acpi_enforce_resources=	[ACPI]
> +			{ strict, auto, lax, no }
> +			Check for resource conflicts between native drivers
> +			and ACPI OperationRegions (SystemIO and System Memory
> +			only). IO ports and memory declared in ACPI might be
> +			used by the ACPI subsystem in arbitrary AML code and
> +			can interfere with legacy drivers.
> +			strict: access to resources claimed by ACPI is denied;
> +			legacy drivers trying to access reserved resources
> +			will fail to load.
> +			auto (default): try and detect ACPI devices with known
> +			ACPI drivers; escalates the setting to "strict" if such
> +			a device is found, otherwise behaves like "lax".
> +			lax: access to resources claimed by ACPI is allowed;
> +			legacy drivers trying to access reserved resources
> +			will load and a warning message is logged.
> +			no: ACPI OperationRegions are not marked as reserved,
> +			no further checks are performed.
> +
>  	agp=		[AGP]
>  			{ off | try_unsupported }
>  			off: disable AGP support
> Index: b/drivers/acpi/osl.c
> ===================================================================
> --- a/drivers/acpi/osl.c	2009-01-17 21:22:49.190882303 +0100
> +++ b/drivers/acpi/osl.c	2009-01-25 22:04:30.759209000 +0100
> @@ -1063,7 +1063,10 @@
>   * in arbitrary AML code and can interfere with legacy drivers.
>   * acpi_enforce_resources= can be set to:
>   *
> - *   - strict           (2)
> + *   - auto             (2)
> + *     -> detect possible conflicts with ACPI drivers and switch to
> + *     strict if needed, otherwise act like lax
> + *   - strict           (3)
>   *     -> further driver trying to access the resources will not load
>   *   - lax (default)    (1)
>   *     -> further driver trying to access the resources will load, but you
> @@ -1073,11 +1076,12 @@
>   *     -> ACPI Operation Region resources will not be registered
>   *
>   */
> -#define ENFORCE_RESOURCES_STRICT 2
> +#define ENFORCE_RESOURCES_STRICT 3
> +#define ENFORCE_RESOURCES_AUTO   2
>  #define ENFORCE_RESOURCES_LAX    1
>  #define ENFORCE_RESOURCES_NO     0
>
> -static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>
>  static int __init acpi_enforce_resources_setup(char *str)
>  {
> @@ -1086,6 +1090,8 @@
>
>  	if (!strcmp("strict", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
> +	else if (!strcmp("auto", str))
> +		acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>  	else if (!strcmp("lax", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
>  	else if (!strcmp("no", str))
> @@ -1096,6 +1102,35 @@
>
>  __setup("acpi_enforce_resources=", acpi_enforce_resources_setup);
>
> +static acpi_status acpi_detect_asus_atk_cb(acpi_handle obj_handle,
> +		u32 nesting_level, void *context, void **return_value)
> +{
> +	*((bool *)return_value) = true;
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static int __init acpi_detect_asus_atk(void)
> +{
> +	acpi_status ret;
> +	bool found = false;
> +
> +	if (acpi_enforce_resources != ENFORCE_RESOURCES_AUTO)
> +		return 0;
> +
> +	ret = acpi_get_devices("ATK0110", acpi_detect_asus_atk_cb,
> +			NULL, (void **)&found);
> +
> +	if (ret == AE_OK && found) {
> +		printk(KERN_DEBUG "Asus ATK0110 interface detected: "
> +				"enforcing resource checking.\n");
> +		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
> +	} else
> +		acpi_enforce_resources =  ENFORCE_RESOURCES_LAX;
> +
> +	return 0;
> +}
> +fs_initcall(acpi_detect_asus_atk);
> +
>  /* Check for resource conflicts between ACPI OperationRegions and native
>   * drivers */
>  int acpi_check_resource_conflict(struct resource *res)
>
>
> Luca

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

On Thu, Jan 29, 2009 at 11:30 AM, Thomas Renninger <trenn@suse.de> wrote:
> On Sunday 25 January 2009 22:05:20 Luca Tettamanti wrote:
>> (This is an improved version of my earlier patch, I've reworked deviced
>> detection to simply check for the desired HID)
>>
>> The following patch adds "auto" option to "acpi_enforce_resource"; this
>> value is also the new default.
>> "auto" enforces strict resource checking - disallowing access by native
>> drivers to IO ports and memory regions claimed by ACPI firmware - when a
>> device with a known ACPI driver is found (currently only ASUS ATK0110 is
>> checked), and reverts to lax checking otherwise.
>>
>> The patch is mainly aimed to block native hwmon drivers from touching
>> monitoring chips that ACPI thinks it own.
>
> Having this in the core ACPI code is ugly.

I think we all agree :-)

> Either this should be more generic (instead of hardcoded "ATK0110" device,
> use a list to add further specific thermal ACPI drivers). Hmm, maybe it's
> only ASUS violating the spec?

ASUS it's not actually violating any spec...

> Thinkpads seem to read out extra thermal
> data from EC and shouldn't interfere with hwmon drivers.

The point is that there is only 1 physical sensor, which both ACPI and
a native driver want to drive; transaction on SMBus to read the sensor
are usually in the form "select bank" + "read" and the sequence is
*not* atomic. In ASUS case the IO ports are correctly reserved by the
firmware, but (historically) this wasn't taken into account.
Aside from ASUS the problem is generic since there are two drivers
poking at the same hardware; for example there are reports of native
drivers interfering with normal TZ polling (see [1]). The EC does not
make any difference, since a native driver would speak directly to the
monitoring chip, effectively by-passing the EC.
Now, in principle "strict" is the correct behaviour for the resource
checking code, but it would break many working setups leaving the
users without any kind of hw monitoring. The "auto" hack is meant to
force the users to migrate to the ACPI driver...

> Are there other known, model specific ACPI devices,
> accessing thermal IOs directly which could interfere with hwmon, then it
> might be worth it.

Right now "auto" is ASUS-specific because *I* know that there any many
different native drivers that works on various boards (and I wrote the
the ACPI driver...); At least eeepc and (some?) thinkpads have ACPI
hwmon capabilities but I don't know whether there are native drivers
available or not (but they could be "blacklisted"  preemptively like
ASUS ATK).


Luca
[1] http://www.lm-sensors.org/ticket/2072 and:
http://thread.gmane.org/gmane.linux.drivers.sensors/12359

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Thomas Renninger]

On Thursday 29 January 2009 16:16:38 Luca Tettamanti wrote:
> On Thu, Jan 29, 2009 at 11:30 AM, Thomas Renninger <trenn@suse.de> wrote:
> > On Sunday 25 January 2009 22:05:20 Luca Tettamanti wrote:
> >> (This is an improved version of my earlier patch, I've reworked deviced
> >> detection to simply check for the desired HID)
> >>
> >> The following patch adds "auto" option to "acpi_enforce_resource"; this
> >> value is also the new default.
> >> "auto" enforces strict resource checking - disallowing access by native
> >> drivers to IO ports and memory regions claimed by ACPI firmware - when a
> >> device with a known ACPI driver is found (currently only ASUS ATK0110 is
> >> checked), and reverts to lax checking otherwise.
> >>
> >> The patch is mainly aimed to block native hwmon drivers from touching
> >> monitoring chips that ACPI thinks it own.
> >
> > Having this in the core ACPI code is ugly.
>
> I think we all agree :-)
>
> > Either this should be more generic (instead of hardcoded "ATK0110"
> > device, use a list to add further specific thermal ACPI drivers). Hmm,
> > maybe it's only ASUS violating the spec?
>
> ASUS it's not actually violating any spec...
They have to export the temperature through a thermal ACPI
device, not through the ASUS specific ATK0110 device. This is
what I mean whether there might be other vendor specific ACPI
devices violating the spec (by providing temperature read
functionality which should be done through the generic thermal
ACPI device).

> > Thinkpads seem to read out extra thermal
> > data from EC and shouldn't interfere with hwmon drivers.
>
> The point is that there is only 1 physical sensor, which both ACPI and
> a native driver want to drive; transaction on SMBus to read the sensor
> are usually in the form "select bank" + "read" and the sequence is
> *not* atomic. In ASUS case the IO ports are correctly reserved by the
> firmware, but (historically) this wasn't taken into account.
I know about this problem.

> Aside from ASUS the problem is generic since there are two drivers
> poking at the same hardware; for example there are reports of native
> drivers interfering with normal TZ polling (see [1])
Yes, this is even worse as the check that you want to add will not
catch it. Looking a bit through ASUS DSDTs this seem to be common
on most of them. I put some example DSDT code of a recent M2A-VM-HDMI
workstation at the end.

> The EC does not
> make any difference, since a native driver would speak directly to the
> monitoring chip, effectively by-passing the EC.
> Now, in principle "strict" is the correct behaviour for the resource
> checking code, but it would break many working setups leaving the
> users without any kind of hw monitoring. The "auto" hack is meant to
> force the users to migrate to the ACPI driver...
>
> > Are there other known, model specific ACPI devices,
> > accessing thermal IOs directly which could interfere with hwmon, then it
> > might be worth it.
>
> Right now "auto" is ASUS-specific because *I* know that there any many
> different native drivers that works on various boards (and I wrote the
> the ACPI driver...);
Hmm, grep -r ATK0110 drivers/platform/x86 in latest ACPI test tree is
empty, can you give me a pointer to a recent version of your driver.

> At least eeepc and (some?) thinkpads have ACPI
> hwmon capabilities but I don't know whether there are native drivers
> available or not (but they could be "blacklisted"  preemptively like
> ASUS ATK).
I expect that ASUS only will interfere with specific hwmon driver(s)?
So why don't you move the check into the hwmon driver and make it not
load on these systems?
It's still ugly, but IMO better than to pollute the ACPI core.

Here some thermal DSDT parts of a recent ASUS machine:

The temperature exported through the generic thermal ACPI device:

    OperationRegion (IP, SystemIO, 0x022D, 0x02)
    Field (IP, ByteAcc, NoLock, Preserve)
    {
        INDX,   8,
        DAT0,   8
    }

    Method (SBYT, 2, NotSerialized)
    {
        Store (Arg0, INDX)
        Store (Arg1, DAT0)
    }

    Method (GBYT, 1, NotSerialized)
    {
        Store (Arg0, INDX)
        Store (DAT0, Local0)
        Return (Local0)
    }

Method (_TMP, 0, NotSerialized)
    {
    /* makes use of RTMP which uses GBYT and SBYT to actually read
       the temp
    */
    }

There is another RTMP function in ATK0110 (do you use that?)
The other RTMP in ATK0110 seem to use this IO area to read out
the temperatures (MBTE and TSR1, for mainboard and CPU temp?):

                OperationRegion (HWRE, SystemIO, 0x022D, 0x02)
                Field (HWRE, ByteAcc, NoLock, Preserve)
                {
                    HIDX,   8,
                    HDAT,   8
                }

                IndexField (HIDX, HDAT, ByteAcc, NoLock, Preserve)
                {
                            Offset (0x0B),
                    FD11,   3,
                    FD12,   3,
                    FD13,   1,
                            Offset (0x0C),
                            Offset (0x0D),
                    FAN1,   8,
                    FAN2,   8,
                            Offset (0x18),
                    FEN1,   8,
                    FEN2,   8,
                            Offset (0x20),
                    VCOR,   8,
                    V33V,   8,
                            Offset (0x23),
                    V50V,   8,
                    V12V,   8,
                            Offset (0x29),
                    TSR1,   8,
                    MBTE,   8,
                            Offset (0x80),
                    FAN3,   8,
                    FEN3,   8
                }

Can the hwmon people identify which hwmon driver is used
for above devices and the ATK0110 check be inserted there.

On the M2A-VM-HDMI machine where DSDT extracts from above are,
running libsensors I get:

Driver `it87' (should be inserted):
  Detects correctly:
  * ISA bus, address 0x228
    Chip `ITE IT8716F Super IO Sensors' (confidence: 9)

Driver `to-be-written' (should be inserted):
  Detects correctly:
  * Chip `AMD K10 thermal sensors' (confidence: 9)

If this is an ASUS only problem with very specific thermal
sensors, better add the quirk at the specific sensor driver.

If this is something general, then maybe you can convince Len
to add something to the ACPI core, but this should be more
generic then.

      Thomas

PS: I just realize that in ATK0110 the same thermal device
(IO port 0x22D) is used as in the generic ACPI device (_TMP).
So it looks like your asus thermal driver will not only interfere
with the hwmon driver, but with the ACPI thermal driver itself.

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Hans de Goede]

Thomas Renninger wrote:
>>> Either this should be more generic (instead of hardcoded "ATK0110"
>>> device, use a list to add further specific thermal ACPI drivers). Hmm,
>>> maybe it's only ASUS violating the spec?
>> ASUS it's not actually violating any spec...
> They have to export the temperature through a thermal ACPI
> device, not through the ASUS specific ATK0110 device. This is
> what I mean whether there might be other vendor specific ACPI
> devices violating the spec (by providing temperature read
> functionality which should be done through the generic thermal
> ACPI device).
> 

hwmon IC's monitor far more then just temperatures, ASUS is doing the right 
thing here by providing an ACPI interface to also read voltages and fan speeds, 
so that these can be read in a way that does not interfere with the ACPI code.

And although even their interface does not expose the full functionality of the 
hwmon IC's, it is much much better then what the thermal ACPI code gives us.

>>> Thinkpads seem to read out extra thermal
>>> data from EC and shouldn't interfere with hwmon drivers.
>> The point is that there is only 1 physical sensor, which both ACPI and
>> a native driver want to drive; transaction on SMBus to read the sensor
>> are usually in the form "select bank" + "read" and the sequence is
>> *not* atomic. In ASUS case the IO ports are correctly reserved by the
>> firmware, but (historically) this wasn't taken into account.
> I know about this problem.
> 
>> Aside from ASUS the problem is generic since there are two drivers
>> poking at the same hardware; for example there are reports of native
>> drivers interfering with normal TZ polling (see [1])
> Yes, this is even worse as the check that you want to add will not
> catch it.

Ack, so the proper solution would be to just make the acpi_enforce_resources 
strict by default, but this may cause lack of functionality for some user who 
are currently using native drivers for hwmon features. Which is why I (hwmon 
subsytem contributor and Jean Delvare (i2c and (ex)hwmon subsystem maintainer) 
proposed this auto setting as a compromise. I'm fine with changing the default 
to strict, AFAIK Jean prefers the auto setting.

<snip>

>> At least eeepc and (some?) thinkpads have ACPI
>> hwmon capabilities but I don't know whether there are native drivers
>> available or not (but they could be "blacklisted"  preemptively like
>> ASUS ATK).
> I expect that ASUS only will interfere with specific hwmon driver(s)?

Yes only those used on their motherboards, and given the wide range of 
motherboard ASUS produces and they offer the ATK0110 interface on almost all of 
them, it pretty much comes down to "all hwmon and smbus drivers", although I'm 
sure if we look we can find exceptions.

> So why don't you move the check into the hwmon driver and make it not
> load on these systems?
> It's still ugly, but IMO better than to pollute the ACPI core.
> 

Because we do not want to do this in a zillion places when it should be done in 
only one. The right solution is to make acpi_enforce_resources strict the 
default, the auto setting is meant as a slow migration path to that right 
solution, as doing that right now probably will cause lots of complaints.

Actually, the really right solution would be for the ACPI subsystem to actually 
claim all the resources using the regular resource tracking so that drivers who 
want to check for resource conflicts with ACPI do not have to explicitly call
acpi_check_resource_conflict() (and friends), but instead will get an error 
when trying to claim the resources from the regular resource system. However 
chances are this will break things on quite a lot of systems, still it would be 
the right thing to do in the end IMHO.

<snip>

> If this is an ASUS only problem with very specific thermal
> sensors, better add the quirk at the specific sensor driver.
> 

It is neither ASUS specific, not is it limited to specific hwmon IC's, these 
are not thermal sensors, they are capable of monitoring far more then just 
temperature, which is why the ACPI thermal device interface is insufficient.

> PS: I just realize that in ATK0110 the same thermal device
> (IO port 0x22D) is used as in the generic ACPI device (_TMP).
> So it looks like your asus thermal driver will not only interfere
> with the hwmon driver, but with the ACPI thermal driver itself.

I would expect the ACPI code to take care of any conflicts between the various 
interfaces it offers itself. And if it is unsafe to call multiple ACPI methods 
at the same time, I would expect the ACPI core to fix this.

Also note that the above paragraph actually advocates in favour of making some 
kind of change to acpi_enforce_resources. Proposal, what if we change the auto 
setting to not only mean "strict" when an ATK0110 interface is present, but 
also when the ACPI thermal zone interface is present ?

Thanks & Regards,

Hans

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

On Thu, 29 Jan 2009 19:58:00 +0100, Hans de Goede wrote:
> Thomas Renninger wrote:
> >>> Either this should be more generic (instead of hardcoded "ATK0110"
> >>> device, use a list to add further specific thermal ACPI drivers). Hmm,
> >>> maybe it's only ASUS violating the spec?
> >> ASUS it's not actually violating any spec...
> > They have to export the temperature through a thermal ACPI
> > device, not through the ASUS specific ATK0110 device. This is
> > what I mean whether there might be other vendor specific ACPI
> > devices violating the spec (by providing temperature read
> > functionality which should be done through the generic thermal
> > ACPI device).
> > 
> 
> hwmon IC's monitor far more then just temperatures, ASUS is doing the right 
> thing here by providing an ACPI interface to also read voltages and fan speeds, 
> so that these can be read in a way that does not interfere with the ACPI code.
> 
> And although even their interface does not expose the full functionality of the 
> hwmon IC's, it is much much better then what the thermal ACPI code gives us.
> 
> >>> Thinkpads seem to read out extra thermal
> >>> data from EC and shouldn't interfere with hwmon drivers.
> >> The point is that there is only 1 physical sensor, which both ACPI and
> >> a native driver want to drive; transaction on SMBus to read the sensor
> >> are usually in the form "select bank" + "read" and the sequence is
> >> *not* atomic. In ASUS case the IO ports are correctly reserved by the
> >> firmware, but (historically) this wasn't taken into account.
> > I know about this problem.
> > 
> >> Aside from ASUS the problem is generic since there are two drivers
> >> poking at the same hardware; for example there are reports of native
> >> drivers interfering with normal TZ polling (see [1])
> > Yes, this is even worse as the check that you want to add will not
> > catch it.
> 
> Ack, so the proper solution would be to just make the acpi_enforce_resources 
> strict by default, but this may cause lack of functionality for some user who 
> are currently using native drivers for hwmon features. Which is why I (hwmon 
> subsytem contributor and Jean Delvare (i2c and (ex)hwmon subsystem maintainer) 
> proposed this auto setting as a compromise. I'm fine with changing the default 
> to strict, AFAIK Jean prefers the auto setting.
> 
> <snip>
> 
> >> At least eeepc and (some?) thinkpads have ACPI
> >> hwmon capabilities but I don't know whether there are native drivers
> >> available or not (but they could be "blacklisted"  preemptively like
> >> ASUS ATK).
> > I expect that ASUS only will interfere with specific hwmon driver(s)?
> 
> Yes only those used on their motherboards, and given the wide range of 
> motherboard ASUS produces and they offer the ATK0110 interface on almost all of 
> them, it pretty much comes down to "all hwmon and smbus drivers", although I'm 
> sure if we look we can find exceptions.
> 
> > So why don't you move the check into the hwmon driver and make it not
> > load on these systems?
> > It's still ugly, but IMO better than to pollute the ACPI core.
> > 
> 
> Because we do not want to do this in a zillion places when it should be done in 
> only one. The right solution is to make acpi_enforce_resources strict the 
> default, the auto setting is meant as a slow migration path to that right 
> solution, as doing that right now probably will cause lots of complaints.
> 
> Actually, the really right solution would be for the ACPI subsystem to actually 
> claim all the resources using the regular resource tracking so that drivers who 
> want to check for resource conflicts with ACPI do not have to explicitly call
> acpi_check_resource_conflict() (and friends), but instead will get an error 
> when trying to claim the resources from the regular resource system. However 
> chances are this will break things on quite a lot of systems, still it would be 
> the right thing to do in the end IMHO.
> 
> <snip>
> 
> > If this is an ASUS only problem with very specific thermal
> > sensors, better add the quirk at the specific sensor driver.
> > 
> 
> It is neither ASUS specific, not is it limited to specific hwmon IC's, these 
> are not thermal sensors, they are capable of monitoring far more then just 
> temperature, which is why the ACPI thermal device interface is insufficient.
> 
> > PS: I just realize that in ATK0110 the same thermal device
> > (IO port 0x22D) is used as in the generic ACPI device (_TMP).
> > So it looks like your asus thermal driver will not only interfere
> > with the hwmon driver, but with the ACPI thermal driver itself.
> 
> I would expect the ACPI code to take care of any conflicts between the various 
> interfaces it offers itself. And if it is unsafe to call multiple ACPI methods 
> at the same time, I would expect the ACPI core to fix this.

For the records, I second everything Hans wrote above.

> Also note that the above paragraph actually advocates in favour of making some 
> kind of change to acpi_enforce_resources. Proposal, what if we change the auto 
> setting to not only mean "strict" when an ATK0110 interface is present, but 
> also when the ACPI thermal zone interface is present ?

I expect problems if you do that without any exception. There are a
number of desktop motherboards out there with pretty broken TZ
implementations. To mention just one, my Jetway K8M8MS motherboard has a
TZ which reports 9 degrees C all the time. I disassembled the DSDT to
find out that the code is incorrect and they are reading a
configuration register instead of the temperature register. Apparently
customers didn't care at all, as this was never fixed. I reported to
Jetway but didn't get any answer.

More generally, the ACPI thermal zone doesn't provide a tenth of the
functionality of native hardware monitoring drivers on desktop and
server systems. So switching to strict when a thermal zone is found
would be as painful as switching to strict unconditionally, in that it
will cause a loss of functionality for many users out there. Whoever
does this will first have to become the i2c maintainer and the hwmon
maintainer, and then take all the flames. That won't be me for sure.

Thanks,
-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Thomas Renninger]

On Thursday 29 January 2009 19:58:00 Hans de Goede wrote:
> Thomas Renninger wrote:
> >>> Either this should be more generic (instead of hardcoded "ATK0110"
> >>> device, use a list to add further specific thermal ACPI drivers). Hmm,
> >>> maybe it's only ASUS violating the spec?
> >>
> >> ASUS it's not actually violating any spec...
> >
> > They have to export the temperature through a thermal ACPI
> > device, not through the ASUS specific ATK0110 device. This is
> > what I mean whether there might be other vendor specific ACPI
> > devices violating the spec (by providing temperature read
> > functionality which should be done through the generic thermal
> > ACPI device).
>
> hwmon IC's monitor far more then just temperatures, ASUS is doing the right
> thing here by providing an ACPI interface to also read voltages and fan
> speeds, so that these can be read in a way that does not interfere with the
> ACPI code.
>
> And although even their interface does not expose the full functionality of
> the hwmon IC's, it is much much better then what the thermal ACPI code
> gives us.
>
> >>> Thinkpads seem to read out extra thermal
> >>> data from EC and shouldn't interfere with hwmon drivers.
> >>
> >> The point is that there is only 1 physical sensor, which both ACPI and
> >> a native driver want to drive; transaction on SMBus to read the sensor
> >> are usually in the form "select bank" + "read" and the sequence is
> >> *not* atomic. In ASUS case the IO ports are correctly reserved by the
> >> firmware, but (historically) this wasn't taken into account.
> >
> > I know about this problem.
> >
> >> Aside from ASUS the problem is generic since there are two drivers
> >> poking at the same hardware; for example there are reports of native
> >> drivers interfering with normal TZ polling (see [1])
> >
> > Yes, this is even worse as the check that you want to add will not
> > catch it.
>
> Ack, so the proper solution would be to just make the
> acpi_enforce_resources strict by default, but this may cause lack of
> functionality for some user who are currently using native drivers for
> hwmon features. Which is why I (hwmon subsytem contributor and Jean Delvare
> (i2c and (ex)hwmon subsystem maintainer) proposed this auto setting as a
> compromise. I'm fine with changing the default to strict, AFAIK Jean
> prefers the auto setting.
>
> <snip>
>
> >> At least eeepc and (some?) thinkpads have ACPI
> >> hwmon capabilities but I don't know whether there are native drivers
> >> available or not (but they could be "blacklisted"  preemptively like
> >> ASUS ATK).
> >
> > I expect that ASUS only will interfere with specific hwmon driver(s)?
>
> Yes only those used on their motherboards, and given the wide range of
> motherboard ASUS produces and they offer the ATK0110 interface on almost
> all of them, it pretty much comes down to "all hwmon and smbus drivers",
> although I'm sure if we look we can find exceptions.
I thought it may only be one specific hwmon driver which could
interfere on ASUS boards with the ATK0110 device. I agree that
it would not make sense to cluster all hwmon drivers.

The problem is similar to the video backlight switching problem
(legacy drivers vs preferred generic ACPI video driver) where
you also must know which driver to take before module loading time.

I hoped to be able to come up with something more clever or say a nicer
solution or at least a short discussion. But I also have no better idea.
Maybe all such code should end up in a drivers/acpi/quirks.c file
at some time, similar to other places in the kernel.

I think it's time for Len looking at this. I wonder whether he will take
this patch or why he won't.
This is an ugly problem which unfortunately needs an ugly fix/check.

This code snippet should be part of your ATK0110 driver then and
not submitted/committed standalone?

Please take me into CC the next time you submit your asus-laptop/acpi driver, 
I like to have a look at it and can also give it a test.

> > So why don't you move the check into the hwmon driver and make it not
> > load on these systems?
> > It's still ugly, but IMO better than to pollute the ACPI core.
>
> Because we do not want to do this in a zillion places when it should be
> done in only one. The right solution is to make acpi_enforce_resources
> strict the default, the auto setting is meant as a slow migration path to
> that right solution, as doing that right now probably will cause lots of
> complaints.


> Actually, the really right solution would be for the ACPI subsystem to
> actually claim all the resources using the regular resource tracking so
> that drivers who want to check for resource conflicts with ACPI do not have
> to explicitly call acpi_check_resource_conflict() (and friends), but
> instead will get an error when trying to claim the resources from the
> regular resource system. However chances are this will break things on
> quite a lot of systems, still it would be the right thing to do in the end
> IMHO.
>
> <snip>
>
> > If this is an ASUS only problem with very specific thermal
> > sensors, better add the quirk at the specific sensor driver.
>
> It is neither ASUS specific, not is it limited to specific hwmon IC's,
> these are not thermal sensors, they are capable of monitoring far more then
> just temperature, which is why the ACPI thermal device interface is
> insufficient.
>
> > PS: I just realize that in ATK0110 the same thermal device
> > (IO port 0x22D) is used as in the generic ACPI device (_TMP).
> > So it looks like your asus thermal driver will not only interfere
> > with the hwmon driver, but with the ACPI thermal driver itself.
>
> I would expect the ACPI code to take care of any conflicts between the
> various interfaces it offers itself. And if it is unsafe to call multiple
> ACPI methods at the same time, I would expect the ACPI core to fix this.
Yes. ACPI can easily make sure that not several resources are accessed at
the same time. It's the ACPI vs native OS code where this is so hard.

    Thomas

> Also note that the above paragraph actually advocates in favour of making
> some kind of change to acpi_enforce_resources. Proposal, what if we change
> the auto setting to not only mean "strict" when an ATK0110 interface is
> present, but also when the ACPI thermal zone interface is present ?

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

On Fri, Jan 30, 2009 at 03:29:13PM +0100, Thomas Renninger wrote: 
> I thought it may only be one specific hwmon driver which could
> interfere on ASUS boards with the ATK0110 device. I agree that
> it would not make sense to cluster all hwmon drivers.
> 
> The problem is similar to the video backlight switching problem
> (legacy drivers vs preferred generic ACPI video driver) where
> you also must know which driver to take before module loading time.
> 
> I hoped to be able to come up with something more clever or say a nicer
> solution or at least a short discussion. But I also have no better idea.
> Maybe all such code should end up in a drivers/acpi/quirks.c file
> at some time, similar to other places in the kernel.
> 
> I think it's time for Len looking at this. I wonder whether he will take
> this patch or why he won't.
> This is an ugly problem which unfortunately needs an ugly fix/check.

I've rewritten the patch, the idea behind it is still the same but the
solution is more generic. The code checks a list of DMI entries and
(optionally) a HID for each entry; if the DMI data matches and the HID
(if not NULL) is found then resource checking is set to strict, otherwise
it falls back to lax.

---
Just RFC, it's still UNTESTED...

 drivers/acpi/osl.c |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6729a49..c6d1a1c 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
  * in arbitrary AML code and can interfere with legacy drivers.
  * acpi_enforce_resources= can be set to:
  *
- *   - strict           (2)
+ *   - auto             (2)
+ *     -> detect possible conflicts with ACPI drivers and switch to
+ *     strict if needed, otherwise act like lax
+ *   - strict           (3)
  *     -> further driver trying to access the resources will not load
  *   - lax (default)    (1)
  *     -> further driver trying to access the resources will load, but you
@@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
  *     -> ACPI Operation Region resources will not be registered
  *
  */
-#define ENFORCE_RESOURCES_STRICT 2
+#define ENFORCE_RESOURCES_STRICT 3
+#define ENFORCE_RESOURCES_AUTO   2
 #define ENFORCE_RESOURCES_LAX    1
 #define ENFORCE_RESOURCES_NO     0
 
-static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
+static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
 
 static int __init acpi_enforce_resources_setup(char *str)
 {
@@ -1086,6 +1090,8 @@ static int __init acpi_enforce_resources_setup(char *str)
 
 	if (!strcmp("strict", str))
 		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
+	else if (!strcmp("auto", str))
+		acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
 	else if (!strcmp("lax", str))
 		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
 	else if (!strcmp("no", str))
@@ -1096,6 +1102,73 @@ static int __init acpi_enforce_resources_setup(char *str)
 
 __setup("acpi_enforce_resources=", acpi_enforce_resources_setup);
 
+static acpi_status acpi_res_quick_check_hid_cb(acpi_handle obj_handle,
+		u32 nesting_level, void *context, void **return_value)
+{
+	*((bool *)return_value) = true;
+	return AE_CTRL_TERMINATE;
+}
+
+static int acpi_res_quick_check_hid(const struct dmi_system_id *d)
+{
+	acpi_status ret;
+	bool found = false;
+	char *hid;
+
+	if (!d->driver_data)
+		goto strict;
+
+	hid = d->driver_data;
+	ret = acpi_get_devices(hid, acpi_res_quick_check_hid_cb,
+				NULL, (void **)&found);
+
+	if (ret == AE_OK && found)
+		goto strict;
+
+	return 0;
+strict:
+	printk(KERN_DEBUG "ACPI: detected %s system: "
+			"enforcing strict resource checking\n", d->ident);
+	return 1;
+}
+
+/* The following systems have ACPI drivers that might overlap the
+ * functionality of native drivers (mostly in hwmon subsys). If
+ * acpi_enforce_resources is set to auto the following table is
+ * used to enforce strict checking in matching systems, in order
+ * to avoid conflicts.
+ * Note that driver_data and the acpi_res_quick_check_hid callback
+ * are used to further refine the match, checking for the presence
+ * of the given HID (driver_data) in the DSDT. Both fields are
+ * optional for DMI-only rules.
+ */
+static const struct dmi_system_id resource_quirks[] = {
+	{
+		.ident = "Asus",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK Computer"),
+		},
+		.driver_data = "ATK0110",
+		.callback = acpi_res_quick_check_hid,
+	},
+	{ }
+};
+
+static int __init acpi_apply_resource_quirk(void)
+{
+	if (acpi_enforce_resources != ENFORCE_RESOURCES_AUTO)
+		return 0;
+
+	dmi_check_system(resource_quirks);
+
+	if (acpi_enforce_resources == ENFORCE_RESOURCES_AUTO)
+		/* This system is not listed, fallback to 'lax' */
+		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
+
+	return 0;
+}
+fs_initcall(acpi_apply_resource_quirk);
+
 /* Check for resource conflicts between ACPI OperationRegions and native
  * drivers */
 int acpi_check_resource_conflict(struct resource *res)

Luca
-- 
Mi piace avere amici rispettabili;
Mi piace essere il peggiore della compagnia.

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

Hi Luca,

On Sun, 1 Feb 2009 22:22:43 +0100, Luca Tettamanti wrote:
> I've rewritten the patch, the idea behind it is still the same but the
> solution is more generic. The code checks a list of DMI entries and
> (optionally) a HID for each entry; if the DMI data matches and the HID
> (if not NULL) is found then resource checking is set to strict, otherwise
> it falls back to lax.
> 
> ---
> Just RFC, it's still UNTESTED...
> 
>  drivers/acpi/osl.c |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 6729a49..c6d1a1c 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
>   * in arbitrary AML code and can interfere with legacy drivers.
>   * acpi_enforce_resources= can be set to:
>   *
> - *   - strict           (2)
> + *   - auto             (2)
> + *     -> detect possible conflicts with ACPI drivers and switch to
> + *     strict if needed, otherwise act like lax
> + *   - strict           (3)
>   *     -> further driver trying to access the resources will not load
>   *   - lax (default)    (1)
>   *     -> further driver trying to access the resources will load, but you
> @@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
>   *     -> ACPI Operation Region resources will not be registered
>   *
>   */
> -#define ENFORCE_RESOURCES_STRICT 2
> +#define ENFORCE_RESOURCES_STRICT 3
> +#define ENFORCE_RESOURCES_AUTO   2
>  #define ENFORCE_RESOURCES_LAX    1
>  #define ENFORCE_RESOURCES_NO     0

I don't see any reason to change ENFORCE_RESOURCES_STRICT from 2 to 3.
Just add ENFORCE_RESOURCES_AUTO as 3 and that's it, makes your patch
smaller.

>  
> -static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>  
>  static int __init acpi_enforce_resources_setup(char *str)
>  {
> @@ -1086,6 +1090,8 @@ static int __init acpi_enforce_resources_setup(char *str)
>  
>  	if (!strcmp("strict", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
> +	else if (!strcmp("auto", str))
> +		acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>  	else if (!strcmp("lax", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
>  	else if (!strcmp("no", str))
> @@ -1096,6 +1102,73 @@ static int __init acpi_enforce_resources_setup(char *str)
>  
>  __setup("acpi_enforce_resources=", acpi_enforce_resources_setup);
>  
> +static acpi_status acpi_res_quick_check_hid_cb(acpi_handle obj_handle,
> +		u32 nesting_level, void *context, void **return_value)
> +{
> +	*((bool *)return_value) = true;
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static int acpi_res_quick_check_hid(const struct dmi_system_id *d)
> +{
> +	acpi_status ret;
> +	bool found = false;
> +	char *hid;
> +
> +	if (!d->driver_data)
> +		goto strict;
> +
> +	hid = d->driver_data;
> +	ret = acpi_get_devices(hid, acpi_res_quick_check_hid_cb,
> +				NULL, (void **)&found);
> +
> +	if (ret == AE_OK && found)
> +		goto strict;
> +
> +	return 0;
> +strict:
> +	printk(KERN_DEBUG "ACPI: detected %s system: "
> +			"enforcing strict resource checking\n", d->ident);

Should be pr_debug().

> +	return 1;
> +}
> +
> +/* The following systems have ACPI drivers that might overlap the
> + * functionality of native drivers (mostly in hwmon subsys). If
> + * acpi_enforce_resources is set to auto the following table is
> + * used to enforce strict checking in matching systems, in order
> + * to avoid conflicts.
> + * Note that driver_data and the acpi_res_quick_check_hid callback
> + * are used to further refine the match, checking for the presence
> + * of the given HID (driver_data) in the DSDT. Both fields are
> + * optional for DMI-only rules.
> + */
> +static const struct dmi_system_id resource_quirks[] = {

You must include <linux/dmi.h> for this.

I think it's better to tag this structure as __initdata than const, so
that it is cleaned up after initialization. Then I guess you can also
tag acpi_res_quick_check_hid_cb() and acpi_res_quick_check_hid() as
__init so the runtime memory overhead is zero.

> +	{
> +		.ident = "Asus",
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK Computer"),
> +		},
> +		.driver_data = "ATK0110",
> +		.callback = acpi_res_quick_check_hid,
> +	},
> +	{ }
> +};
> +
> +static int __init acpi_apply_resource_quirk(void)
> +{
> +	if (acpi_enforce_resources != ENFORCE_RESOURCES_AUTO)
> +		return 0;
> +
> +	dmi_check_system(resource_quirks);
> +
> +	if (acpi_enforce_resources == ENFORCE_RESOURCES_AUTO)
> +		/* This system is not listed, fallback to 'lax' */
> +		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +
> +	return 0;
> +}
> +fs_initcall(acpi_apply_resource_quirk);
> +
>  /* Check for resource conflicts between ACPI OperationRegions and native
>   * drivers */
>  int acpi_check_resource_conflict(struct resource *res)

Otherwise it looks good to me, pretty clean and flexible. Good work!

-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

On Mon, Feb 2, 2009 at 10:11 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Sun, 1 Feb 2009 22:22:43 +0100, Luca Tettamanti wrote:
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
>>   * in arbitrary AML code and can interfere with legacy drivers.
>>   * acpi_enforce_resources= can be set to:
>>   *
>> - *   - strict           (2)
>> + *   - auto             (2)
>> + *     -> detect possible conflicts with ACPI drivers and switch to
>> + *     strict if needed, otherwise act like lax
>> + *   - strict           (3)
>>   *     -> further driver trying to access the resources will not load
>>   *   - lax (default)    (1)
>>   *     -> further driver trying to access the resources will load, but you
>> @@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
>>   *     -> ACPI Operation Region resources will not be registered
>>   *
>>   */
>> -#define ENFORCE_RESOURCES_STRICT 2
>> +#define ENFORCE_RESOURCES_STRICT 3
>> +#define ENFORCE_RESOURCES_AUTO   2
>>  #define ENFORCE_RESOURCES_LAX    1
>>  #define ENFORCE_RESOURCES_NO     0
>
> I don't see any reason to change ENFORCE_RESOURCES_STRICT from 2 to 3.
> Just add ENFORCE_RESOURCES_AUTO as 3 and that's it, makes your patch
> smaller.

There's an unspoken reason ;-) The options are ordered by
"strictness", I was experimenting with an API to export the parameter,
in order to move the code to a separate quirk file. Since it's not
relevant in this patch I'll back out the change.

>> +static int acpi_res_quick_check_hid(const struct dmi_system_id *d)
>> +{
>> +     acpi_status ret;
>> +     bool found = false;
>> +     char *hid;
>> +
>> +     if (!d->driver_data)
>> +             goto strict;
>> +
>> +     hid = d->driver_data;
>> +     ret = acpi_get_devices(hid, acpi_res_quick_check_hid_cb,
>> +                             NULL, (void **)&found);
>> +
>> +     if (ret == AE_OK && found)
>> +             goto strict;
>> +
>> +     return 0;
>> +strict:
>> +     printk(KERN_DEBUG "ACPI: detected %s system: "
>> +                     "enforcing strict resource checking\n", d->ident);
>
> Should be pr_debug().

Actually I was considering raising the level to info, it might make
easier to explain to the users why they cannot load their favorite
driver anymore.

>> +     return 1;
>> +}
>> +
>> +/* The following systems have ACPI drivers that might overlap the
>> + * functionality of native drivers (mostly in hwmon subsys). If
>> + * acpi_enforce_resources is set to auto the following table is
>> + * used to enforce strict checking in matching systems, in order
>> + * to avoid conflicts.
>> + * Note that driver_data and the acpi_res_quick_check_hid callback
>> + * are used to further refine the match, checking for the presence
>> + * of the given HID (driver_data) in the DSDT. Both fields are
>> + * optional for DMI-only rules.
>> + */
>> +static const struct dmi_system_id resource_quirks[] = {
>
> You must include <linux/dmi.h> for this.

Right, it's included indirectly from somewhere...

> I think it's better to tag this structure as __initdata than const, so
> that it is cleaned up after initialization. Then I guess you can also
> tag acpi_res_quick_check_hid_cb() and acpi_res_quick_check_hid() as
> __init so the runtime memory overhead is zero.

Ok, will do.

Luca

[PATCH 1/2] RFC: ACPI: Interface for ACPI drivers to place quirk code which gets executed early

[Thomas Renninger]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6620 bytes --]

Hi,
On Monday 02 February 2009 12:38:24 Luca Tettamanti wrote:> On Mon, Feb 2, 2009 at 10:11 AM, Jean Delvare <khali@linux-fr.org> wrote:> > On Sun, 1 Feb 2009 22:22:43 +0100, Luca Tettamanti wrote:> >> --- a/drivers/acpi/osl.c> >> +++ b/drivers/acpi/osl.c> >> @@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on",> >> acpi_wake_gpes_always_on_setup); * in arbitrary AML code and can> >> interfere with legacy drivers. * acpi_enforce_resources= can be set to:> >>   *> >> - *   - strict           (2)> >> + *   - auto             (2)> >> + *     -> detect possible conflicts with ACPI drivers and switch to> >> + *     strict if needed, otherwise act like lax> >> + *   - strict           (3)> >>   *     -> further driver trying to access the resources will not load> >>   *   - lax (default)    (1)> >>   *     -> further driver trying to access the resources will load, but> >> you @@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on",> >> acpi_wake_gpes_always_on_setup); *     -> ACPI Operation Region> >> resources will not be registered *> >>   */> >> -#define ENFORCE_RESOURCES_STRICT 2> >> +#define ENFORCE_RESOURCES_STRICT 3> >> +#define ENFORCE_RESOURCES_AUTO   2> >>  #define ENFORCE_RESOURCES_LAX    1> >>  #define ENFORCE_RESOURCES_NO     0> >> > I don't see any reason to change ENFORCE_RESOURCES_STRICT from 2 to 3.> > Just add ENFORCE_RESOURCES_AUTO as 3 and that's it, makes your patch> > smaller.>> There's an unspoken reason ;-) The options are ordered by> "strictness", I was experimenting with an API to export the parameter,> in order to move the code to a separate quirk file. Since it's not> relevant in this patch I'll back out the change.
I am still running an fsck as I filled up the fs I worked on to 100%, whichtakes hours for the 300G partition. I hope I didn't mess up something, dueto the full fs, but I do not want to wait until tomorrow, you already mightwant to have a look at this.
These two patches are tested on a ASUS machine and worked as expected,but probably may still need some cleanup.
There is the problem that the quirks are not executed for deviceswhich are not present. But this is due to the poor ACPI hotplug design,which hopefully will be reworked at some time.
Both are against Len's latest ACPI git test branch.
Len, if you like these, tell me and I am going to make themcheck_patch clean and I will send them again.
Thanks,
    Thomas
---
ACPI: Interface for ACPI drivers to place quirk code which gets executed early
Some ACPI drivers need ACPI namespace info before the driver is actuallyloaded.Best examples are legacy vs ACPI driver competitors like ATK0110 vs hwmondrivers and the generic ACPI video driver vs laptop drivers.There may also pop up other examples in the future where this interface willbecome convenient.
Signed-off-by: Thomas Renninger <trenn@suse.de>
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefileindex 65d90c7..498cc6f 100644--- a/drivers/acpi/Makefile+++ b/drivers/acpi/Makefile@@ -59,3 +59,4 @@ obj-$(CONFIG_ACPI_HOTPLUG_MEMORY)	+= acpi_memhotplug.o obj-$(CONFIG_ACPI_PROCFS_POWER)	+= cm_sbs.o obj-$(CONFIG_ACPI_SBS)		+= sbshc.o obj-$(CONFIG_ACPI_SBS)		+= sbs.o+obj-y                           += quirks.o\ No newline at end of filediff --git a/drivers/acpi/acpi.h b/drivers/acpi/acpi.hnew file mode 100644index 0000000..95e29ca--- /dev/null+++ b/drivers/acpi/acpi.h@@ -0,0 +1,8 @@+#ifndef _LINUX_LOCAL_ACPI_H+#define _LINUX_LOCAL_ACPI_H++#include <linux/init.h>++void __init acpi_device_quirks(void);++#endif /* _LINUX_LOCAL_ACPI_H */diff --git a/drivers/acpi/quirks.c b/drivers/acpi/quirks.cnew file mode 100644index 0000000..844480e--- /dev/null+++ b/drivers/acpi/quirks.c@@ -0,0 +1,73 @@+/*+ *  Copyright (c) 2008 Thomas Renninger <trenn@suse.de>+ *+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~+ *+ *  This program is free software; you can redistribute it and/or modify+ *  it under the terms of the GNU General Public License as published by+ *  the Free Software Foundation; either version 2 of the License, or+ *  (at your option) any later version.+ *+ *  This program is distributed in the hope that it will be useful,+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the+ *  GNU General Public License for more details.+ *+ *  You should have received a copy of the GNU General Public License+ *  along with this program; if not, write to the Free Software+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA+ *+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~+ */++#include <linux/mod_devicetable.h>+#include <acpi/acpi_bus.h>+#include "acpi.h"++struct acpi_device_fixup {+	const char *hid;+	int (*fixup_cb)(struct acpi_device*);+};++/*+ * Add a callback here if your module needs to process code after the ACPI+ * core has parsed the DSDT and initialized all devices, but the code must+ * be processed before module load time.+ * Good candiates are ACPI vs legacy driver decisions which must be made+ * before the driver is loaded.+ */+const struct acpi_device_fixup __initdata fixup_table[] = {+};++static acpi_status __init+acpi_device_quirks_callback(acpi_handle handle,+			    u32 level, void *ctxt, void **retv)+{+	acpi_status status;+	struct acpi_device *dev;+	struct acpi_device_id dev_id;+	int x, ret;++	status = acpi_bus_get_device(handle, &dev);+	if (ACPI_FAILURE(status))+		return AE_OK;++	for (x = 0; x < ARRAY_SIZE(fixup_table); x++) {+		memcpy(&dev_id.id, fixup_table[x].hid, ACPI_ID_LEN);+		if (!acpi_match_device_ids(dev, &dev_id)) {+			ret = fixup_table[x].fixup_cb(dev);+			if (ret)+				printk (KERN_ERR "Fixup failed for device: "+					"%s\n", fixup_table[x].hid);+		}+	}+	return AE_OK;+}++void __init acpi_device_quirks(void) {++	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,+			    ACPI_UINT32_MAX,+			    acpi_device_quirks_callback,+			    NULL, NULL);+}diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.cindex c54d7b6..1c25747 100644--- a/drivers/acpi/scan.c+++ b/drivers/acpi/scan.c@@ -10,6 +10,7 @@ #include <linux/kthread.h>  #include <acpi/acpi_drivers.h>+#include "acpi.h"  #define _COMPONENT		ACPI_BUS_COMPONENT ACPI_MODULE_NAME("scan");@@ -1562,6 +1563,8 @@ static int __init acpi_scan_init(void)  	if (result) 		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);+	else+		acpi_device_quirks();        Done: 	return result;\0ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] RFC: ACPI: Interface for ACPI drivers to place quirk code which gets executed early

[Luca Tettamanti]

Il Mon, Feb 02, 2009 at 06:22:10PM +0100, Thomas Renninger ha scritto: 
> These two patches are tested on a ASUS machine and worked as expected,
> but probably may still need some cleanup.

I'd keep the DMI+HID approach since it's more flexible:
- (AFAICS) Thinkpads have different methods for hwmon depending on the
  model and no fixed HID
- With DMI it would be possible to include ASUS motherboards (ATK w/
  hwmon) but exclude ASUS laptops (ATK w/o hwmon).

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index c54d7b6..1c25747 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -10,6 +10,7 @@
>  #include <linux/kthread.h>
>  
>  #include <acpi/acpi_drivers.h>
> +#include "acpi.h"
>  
>  #define _COMPONENT		ACPI_BUS_COMPONENT
>  ACPI_MODULE_NAME("scan");
> @@ -1562,6 +1563,8 @@ static int __init acpi_scan_init(void)
>  
>  	if (result)
>  		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);
> +	else
> +		acpi_device_quirks();

Hum, it's not immediatly clear why you put that call in the else
branch. Maybe put:

if (!result)
        acpi_device_quirks();

before the cleanup?

Luca
-- 
"La mia teoria scientifica preferita e` quella secondo la quale gli 
 anelli di Saturno sarebbero interamente composti dai bagagli andati 
 persi nei viaggi aerei." -- Mark Russel

Re: [PATCH 1/2] RFC: ACPI: Interface for ACPI drivers to place quirk code which gets executed early

[Thomas Renninger]

On Monday 02 February 2009 21:22:46 Luca Tettamanti wrote:
> Il Mon, Feb 02, 2009 at 06:22:10PM +0100, Thomas Renninger ha scritto:
> > These two patches are tested on a ASUS machine and worked as expected,
> > but probably may still need some cleanup.
>
> I'd keep the DMI+HID approach since it's more flexible:
> - (AFAICS) Thinkpads have different methods for hwmon depending on the
>   model and no fixed HID
> - With DMI it would be possible to include ASUS motherboards (ATK w/
>   hwmon) but exclude ASUS laptops (ATK w/o hwmon).
I thought the ATK01[01]0 devices are ASUS specific.
I now found an ATK0100 (not the ATK0110 this is about) on a Sony and a
Samsung.
I still wonder why you want to restrict the check to ASUS.
Your ATK0110 driver would also load on any other machine which has such
a device. And why shouldn't a Samsung/Sony/... machine with a ATK0110
device not access the hwmon sensor through it?
Should we already look a bit deeper into the ATK0110 device in the quirk
to make sure it provides thermal, fan or other hwmon device accessing
functionality?

> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index c54d7b6..1c25747 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/kthread.h>
> >
> >  #include <acpi/acpi_drivers.h>
> > +#include "acpi.h"
> >
> >  #define _COMPONENT		ACPI_BUS_COMPONENT
> >  ACPI_MODULE_NAME("scan");
> > @@ -1562,6 +1563,8 @@ static int __init acpi_scan_init(void)
> >
> >  	if (result)
> >  		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);
> > +	else
> > +		acpi_device_quirks();
>
> Hum, it's not immediatly clear why you put that call in the else
> branch. Maybe put:
>
> if (!result)
>         acpi_device_quirks();
>
> before the cleanup?
Yes that looks ugly, yours is nicer...

Also thanks for the "auto not handled properly" hint, I forgot
that part.

Thanks,

    Thomas

Re: [PATCH 1/2] RFC: ACPI: Interface for ACPI drivers to place quirk code which gets executed early

[Luca Tettamanti]

On Tue, Feb 3, 2009 at 2:08 PM, Thomas Renninger <trenn@suse.de> wrote:
> On Monday 02 February 2009 21:22:46 Luca Tettamanti wrote:
>> Il Mon, Feb 02, 2009 at 06:22:10PM +0100, Thomas Renninger ha scritto:
>> > These two patches are tested on a ASUS machine and worked as expected,
>> > but probably may still need some cleanup.
>>
>> I'd keep the DMI+HID approach since it's more flexible:
>> - (AFAICS) Thinkpads have different methods for hwmon depending on the
>>   model and no fixed HID
>> - With DMI it would be possible to include ASUS motherboards (ATK w/
>>   hwmon) but exclude ASUS laptops (ATK w/o hwmon).
> I thought the ATK01[01]0 devices are ASUS specific.

Me too, I was thinking about using DMI_CHASSIS_TYPE to restrict the
match only to motherboard (not notebooks).

> I now found an ATK0100 (not the ATK0110 this is about) on a Sony and a
> Samsung.
> I still wonder why you want to restrict the check to ASUS.
> Your ATK0110 driver would also load on any other machine which has such
> a device. And why shouldn't a Samsung/Sony/... machine with a ATK0110
> device not access the hwmon sensor through it?

Ah, I was unaware that it was used also by other vendors. It the same
interface though? Do you have a dump of the DSDT of such machines?

> Should we already look a bit deeper into the ATK0110 device in the quirk
> to make sure it provides thermal, fan or other hwmon device accessing
> functionality?

Hum, maybe it's the best thing to do. Hans,  Jean what do you think?

Luca

Re: [PATCH 1/2] RFC: ACPI: Interface for ACPI drivers to place quirk code which gets executed early

[Jean Delvare]

On Tue, 3 Feb 2009 14:45:17 +0100, Luca Tettamanti wrote:
> On Tue, Feb 3, 2009 at 2:08 PM, Thomas Renninger <trenn@suse.de> wrote:
> > On Monday 02 February 2009 21:22:46 Luca Tettamanti wrote:
> >> Il Mon, Feb 02, 2009 at 06:22:10PM +0100, Thomas Renninger ha scritto:
> >> > These two patches are tested on a ASUS machine and worked as expected,
> >> > but probably may still need some cleanup.
> >>
> >> I'd keep the DMI+HID approach since it's more flexible:
> >> - (AFAICS) Thinkpads have different methods for hwmon depending on the
> >>   model and no fixed HID
> >> - With DMI it would be possible to include ASUS motherboards (ATK w/
> >>   hwmon) but exclude ASUS laptops (ATK w/o hwmon).
> > I thought the ATK01[01]0 devices are ASUS specific.
> 
> Me too, I was thinking about using DMI_CHASSIS_TYPE to restrict the
> match only to motherboard (not notebooks).
> 
> > I now found an ATK0100 (not the ATK0110 this is about) on a Sony and a
> > Samsung.
> > I still wonder why you want to restrict the check to ASUS.
> > Your ATK0110 driver would also load on any other machine which has such
> > a device. And why shouldn't a Samsung/Sony/... machine with a ATK0110
> > device not access the hwmon sensor through it?
> 
> Ah, I was unaware that it was used also by other vendors. It the same
> interface though? Do you have a dump of the DSDT of such machines?
> 
> > Should we already look a bit deeper into the ATK0110 device in the quirk
> > to make sure it provides thermal, fan or other hwmon device accessing
> > functionality?
> 
> Hum, maybe it's the best thing to do. Hans,  Jean what do you think?

I'd suggest we first check whether the other vendor implementations are
compatible or not. Then we will know if we must set the resource check
to strict for these as well.

I would still recommend vendor-based tests. Given that ACPI device
names aren't standard, we can't exclude that different vendors come up
with different devices using the same name. So checking only the device
names seem too risky in general (even though it might work OK for
specific cases such as the ATK0110.)

-- 
Jean Delvare

Re: [PATCH 1/2] RFC: ACPI: Interface for ACPI drivers to place quirk code which gets executed early

[Thomas Renninger]

Hi,

On Monday 02 February 2009 21:22:46 Luca Tettamanti wrote:
> Il Mon, Feb 02, 2009 at 06:22:10PM +0100, Thomas Renninger ha scritto:
> > These two patches are tested on a ASUS machine and worked as expected,
> > but probably may still need some cleanup.
>
> I'd keep the DMI+HID approach since it's more flexible:
> - (AFAICS) Thinkpads have different methods for hwmon depending on the
>   model and no fixed HID
> - With DMI it would be possible to include ASUS motherboards (ATK w/
>   hwmon) but exclude ASUS laptops (ATK w/o hwmon).
>
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index c54d7b6..1c25747 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/kthread.h>
> >
> >  #include <acpi/acpi_drivers.h>
> > +#include "acpi.h"
> >
> >  #define _COMPONENT		ACPI_BUS_COMPONENT
> >  ACPI_MODULE_NAME("scan");
> > @@ -1562,6 +1563,8 @@ static int __init acpi_scan_init(void)
> >
> >  	if (result)
> >  		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);
> > +	else
> > +		acpi_device_quirks();
>
> Hum, it's not immediatly clear why you put that call in the else
> branch. Maybe put:
>
> if (!result)
>         acpi_device_quirks();
>
> before the cleanup?

Looking at this again (and trying to convert the video_detect things into
that) I am not sure whether this is the right place.
While I'd prefer to not touch the video_detect stuff right now because:
  - it could easily be extended to provide amount of brightness levels
    (I sent test code a while ago) to take them into account into the
    native vs ACPI driver decision.
  - Therefore it's better to let the drivers call it and not implement
    it as a standalone quirk
  - It needs the PCI subsystem initialized

Still I like my approach for the ATK (and upcoming similar?) issue(s).
I wonder whether it should be called later still.
Should this be called early, after adding the devices like above or
after PCI acpi and PNP acpi initialization?
Then it needs to be triggered by a separate fs_initcall?

     Thomas

[PATCH 2/2] RFC: ACPI: Set enforce_resources to strict if a ATK0110 device is found in namespace

[Thomas Renninger]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6217 bytes --]

On Monday 02 February 2009 12:38:24 you wrote:> On Mon, Feb 2, 2009 at 10:11 AM, Jean Delvare <khali@linux-fr.org> wrote:> > On Sun, 1 Feb 2009 22:22:43 +0100, Luca Tettamanti wrote:> >> --- a/drivers/acpi/osl.c> >> +++ b/drivers/acpi/osl.c> >> @@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on",> >> acpi_wake_gpes_always_on_setup); * in arbitrary AML code and can> >> interfere with legacy drivers. * acpi_enforce_resources= can be set to:> >>   *> >> - *   - strict           (2)> >> + *   - auto             (2)> >> + *     -> detect possible conflicts with ACPI drivers and switch to> >> + *     strict if needed, otherwise act like lax> >> + *   - strict           (3)> >>   *     -> further driver trying to access the resources will not load> >>   *   - lax (default)    (1)> >>   *     -> further driver trying to access the resources will load, but> >> you @@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on",> >> acpi_wake_gpes_always_on_setup); *     -> ACPI Operation Region> >> resources will not be registered *> >>   */> >> -#define ENFORCE_RESOURCES_STRICT 2> >> +#define ENFORCE_RESOURCES_STRICT 3> >> +#define ENFORCE_RESOURCES_AUTO   2> >>  #define ENFORCE_RESOURCES_LAX    1> >>  #define ENFORCE_RESOURCES_NO     0> >> > I don't see any reason to change ENFORCE_RESOURCES_STRICT from 2 to 3.> > Just add ENFORCE_RESOURCES_AUTO as 3 and that's it, makes your patch> > smaller.>> There's an unspoken reason ;-) The options are ordered by> "strictness", I was experimenting with an API to export the parameter,> in order to move the code to a separate quirk file. Since it's not> relevant in this patch I'll back out the change.
And the other one, implementing the ATK0110 quirk.The same probably could be easy done now with video_detect.c.The video device HID should get added in scan.c again as suggested by Bjorn.Then the video_detect.c can go into quirks.c and get deleted.
What do you think?
     Thomas
---ACPI: Set enforce_resources to strict if a ATK0110 device is found in namespace
ATK0110 acpi driver on ASUS boards touches the hwmon HW via ACPI functions.Every native hwmon driver which tries to acquire resources which are alreadydefined as OperationRegions will not load on these boards by default.This avoids interference between non-atomic data/command register readsbetween the hwmon and the ATK0110 Asus ACPI drivers.
Signed-off-by: Thomas Renninger <trenn@suse.de>
diff --git a/drivers/acpi/acpi.h b/drivers/acpi/acpi.hindex 95e29ca..c08037e 100644--- a/drivers/acpi/acpi.h+++ b/drivers/acpi/acpi.h@@ -5,4 +5,28 @@  void __init acpi_device_quirks(void); +/* Check of resource interference between native drivers and ACPI+ * OperationRegions (SystemIO and System Memory only).+ * IO ports and memory declared in ACPI might be used by the ACPI subsystem+ * in arbitrary AML code and can interfere with legacy drivers.+ * acpi_enforce_resources= can be set to:+ *+ *   - strict           (2)+ *     -> further driver trying to access the resources will not load+ *   - lax (default)    (1)+ *     -> further driver trying to access the resources will load, but you+ *     get a system message that something might go wrong...+ *+ *   - no               (0)+ *     -> ACPI Operation Region resources will not be registered+ *+ */+#define ENFORCE_RESOURCES_AUTO   3+#define ENFORCE_RESOURCES_STRICT 2+#define ENFORCE_RESOURCES_LAX    1+#define ENFORCE_RESOURCES_NO     0++extern unsigned int acpi_enforce_resources;++ #endif /* _LINUX_LOCAL_ACPI_H */diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.cindex 6729a49..92e4226 100644--- a/drivers/acpi/osl.c+++ b/drivers/acpi/osl.c@@ -50,6 +50,7 @@ #include <acpi/acpi.h> #include <acpi/acpi_bus.h> #include <acpi/processor.h>+#include "acpi.h"  #define _COMPONENT		ACPI_OS_SERVICES ACPI_MODULE_NAME("osl");@@ -1057,27 +1058,7 @@ static int __init acpi_wake_gpes_always_on_setup(char *str)  __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup); -/* Check of resource interference between native drivers and ACPI- * OperationRegions (SystemIO and System Memory only).- * IO ports and memory declared in ACPI might be used by the ACPI subsystem- * in arbitrary AML code and can interfere with legacy drivers.- * acpi_enforce_resources= can be set to:- *- *   - strict           (2)- *     -> further driver trying to access the resources will not load- *   - lax (default)    (1)- *     -> further driver trying to access the resources will load, but you- *     get a system message that something might go wrong...- *- *   - no               (0)- *     -> ACPI Operation Region resources will not be registered- *- */-#define ENFORCE_RESOURCES_STRICT 2-#define ENFORCE_RESOURCES_LAX    1-#define ENFORCE_RESOURCES_NO     0--static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;+unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;  static int __init acpi_enforce_resources_setup(char *str) {diff --git a/drivers/acpi/quirks.c b/drivers/acpi/quirks.cindex 844480e..a0d0731 100644--- a/drivers/acpi/quirks.c+++ b/drivers/acpi/quirks.c@@ -30,6 +30,20 @@ struct acpi_device_fixup { };  /*+ * ATK0110 is known to interfere with several hwmon drivers, as it also+ * reads temperture, fan, etc. on the same device hwmon drivers do.+ * Make sure possibly interfering hwmon/i2c/smbus drivers touching IO areas+ * which already got declared as ACPI Operation Regions cannot be loaded.+ */+int __init atk0110_interfere_fixups (struct acpi_device *dev) {++	printk ("Found device: %s\n", acpi_device_bid(dev));+	if (acpi_enforce_resources == ENFORCE_RESOURCES_AUTO)+		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;+	return 0;+}++/*  * Add a callback here if your module needs to process code after the ACPI  * core has parsed the DSDT and initialized all devices, but the code must  * be processed before module load time.@@ -37,6 +51,7 @@ struct acpi_device_fixup {  * before the driver is loaded.  */ const struct acpi_device_fixup __initdata fixup_table[] = {+	{ "ATK0110", atk0110_interfere_fixups }, };  static acpi_status __init\0ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] RFC: ACPI: Set enforce_resources to strict if a ATK0110 device is found in namespace

[Luca Tettamanti]

Il Mon, Feb 02, 2009 at 06:22:19PM +0100, Thomas Renninger ha scritto: 
> On Monday 02 February 2009 12:38:24 you wrote:
> > On Mon, Feb 2, 2009 at 10:11 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > > On Sun, 1 Feb 2009 22:22:43 +0100, Luca Tettamanti wrote:
> > >> --- a/drivers/acpi/osl.c
> > >> +++ b/drivers/acpi/osl.c
> > >> @@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on",
> > >> acpi_wake_gpes_always_on_setup); * in arbitrary AML code and can
> > >> interfere with legacy drivers. * acpi_enforce_resources= can be set to:
> > >>   *
> > >> - *   - strict           (2)
> > >> + *   - auto             (2)
> > >> + *     -> detect possible conflicts with ACPI drivers and switch to
> > >> + *     strict if needed, otherwise act like lax
> > >> + *   - strict           (3)
> > >>   *     -> further driver trying to access the resources will not load
> > >>   *   - lax (default)    (1)
> > >>   *     -> further driver trying to access the resources will load, but
> > >> you @@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on",
> > >> acpi_wake_gpes_always_on_setup); *     -> ACPI Operation Region
> > >> resources will not be registered *
> > >>   */
> > >> -#define ENFORCE_RESOURCES_STRICT 2
> > >> +#define ENFORCE_RESOURCES_STRICT 3
> > >> +#define ENFORCE_RESOURCES_AUTO   2
> > >>  #define ENFORCE_RESOURCES_LAX    1
> > >>  #define ENFORCE_RESOURCES_NO     0
> > >
> > > I don't see any reason to change ENFORCE_RESOURCES_STRICT from 2 to 3.
> > > Just add ENFORCE_RESOURCES_AUTO as 3 and that's it, makes your patch
> > > smaller.
> >
> > There's an unspoken reason ;-) The options are ordered by
> > "strictness", I was experimenting with an API to export the parameter,
> > in order to move the code to a separate quirk file. Since it's not
> > relevant in this patch I'll back out the change.
> 
> And the other one, implementing the ATK0110 quirk.
> The same probably could be easy done now with video_detect.c.
> The video device HID should get added in scan.c again as suggested by Bjorn.
> Then the video_detect.c can go into quirks.c and get deleted.
> 
> What do you think?

Well mostly ACK, modulo DMI comment in my other email.

> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 6729a49..92e4226 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
[...]
> -static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>  
>  static int __init acpi_enforce_resources_setup(char *str)
>  {

You're missing the code to parse "auto" here.

I also have this patch from a previous email:

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index d8362cf..10fd2d7 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -258,6 +258,25 @@ and is between 256 and 4096 characters. It is defined in the file
 			to assume that this machine's pmtimer latches its value
 			and always returns good values.
 
+	acpi_enforce_resources=	[ACPI]
+			{ strict, auto, lax, no }
+			Check for resource conflicts between native drivers
+			and ACPI OperationRegions (SystemIO and SystemMemory
+			only). IO ports and memory declared in ACPI might be
+			used by the ACPI subsystem in arbitrary AML code and
+			can interfere with legacy drivers.
+			strict: access to resources claimed by ACPI is denied;
+			legacy drivers trying to access reserved resources
+			will fail to load.
+			auto (default): try and detect ACPI devices with known
+			ACPI drivers; escalates the setting to "strict" if such
+			a device is found, otherwise behaves like "lax".
+			lax: access to resources claimed by ACPI is allowed;
+			legacy drivers trying to access reserved resources
+			will load and a warning message is logged.
+			no: ACPI OperationRegions are not marked as reserved,
+			no further checks are performed.
+
 	agp=		[AGP]
 			{ off | try_unsupported }
 			off: disable AGP support

Luca
-- 
"L'amore consiste nell'essere cretini insieme." -- P. Valery

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Thomas Renninger]

On Sunday 01 February 2009 22:22:43 Luca Tettamanti wrote:
> On Fri, Jan 30, 2009 at 03:29:13PM +0100, Thomas Renninger wrote:
> > I thought it may only be one specific hwmon driver which could
> > interfere on ASUS boards with the ATK0110 device. I agree that
> > it would not make sense to cluster all hwmon drivers.
> >
> > The problem is similar to the video backlight switching problem
> > (legacy drivers vs preferred generic ACPI video driver) where
> > you also must know which driver to take before module loading time.
> >
> > I hoped to be able to come up with something more clever or say a nicer
> > solution or at least a short discussion. But I also have no better idea.
> > Maybe all such code should end up in a drivers/acpi/quirks.c file
> > at some time, similar to other places in the kernel.
> >
> > I think it's time for Len looking at this. I wonder whether he will take
> > this patch or why he won't.
> > This is an ugly problem which unfortunately needs an ugly fix/check.
>
> I've rewritten the patch, the idea behind it is still the same but the
> solution is more generic. The code checks a list of DMI entries and
> (optionally) a HID for each entry; if the DMI data matches and the HID
> (if not NULL) is found then resource checking is set to strict, otherwise
> it falls back to lax.
Please wait.
I am currently on a solution which would encapsulate such workarounds a bit 
more into an own file.

Give me an hour or two, I hope to be able to come up with something until
then.

    Thomas

>
> ---
> Just RFC, it's still UNTESTED...
>
>  drivers/acpi/osl.c |   79
> ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 76
> insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 6729a49..c6d1a1c 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on",
> acpi_wake_gpes_always_on_setup); * in arbitrary AML code and can interfere
> with legacy drivers.
>   * acpi_enforce_resources= can be set to:
>   *
> - *   - strict           (2)
> + *   - auto             (2)
> + *     -> detect possible conflicts with ACPI drivers and switch to
> + *     strict if needed, otherwise act like lax
> + *   - strict           (3)
>   *     -> further driver trying to access the resources will not load
>   *   - lax (default)    (1)
>   *     -> further driver trying to access the resources will load, but you
> @@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on",
> acpi_wake_gpes_always_on_setup); *     -> ACPI Operation Region resources
> will not be registered
>   *
>   */
> -#define ENFORCE_RESOURCES_STRICT 2
> +#define ENFORCE_RESOURCES_STRICT 3
> +#define ENFORCE_RESOURCES_AUTO   2
>  #define ENFORCE_RESOURCES_LAX    1
>  #define ENFORCE_RESOURCES_NO     0
>
> -static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>
>  static int __init acpi_enforce_resources_setup(char *str)
>  {
> @@ -1086,6 +1090,8 @@ static int __init acpi_enforce_resources_setup(char
> *str)
>
>  	if (!strcmp("strict", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
> +	else if (!strcmp("auto", str))
> +		acpi_enforce_resources = ENFORCE_RESOURCES_AUTO;
>  	else if (!strcmp("lax", str))
>  		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
>  	else if (!strcmp("no", str))
> @@ -1096,6 +1102,73 @@ static int __init acpi_enforce_resources_setup(char
> *str)
>
>  __setup("acpi_enforce_resources=", acpi_enforce_resources_setup);
>
> +static acpi_status acpi_res_quick_check_hid_cb(acpi_handle obj_handle,
> +		u32 nesting_level, void *context, void **return_value)
> +{
> +	*((bool *)return_value) = true;
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static int acpi_res_quick_check_hid(const struct dmi_system_id *d)
> +{
> +	acpi_status ret;
> +	bool found = false;
> +	char *hid;
> +
> +	if (!d->driver_data)
> +		goto strict;
> +
> +	hid = d->driver_data;
> +	ret = acpi_get_devices(hid, acpi_res_quick_check_hid_cb,
> +				NULL, (void **)&found);
> +
> +	if (ret == AE_OK && found)
> +		goto strict;
> +
> +	return 0;
> +strict:
> +	printk(KERN_DEBUG "ACPI: detected %s system: "
> +			"enforcing strict resource checking\n", d->ident);
> +	return 1;
> +}
> +
> +/* The following systems have ACPI drivers that might overlap the
> + * functionality of native drivers (mostly in hwmon subsys). If
> + * acpi_enforce_resources is set to auto the following table is
> + * used to enforce strict checking in matching systems, in order
> + * to avoid conflicts.
> + * Note that driver_data and the acpi_res_quick_check_hid callback
> + * are used to further refine the match, checking for the presence
> + * of the given HID (driver_data) in the DSDT. Both fields are
> + * optional for DMI-only rules.
> + */
> +static const struct dmi_system_id resource_quirks[] = {
> +	{
> +		.ident = "Asus",
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK Computer"),
> +		},
> +		.driver_data = "ATK0110",
> +		.callback = acpi_res_quick_check_hid,
> +	},
> +	{ }
> +};
> +
> +static int __init acpi_apply_resource_quirk(void)
> +{
> +	if (acpi_enforce_resources != ENFORCE_RESOURCES_AUTO)
> +		return 0;
> +
> +	dmi_check_system(resource_quirks);
> +
> +	if (acpi_enforce_resources == ENFORCE_RESOURCES_AUTO)
> +		/* This system is not listed, fallback to 'lax' */
> +		acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +
> +	return 0;
> +}
> +fs_initcall(acpi_apply_resource_quirk);
> +
>  /* Check for resource conflicts between ACPI OperationRegions and native
>   * drivers */
>  int acpi_check_resource_conflict(struct resource *res)
>
> Luca

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

Il Thu, Jan 29, 2009 at 05:29:23PM +0100, Thomas Renninger ha scritto: 
> On Thursday 29 January 2009 16:16:38 Luca Tettamanti wrote:
> > On Thu, Jan 29, 2009 at 11:30 AM, Thomas Renninger <trenn@suse.de> wrote:
> > > On Sunday 25 January 2009 22:05:20 Luca Tettamanti wrote:
> > >> (This is an improved version of my earlier patch, I've reworked deviced
> > >> detection to simply check for the desired HID)
> > >>
> > >> The following patch adds "auto" option to "acpi_enforce_resource"; this
> > >> value is also the new default.
> > >> "auto" enforces strict resource checking - disallowing access by native
> > >> drivers to IO ports and memory regions claimed by ACPI firmware - when a
> > >> device with a known ACPI driver is found (currently only ASUS ATK0110 is
> > >> checked), and reverts to lax checking otherwise.
> > >>
> > >> The patch is mainly aimed to block native hwmon drivers from touching
> > >> monitoring chips that ACPI thinks it own.
> > >
> > > Having this in the core ACPI code is ugly.
> >
> > I think we all agree :-)
> >
> > > Either this should be more generic (instead of hardcoded "ATK0110"
> > > device, use a list to add further specific thermal ACPI drivers). Hmm,
> > > maybe it's only ASUS violating the spec?
> >
> > ASUS it's not actually violating any spec...
> They have to export the temperature through a thermal ACPI
> device, not through the ASUS specific ATK0110 device. This is
> what I mean whether there might be other vendor specific ACPI
> devices violating the spec (by providing temperature read
> functionality which should be done through the generic thermal
> ACPI device).

Ah, I see. As Hans stated the ATK device covers more than just
temperatures and it seems that Asus intends to keep it (the interface is
evolving and Windows utils use it). The pattern seems to be _TMP for
notebook (but not the rest of the hwmon stuff) and full hwmon (but not
_TMP) for desktop. Your M2V is actually an exception...

> > The EC does not
> > make any difference, since a native driver would speak directly to the
> > monitoring chip, effectively by-passing the EC.
> > Now, in principle "strict" is the correct behaviour for the resource
> > checking code, but it would break many working setups leaving the
> > users without any kind of hw monitoring. The "auto" hack is meant to
> > force the users to migrate to the ACPI driver...
> >
> > > Are there other known, model specific ACPI devices,
> > > accessing thermal IOs directly which could interfere with hwmon, then it
> > > might be worth it.
> >
> > Right now "auto" is ASUS-specific because *I* know that there any many
> > different native drivers that works on various boards (and I wrote the
> > the ACPI driver...);
> Hmm, grep -r ATK0110 drivers/platform/x86 in latest ACPI test tree is
> empty, can you give me a pointer to a recent version of your driver.

It's not upstream yet; the idea was to disable native drivers (with
"auto") and merge the driver at the same time; otherwise it might not be
safe to use it. I'm attaching the patch for reference.

> > At least eeepc and (some?) thinkpads have ACPI
> > hwmon capabilities but I don't know whether there are native drivers
> > available or not (but they could be "blacklisted"  preemptively like
> > ASUS ATK).
> I expect that ASUS only will interfere with specific hwmon driver(s)?
> So why don't you move the check into the hwmon driver and make it not
> load on these systems?
> It's still ugly, but IMO better than to pollute the ACPI core.

That would involve disabling just about everything... we don't wont to
touch sensors that are owned by the firmware, but ATM it's not possible
to enforce strict resource validation since it may break unrelated
stuff.

> Here some thermal DSDT parts of a recent ASUS machine:
> 
> The temperature exported through the generic thermal ACPI device:
> 
>     OperationRegion (IP, SystemIO, 0x022D, 0x02)
>     Field (IP, ByteAcc, NoLock, Preserve)
>     {
>         INDX,   8,
>         DAT0,   8
>     }
> 
>     Method (SBYT, 2, NotSerialized)
>     {
>         Store (Arg0, INDX)
>         Store (Arg1, DAT0)
>     }
> 
>     Method (GBYT, 1, NotSerialized)
>     {
>         Store (Arg0, INDX)
>         Store (DAT0, Local0)
>         Return (Local0)
>     }
> 
> Method (_TMP, 0, NotSerialized)
>     {
>     /* makes use of RTMP which uses GBYT and SBYT to actually read
>        the temp
>     */
>     }
> 
> There is another RTMP function in ATK0110 (do you use that?)

There are actually 2 interfaces: the older one is based on
TSIF/FSIF/VSIF for enumerating the available sensors (temp, fan,
voltage) and RTMP/RFAN/RVLT for reading; the newer one uses two giant
demux (GGRP and GITM) for enumerating and reading.
There's a lot of other stuff (bus and memory frequencies, fan control,
OC profiles) but it's not used by the driver implemented (yet).

> On the M2A-VM-HDMI machine where DSDT extracts from above are,
> running libsensors I get:
> 
> Driver `it87' (should be inserted):
>   Detects correctly:
>   * ISA bus, address 0x228
>     Chip `ITE IT8716F Super IO Sensors' (confidence: 9)
> 
> Driver `to-be-written' (should be inserted):
>   Detects correctly:
>   * Chip `AMD K10 thermal sensors' (confidence: 9)
> 
> If this is an ASUS only problem with very specific thermal
> sensors, better add the quirk at the specific sensor driver.
> 
> If this is something general, then maybe you can convince Len
> to add something to the ACPI core, but this should be more
> generic then.

The point is: resource validation should be strict since we have no idea
of what AML might be doing. But we also don't want to face hordes of
angry users, so the proposed "auto" trick enforces strict validation when
we have an alternative and falls back to lax otherwise.

Now, both eeepc and thinkpad platform drivers provide fan managent via
ACPI (and temperatures via standard thermal zones I suppose), so they
could use the same treatment.

> PS: I just realize that in ATK0110 the same thermal device
> (IO port 0x22D) is used as in the generic ACPI device (_TMP).
> So it looks like your asus thermal driver will not only interfere
> with the hwmon driver, but with the ACPI thermal driver itself.

Unless I'm mistaked method execution is serialized by the interpreter,
so they won't interfere with each other. If something else (maybe a SMI
handler?) can execute the same stuff they'd better use a mutex to
synchronize the execution (regardless of any additional driver).

(driver follows)
---
Asus boards have an ACPI interface for interacting with the hwmon (fan,
temperatures, voltages) subsystem; this driver exposes the relevant
information via the standard sysfs interface.

There are two different ACPI interfaces:
- an old one (based on RVLT/RFAN/RTMP)
- a new one (GGRP/GITM)
Both may be present but there a few cases (my board, sigh) where the
new interface is just an empty stub; the driver defaults to the old one
when both are present.
The old interface has received a considerable testing, but I'm still
awaiting confirmation from my tester that the new one is working as
expected (hence the debug code is still enabled).

Currently all the attributes are read-only, though a (partial) control
should be possible with a bit more work.

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
---
 drivers/acpi/acpica/nsxfeval.c |    3 
 drivers/hwmon/Kconfig          |   12 
 drivers/hwmon/Makefile         |    1 
 drivers/hwmon/asus_atk0110.c   | 1015 +++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpixf.h          |    2 
 5 files changed, 1029 insertions(+), 4 deletions(-)

Index: b/include/acpi/acpixf.h
===================================================================
--- a/include/acpi/acpixf.h	2009-01-17 18:38:24.518880624 +0100
+++ b/include/acpi/acpixf.h	2009-01-17 19:05:14.922881979 +0100
@@ -187,14 +187,12 @@
 		     struct acpi_object_list *parameter_objects,
 		     struct acpi_buffer *return_object_buffer);
 
-#ifdef ACPI_FUTURE_USAGE
 acpi_status
 acpi_evaluate_object_typed(acpi_handle object,
 			   acpi_string pathname,
 			   struct acpi_object_list *external_params,
 			   struct acpi_buffer *return_buffer,
 			   acpi_object_type return_type);
-#endif
 
 acpi_status
 acpi_get_object_info(acpi_handle handle, struct acpi_buffer *return_buffer);
Index: b/drivers/hwmon/asus_atk0110.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/drivers/hwmon/asus_atk0110.c	2009-01-26 21:30:59.246505545 +0100
@@ -0,0 +1,1015 @@
+/*
+ * Copyright (C) 2007-2009 Luca Tettamanti <kronos.it@gmail.com>
+ *
+ * This file is released under the GPLv2
+ * See COPYING in the top level directory of the kernel tree.
+ */
+
+#define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/hwmon.h>
+#include <linux/list.h>
+#include <linux/module.h>
+
+#include <acpi/acpi.h>
+#include <acpi/acpixf.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/acpi_bus.h>
+
+
+#define ATK_HID "ATK0110"
+#define ATK_DRV "atk-hwmon"
+
+/* Minimum time between readings, enforced in order to avoid
+ * hogging the CPU.
+ */
+#define CACHE_TIME		HZ
+
+#define BOARD_ID		"MBIF"
+#define METHOD_ENUMERATE	"GGRP"
+#define METHOD_READ		"GITM"
+#define METHOD_WRITE		"SITM"
+#define METHOD_OLD_READ_TMP	"RTMP"
+#define METHOD_OLD_READ_VLT	"RVLT"
+#define METHOD_OLD_READ_FAN	"RFAN"
+#define METHOD_OLD_ENUM_TMP	"TSIF"
+#define METHOD_OLD_ENUM_VLT	"VSIF"
+#define METHOD_OLD_ENUM_FAN	"FSIF"
+
+#define ATK_MUX_HWMON		0x00000006ULL
+
+#define ATK_CLASS_MASK		0xff000000ULL
+#define ATK_CLASS_FREQ_CTL	0x03000000ULL
+#define ATK_CLASS_FAN_CTL	0x04000000ULL
+#define ATK_CLASS_HWMON		0x06000000ULL
+
+#define ATK_TYPE_MASK		0x00ff0000ULL
+#define HWMON_TYPE_VOLT		0x00020000ULL
+#define HWMON_TYPE_TEMP		0x00030000ULL
+#define HWMON_TYPE_FAN		0x00040000ULL
+
+#define HWMON_SENSOR_ID_MASK	0x0000ffffULL
+
+enum atk_pack_member {
+	HWMON_PACK_FLAGS,
+	HWMON_PACK_NAME,
+	HWMON_PACK_LIMIT1,
+	HWMON_PACK_LIMIT2,
+	HWMON_PACK_ENABLE
+};
+
+/* New package format */
+#define _HWMON_NEW_PACK_SIZE	7
+#define _HWMON_NEW_PACK_FLAGS	0
+#define _HWMON_NEW_PACK_NAME	1
+#define _HWMON_NEW_PACK_UNK1	2
+#define _HWMON_NEW_PACK_UNK2	3
+#define _HWMON_NEW_PACK_LIMIT1	4
+#define _HWMON_NEW_PACK_LIMIT2	5
+#define _HWMON_NEW_PACK_ENABLE	6
+
+/* Old package format */
+#define _HWMON_OLD_PACK_SIZE	5
+#define _HWMON_OLD_PACK_FLAGS	0
+#define _HWMON_OLD_PACK_NAME	1
+#define _HWMON_OLD_PACK_LIMIT1	2
+#define _HWMON_OLD_PACK_LIMIT2	3
+#define _HWMON_OLD_PACK_ENABLE	4
+
+
+struct atk_data {
+	struct device *hwmon_dev;
+	acpi_handle atk_handle;
+	struct acpi_device *acpi_dev;
+
+	bool old_interface;
+
+	/* old interface */
+	acpi_handle rtmp_handle;
+	acpi_handle rvlt_handle;
+	acpi_handle rfan_handle;
+	/* new inteface */
+	acpi_handle enumerate_handle;
+	acpi_handle read_handle;
+
+	int voltage_count;
+	int temperature_count;
+	int fan_count;
+	struct list_head sensor_list;
+};
+
+
+typedef ssize_t (*sysfs_show_func)(struct device *dev,
+			struct device_attribute *attr, char *buf);
+
+static const struct acpi_device_id atk_ids[] = {
+	{ATK_HID, 0},
+	{"", 0},
+};
+MODULE_DEVICE_TABLE(acpi, atk_ids);
+
+#define ATTR_NAME_SIZE 16 /* Worst case is "tempN_input" */
+
+struct atk_sensor_data {
+	struct list_head list;
+	struct atk_data *data;
+	struct device_attribute label_attr;
+	struct device_attribute input_attr;
+	struct device_attribute limit1_attr;
+	struct device_attribute limit2_attr;
+	char label_attr_name[ATTR_NAME_SIZE];
+	char input_attr_name[ATTR_NAME_SIZE];
+	char limit1_attr_name[ATTR_NAME_SIZE];
+	char limit2_attr_name[ATTR_NAME_SIZE];
+	u64 id;
+	u64 type;
+	u64 limit1;
+	u64 limit2;
+	u64 cached_value;
+	unsigned long last_updated; /* in jiffies */
+	bool is_valid;
+	char const *acpi_name;
+};
+
+struct atk_acpi_buffer_u64 {
+	union acpi_object buf;
+	u64 value;
+};
+
+static int atk_add(struct acpi_device *device);
+static int atk_remove(struct acpi_device *device, int type);
+static void atk_print_sensor(struct atk_data *data, union acpi_object *obj);
+static int atk_read_value(struct atk_sensor_data *sensor, u64 *value);
+static void atk_free_sensors(struct atk_data *data);
+
+static struct acpi_driver atk_driver = {
+	.name	= ATK_HID,
+	.class	= "hwmon",
+	.ids	= atk_ids,
+	.ops	= {
+		.add	= atk_add,
+		.remove	= atk_remove,
+	},
+};
+
+#define input_to_atk_sensor(attr) \
+	container_of(attr, struct atk_sensor_data, input_attr)
+
+#define label_to_atk_sensor(attr) \
+	container_of(attr, struct atk_sensor_data, label_attr)
+
+#define limit1_to_atk_sensor(attr) \
+	container_of(attr, struct atk_sensor_data, limit1_attr)
+
+#define limit2_to_atk_sensor(attr) \
+	container_of(attr, struct atk_sensor_data, limit2_attr)
+
+static ssize_t atk_input_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct atk_sensor_data *s = input_to_atk_sensor(attr);
+	u64 value;
+	int err;
+
+	err = atk_read_value(s, &value);
+	if (err)
+		return err;
+
+	if (s->type == HWMON_TYPE_TEMP)
+		/* ACPI returns decidegree */
+		value *= 100;
+
+	return sprintf(buf, "%llu\n", value);
+}
+
+static ssize_t atk_label_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct atk_sensor_data *s = label_to_atk_sensor(attr);
+
+	return sprintf(buf, "%s\n", s->acpi_name);
+}
+
+static ssize_t atk_limit1_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct atk_sensor_data *s = limit1_to_atk_sensor(attr);
+	u64 value = s->limit1;
+
+	if (s->type == HWMON_TYPE_TEMP)
+		value *= 100;
+
+	return sprintf(buf, "%lld\n", value);
+}
+
+static ssize_t atk_limit2_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct atk_sensor_data *s = limit2_to_atk_sensor(attr);
+	u64 value = s->limit2;
+
+	if (s->type == HWMON_TYPE_TEMP)
+		value *= 100;
+
+	return sprintf(buf, "%lld\n", value);
+}
+
+static ssize_t atk_name_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "atk0110\n");
+}
+static struct device_attribute atk_name_attr = __ATTR(name, 0444, atk_name_show, NULL);
+
+static void atk_init_attribute(struct device_attribute *attr, char *name,
+		sysfs_show_func show)
+{
+	attr->attr.name = name;
+	attr->attr.mode = 0444;
+	attr->show = show;
+	attr->store = NULL;
+}
+
+
+static union acpi_object *atk_get_pack_member(struct atk_data *data,
+						union acpi_object *pack,
+						enum atk_pack_member m)
+{
+	bool old_if = data->old_interface;
+	int offset;
+
+	switch (m) {
+	case HWMON_PACK_FLAGS:
+		offset = old_if ? _HWMON_OLD_PACK_FLAGS : _HWMON_NEW_PACK_FLAGS;
+		break;
+	case HWMON_PACK_NAME:
+		offset = old_if ? _HWMON_OLD_PACK_NAME : _HWMON_NEW_PACK_NAME;
+		break;
+	case HWMON_PACK_LIMIT1:
+		offset = old_if ? _HWMON_OLD_PACK_LIMIT1 : _HWMON_NEW_PACK_LIMIT1;
+		break;
+	case HWMON_PACK_LIMIT2:
+		offset = old_if ? _HWMON_OLD_PACK_LIMIT2 : _HWMON_NEW_PACK_LIMIT2;
+		break;
+	case HWMON_PACK_ENABLE:
+		offset = old_if ? _HWMON_OLD_PACK_ENABLE : _HWMON_NEW_PACK_ENABLE;
+		break;
+	default:
+		return NULL;
+	}
+
+	return &pack->package.elements[offset];
+}
+
+
+/* New package format is:
+ * - flag (int)
+ *	class - used for de-muxing the request to the correct GITn
+ *	type (volt, temp, fan)
+ *	sensor id |
+ *	sensor id - used for de-muxing the request _inside_ the GITn
+ * - name (str)
+ * - unknown (int)
+ * - unknown (int)
+ * - limit1 (int)
+ * - limit2 (int)
+ * - enable (int)
+ *
+ * The old package has the same format but it's missing the two unknown fields.
+ */
+static int validate_hwmon_pack(struct atk_data *data, union acpi_object *obj)
+{
+	struct device *dev = &data->acpi_dev->dev;
+	union acpi_object *tmp;
+	bool old_if = data->old_interface;
+	int const expected_size = old_if ? _HWMON_OLD_PACK_SIZE : _HWMON_NEW_PACK_SIZE;
+
+	if (obj->type != ACPI_TYPE_PACKAGE) {
+		dev_warn(dev, "Invalid type: %d\n", obj->type);
+		return -EINVAL;
+	}
+
+	if (obj->package.count != expected_size) {
+		dev_warn(dev, "Invalid package size: %d, expected: %d\n",
+				obj->package.count, expected_size);
+		return -EINVAL;
+	}
+
+	tmp = atk_get_pack_member(data, obj, HWMON_PACK_FLAGS);
+	if (tmp->type != ACPI_TYPE_INTEGER) {
+		dev_warn(dev, "Invalid type (flag): %d\n", tmp->type);
+		return -EINVAL;
+	}
+
+	tmp = atk_get_pack_member(data, obj, HWMON_PACK_NAME);
+	if (tmp->type != ACPI_TYPE_STRING) {
+		dev_warn(dev, "Invalid type (name): %d\n", tmp->type);
+		return -EINVAL;
+	}
+
+	/* Don't check... we don't know what they're useful for anyway */
+#if 0
+	tmp = &obj->package.elements[HWMON_PACK_UNK1];
+	if (tmp->type != ACPI_TYPE_INTEGER) {
+		dev_warn(dev, "Invalid type (unk1): %d\n", tmp->type);
+		return -EINVAL;
+	}
+
+	tmp = &obj->package.elements[HWMON_PACK_UNK2];
+	if (tmp->type != ACPI_TYPE_INTEGER) {
+		dev_warn(dev, "Invalid type (unk2): %d\n", tmp->type);
+		return -EINVAL;
+	}
+#endif
+
+	tmp = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT1);
+	if (tmp->type != ACPI_TYPE_INTEGER) {
+		dev_warn(dev, "Invalid type (limit1): %d\n", tmp->type);
+		return -EINVAL;
+	}
+
+	tmp = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT2);
+	if (tmp->type != ACPI_TYPE_INTEGER) {
+		dev_warn(dev, "Invalid type (limit2): %d\n", tmp->type);
+		return -EINVAL;
+	}
+
+	tmp = atk_get_pack_member(data, obj, HWMON_PACK_ENABLE);
+	if (tmp->type != ACPI_TYPE_INTEGER) {
+		dev_warn(dev, "Invalid type (enable): %d\n", tmp->type);
+		return -EINVAL;
+	}
+
+	atk_print_sensor(data, obj);
+
+	return 0;
+}
+
+static char const *atk_sensor_type(union acpi_object *flags)
+{
+	u64 type = flags->integer.value & ATK_TYPE_MASK;
+	char const *what;
+
+	switch (type) {
+	case HWMON_TYPE_VOLT:
+		what = "voltage";
+		break;
+	case HWMON_TYPE_TEMP:
+		what = "temperature";
+		break;
+	case HWMON_TYPE_FAN:
+		what = "fan";
+		break;
+	default:
+		what = "unknown";
+		break;
+	}
+
+	return what;
+}
+
+static void atk_print_sensor(struct atk_data *data, union acpi_object *obj)
+{
+#ifdef DEBUG
+	struct device *dev = &data->acpi_dev->dev;
+	union acpi_object *flags;
+	union acpi_object *name;
+	union acpi_object *limit1;
+	union acpi_object *limit2;
+	union acpi_object *enable;
+	char const *what;
+
+	flags = atk_get_pack_member(data, obj, HWMON_PACK_FLAGS);
+	name = atk_get_pack_member(data, obj, HWMON_PACK_NAME);
+	limit1 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT1);
+	limit2 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT2);
+	enable = atk_get_pack_member(data, obj, HWMON_PACK_ENABLE);
+
+	what = atk_sensor_type(flags);
+
+	dev_dbg(dev, "%s: %#llx %s [%llu-%llu] %s\n", what,
+			flags->integer.value,
+			name->string.pointer,
+			limit1->integer.value, limit2->integer.value,
+			enable->integer.value ? "enabled" : "disabled");
+#endif
+}
+
+static int atk_read_value_old(struct atk_sensor_data *sensor, u64 *value)
+{
+	struct atk_data *data = sensor->data;
+	struct device *dev = &data->acpi_dev->dev;
+	struct acpi_object_list params;
+	union acpi_object id;
+	acpi_status status;
+	acpi_handle method;
+
+	switch (sensor->type) {
+	case HWMON_TYPE_VOLT:
+		method = data->rvlt_handle;
+		break;
+	case HWMON_TYPE_TEMP:
+		method = data->rtmp_handle;
+		break;
+	case HWMON_TYPE_FAN:
+		method = data->rfan_handle;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	id.type = ACPI_TYPE_INTEGER;
+	id.integer.value = sensor->id;
+
+	params.count = 1;
+	params.pointer = &id;
+
+	status = acpi_evaluate_integer(method, NULL, &params, value);
+	if (status != AE_OK) {
+		dev_warn(dev, "%s: ACPI exception: %s\n", __func__,
+				acpi_format_exception(status));
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int atk_read_value_new(struct atk_sensor_data *sensor, u64 *value)
+{
+	struct atk_data *data = sensor->data;
+	struct device *dev = &data->acpi_dev->dev;
+	struct acpi_object_list params;
+	struct acpi_buffer ret;
+	union acpi_object id;
+	struct atk_acpi_buffer_u64 tmp;
+	acpi_status status;
+	union acpi_object *o;
+	int i;
+
+	id.type = ACPI_TYPE_INTEGER;
+	id.integer.value = sensor->id;
+
+	params.count = 1;
+	params.pointer = &id;
+
+	tmp.buf.type = ACPI_TYPE_BUFFER;
+	tmp.buf.buffer.pointer = (u8 *)&tmp.value;
+	tmp.buf.buffer.length = sizeof(u64);
+	ret.length = sizeof(tmp);
+	ret.pointer = &tmp;
+
+	status = acpi_evaluate_object_typed(data->read_handle, NULL, &params,
+			&ret, ACPI_TYPE_BUFFER);
+	if (status != AE_OK) {
+		dev_warn(dev, "%s: ACPI exception: %s\n", __func__,
+				acpi_format_exception(status));
+		return -EIO;
+	}
+
+	o = ret.pointer;
+	dev_dbg(dev, "type = %d\n", o->type);
+	dev_dbg(dev, "size = %d\n", o->buffer.length);
+
+	for (i = 0; i < o->buffer.length; i++)
+		dev_dbg(dev, "  [%#x] %d\n", (u32)(o->buffer.pointer[i]),
+				(u32)(o->buffer.pointer[i]));
+
+	/* Return buffer format:
+	 * [0-3] "value" is valid flag
+	 * [4-7] value
+	 */
+	if (!(tmp.value & 0xffffffff)) {
+		/* The reading is not valid, possible causes:
+		 * - sensor failure
+		 * - enumeration was FUBAR (and we didn't notice)
+		 */
+		dev_info(dev, "Failure: %#llx\n", tmp.value);
+		return -EIO;
+	}
+
+	*value = (tmp.value & 0xffffffff00000000ULL) >> 32;
+
+	return 0;
+}
+
+static int atk_read_value(struct atk_sensor_data *sensor, u64 *value)
+{
+	int err;
+
+	if (!sensor->is_valid ||
+	    time_after(jiffies, sensor->last_updated + CACHE_TIME)) {
+		if (sensor->data->old_interface)
+			err = atk_read_value_old(sensor, value);
+		else
+			err = atk_read_value_new(sensor, value);
+
+		sensor->is_valid = true;
+		sensor->last_updated = jiffies;
+		sensor->cached_value = *value;
+	} else {
+		*value = sensor->cached_value;
+		err = 0;
+	}
+
+	return err;
+}
+
+static int atk_add_sensor(struct atk_data *data, union acpi_object *obj)
+{
+	struct device *dev = &data->acpi_dev->dev;
+	union acpi_object *flags;
+	union acpi_object *name;
+	union acpi_object *limit1;
+	union acpi_object *limit2;
+	union acpi_object *enable;
+	struct atk_sensor_data *sensor;
+	char const *base_name;
+	char const *limit1_name;
+	char const *limit2_name;
+	u64 type;
+	int err;
+	int *num;
+	int start;
+
+	if (obj->type != ACPI_TYPE_PACKAGE) {
+		/* wft is this? */
+		dev_warn(dev, "Unknown type for ACPI object: (%d)\n",
+				obj->type);
+		return -EINVAL;
+	}
+
+	err = validate_hwmon_pack(data, obj);
+	if (err)
+		return err;
+
+	/* Ok, we have a valid hwmon package */
+	type = atk_get_pack_member(data, obj, HWMON_PACK_FLAGS)->integer.value & ATK_TYPE_MASK;
+
+	switch (type) {
+	case HWMON_TYPE_VOLT:
+		base_name = "in";
+		limit1_name = "min";
+		limit2_name = "max";
+		num = &data->voltage_count;
+		start = 0;
+		break;
+	case HWMON_TYPE_TEMP:
+		base_name = "temp";
+		limit1_name = "max";
+		limit2_name = "crit";
+		num = &data->temperature_count;
+		start = 1;
+		break;
+	case HWMON_TYPE_FAN:
+		base_name = "fan";
+		limit1_name = "min";
+		limit2_name = "max";
+		num = &data->fan_count;
+		start = 1;
+		break;
+	default:
+		dev_warn(dev, "Unknown sensor type: %#llx\n", type);
+		return -EINVAL;
+	}
+
+	enable = atk_get_pack_member(data, obj, HWMON_PACK_ENABLE);
+	if (!enable->integer.value)
+		/* sensor is disabled */
+		return 0;
+
+	flags = atk_get_pack_member(data, obj, HWMON_PACK_FLAGS);
+	name = atk_get_pack_member(data, obj, HWMON_PACK_NAME);
+	limit1 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT1);
+	limit2 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT2);
+
+	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor->acpi_name = kstrdup(name->string.pointer, GFP_KERNEL);
+	if (!sensor->acpi_name) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&sensor->list);
+	sensor->type = type;
+	sensor->data = data;
+	sensor->id = flags->integer.value;
+	sensor->limit1 = limit1->integer.value;
+	sensor->limit2 = limit2->integer.value;
+
+	snprintf(sensor->input_attr_name, ATTR_NAME_SIZE,
+			"%s%d_input", base_name, start + *num);
+	atk_init_attribute(&sensor->input_attr,
+			sensor->input_attr_name,
+			atk_input_show);
+
+	snprintf(sensor->label_attr_name, ATTR_NAME_SIZE,
+			"%s%d_label", base_name, start + *num);
+	atk_init_attribute(&sensor->label_attr,
+			sensor->label_attr_name,
+			atk_label_show);
+
+	snprintf(sensor->limit1_attr_name, ATTR_NAME_SIZE,
+			"%s%d_%s", base_name, start + *num, limit1_name);
+	atk_init_attribute(&sensor->limit1_attr,
+			sensor->limit1_attr_name,
+			atk_limit1_show);
+
+	snprintf(sensor->limit2_attr_name, ATTR_NAME_SIZE,
+			"%s%d_%s", base_name, start + *num, limit2_name);
+	atk_init_attribute(&sensor->limit2_attr,
+			sensor->limit2_attr_name,
+			atk_limit2_show);
+
+	list_add(&sensor->list, &data->sensor_list);
+	(*num)++;
+
+	return 1;
+out:
+	kfree(sensor->acpi_name);
+	kfree(sensor);
+	return err;
+}
+
+static int atk_enumerate_old_hwmon(struct atk_data *data)
+{
+	struct device *dev = &data->acpi_dev->dev;
+	struct acpi_buffer buf;
+	union acpi_object *pack;
+	acpi_status status;
+	int i, ret;
+	int count = 0;
+
+	/* Voltages */
+	buf.length = ACPI_ALLOCATE_BUFFER;
+	status = acpi_evaluate_object_typed(data->atk_handle,
+			METHOD_OLD_ENUM_VLT, NULL, &buf, ACPI_TYPE_PACKAGE);
+	if (status != AE_OK) {
+		dev_warn(dev, METHOD_OLD_ENUM_VLT ": ACPI exception: %s\n",
+				acpi_format_exception(status));
+
+		return -ENODEV;
+	}
+
+	pack = buf.pointer;
+	for (i = 1; i < pack->package.count; i++) {
+		union acpi_object *obj = &pack->package.elements[i];
+
+		ret = atk_add_sensor(data, obj);
+		if (ret > 0)
+			count++;
+	}
+	ACPI_FREE(buf.pointer);
+
+	/* Temperatures */
+	buf.length = ACPI_ALLOCATE_BUFFER;
+	status = acpi_evaluate_object_typed(data->atk_handle, METHOD_OLD_ENUM_TMP, NULL,
+			&buf, ACPI_TYPE_PACKAGE);
+	if (status != AE_OK) {
+		dev_warn(dev, METHOD_OLD_ENUM_TMP ": ACPI exception: %s\n",
+				acpi_format_exception(status));
+
+		ret = -ENODEV;
+		goto cleanup;
+	}
+
+	pack = buf.pointer;
+	for (i = 1; i < pack->package.count; i++) {
+		union acpi_object *obj = &pack->package.elements[i];
+
+		ret = atk_add_sensor(data, obj);
+		if (ret > 0)
+			count++;
+	}
+	ACPI_FREE(buf.pointer);
+
+	/* Fans */
+	buf.length = ACPI_ALLOCATE_BUFFER;
+	status = acpi_evaluate_object_typed(data->atk_handle, METHOD_OLD_ENUM_FAN, NULL,
+			&buf, ACPI_TYPE_PACKAGE);
+	if (status != AE_OK) {
+		dev_warn(dev, METHOD_OLD_ENUM_FAN ": ACPI exception: %s\n",
+				acpi_format_exception(status));
+
+		ret = -ENODEV;
+		goto cleanup;
+	}
+
+	pack = buf.pointer;
+	for (i = 1; i < pack->package.count; i++) {
+		union acpi_object *obj = &pack->package.elements[i];
+
+		ret = atk_add_sensor(data, obj);
+		if (ret > 0)
+			count++;
+	}
+	ACPI_FREE(buf.pointer);
+
+	return count;
+cleanup:
+	atk_free_sensors(data);
+	return ret;
+}
+
+static int atk_enumerate_new_hwmon(struct atk_data *data)
+{
+	struct device *dev = &data->acpi_dev->dev;
+	struct acpi_buffer buf;
+	acpi_status ret;
+	struct acpi_object_list params;
+	union acpi_object id;
+	union acpi_object *pack;
+	int err;
+	int i;
+
+	dev_dbg(dev, "Enumerating hwmon sensors\n");
+
+	id.type = ACPI_TYPE_INTEGER;
+	id.integer.value = ATK_MUX_HWMON;
+	params.count = 1;
+	params.pointer = &id;
+
+	buf.length = ACPI_ALLOCATE_BUFFER;
+	ret = acpi_evaluate_object_typed(data->enumerate_handle, NULL, &params,
+			&buf, ACPI_TYPE_PACKAGE);
+	if (ret != AE_OK) {
+		dev_warn(dev, METHOD_ENUMERATE ": ACPI exception: %s\n",
+				acpi_format_exception(ret));
+		return -ENODEV;
+	}
+
+	/* Result must be a package */
+	pack = buf.pointer;
+
+	if (pack->package.count < 1) {
+		dev_dbg(dev, "%s: hwmon package is too small: %d\n", __func__,
+				pack->package.count);
+		err = -EINVAL;
+		goto out;
+	}
+
+	for (i = 0; i < pack->package.count; i++) {
+		union acpi_object *obj = &pack->package.elements[i];
+
+		atk_add_sensor(data, obj);
+	}
+
+	err = data->voltage_count + data->temperature_count + data->fan_count;
+
+out:
+	ACPI_FREE(buf.pointer);
+	return err;
+}
+
+static int atk_create_files(struct atk_data *data)
+{
+	struct atk_sensor_data *s;
+	int err;
+
+	list_for_each_entry(s, &data->sensor_list, list) {
+		err = device_create_file(data->hwmon_dev, &s->input_attr);
+		if (err)
+			return err;
+		err = device_create_file(data->hwmon_dev, &s->label_attr);
+		if (err)
+			return err;
+		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
+		if (err)
+			return err;
+		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
+		if (err)
+			return err;
+	}
+
+	err = device_create_file(data->hwmon_dev, &atk_name_attr);
+
+	return err;
+}
+
+static void atk_remove_files(struct atk_data *data)
+{
+	struct atk_sensor_data *s;
+
+	list_for_each_entry(s, &data->sensor_list, list) {
+		device_remove_file(data->hwmon_dev, &s->input_attr);
+		device_remove_file(data->hwmon_dev, &s->label_attr);
+		device_remove_file(data->hwmon_dev, &s->limit1_attr);
+		device_remove_file(data->hwmon_dev, &s->limit2_attr);
+	}
+	device_remove_file(data->hwmon_dev, &atk_name_attr);
+}
+
+static void atk_free_sensors(struct atk_data *data)
+{
+	struct list_head *head = &data->sensor_list;
+	struct atk_sensor_data *s, *tmp;
+
+	list_for_each_entry_safe(s, tmp, head, list) {
+		kfree(s->acpi_name);
+		kfree(s);
+	}
+}
+
+static int atk_register_hwmon(struct atk_data *data)
+{
+	struct device *dev = &data->acpi_dev->dev;
+	int err;
+
+	dev_dbg(dev, "registering hwmon device\n");
+	data->hwmon_dev = hwmon_device_register(dev);
+	if (IS_ERR(data->hwmon_dev))
+		return PTR_ERR(data->hwmon_dev);
+
+	dev_dbg(dev, "populating sysfs directory\n");
+	err = atk_create_files(data);
+	if (err)
+		goto remove;
+
+	return 0;
+remove:
+	/* Cleanup the registered files */
+	atk_remove_files(data);
+	hwmon_device_unregister(data->hwmon_dev);
+	return err;
+}
+
+static int atk_check_old_if(struct atk_data *data)
+{
+	struct device *dev = &data->acpi_dev->dev;
+	acpi_handle ret;
+	acpi_status status;
+
+	/* RTMP: read temperature */
+	status = acpi_get_handle(data->atk_handle, METHOD_OLD_READ_TMP, &ret);
+	if (status != AE_OK) {
+		dev_dbg(dev, "method " METHOD_OLD_READ_TMP " not found: %s\n",
+				acpi_format_exception(status));
+		return -ENODEV;
+	}
+	data->rtmp_handle = ret;
+
+	/* RVLT: read voltage */
+	status = acpi_get_handle(data->atk_handle, METHOD_OLD_READ_VLT, &ret);
+	if (status != AE_OK) {
+		dev_dbg(dev, "method " METHOD_OLD_READ_VLT " not found: %s\n",
+				acpi_format_exception(status));
+		return -ENODEV;
+	}
+	data->rvlt_handle = ret;
+
+	/* RFAN: read fan status */
+	status = acpi_get_handle(data->atk_handle, METHOD_OLD_READ_FAN, &ret);
+	if (status != AE_OK) {
+		dev_dbg(dev, "method " METHOD_OLD_READ_FAN " not found: %s\n",
+				acpi_format_exception(status));
+		return -ENODEV;
+	}
+	data->rfan_handle = ret;
+
+	return 0;
+}
+
+static int atk_check_new_if(struct atk_data *data)
+{
+	struct device *dev = &data->acpi_dev->dev;
+	acpi_handle ret;
+	acpi_status status;
+
+	/* Enumeration */
+	status = acpi_get_handle(data->atk_handle, METHOD_ENUMERATE, &ret);
+	if (status != AE_OK) {
+		dev_dbg(dev, "method " METHOD_ENUMERATE " not found: %s\n",
+				acpi_format_exception(status));
+		return -ENODEV;
+	}
+	data->enumerate_handle = ret;
+
+	/* De-multiplexer (read) */
+	status = acpi_get_handle(data->atk_handle, METHOD_READ, &ret);
+	if (status != AE_OK) {
+		dev_dbg(dev, "method " METHOD_READ " not found: %s\n",
+				acpi_format_exception(status));
+		return -ENODEV;
+	}
+	data->read_handle = ret;
+
+	return 0;
+}
+
+static int atk_add(struct acpi_device *device)
+{
+	acpi_status ret;
+	int err;
+	struct acpi_buffer buf;
+	union acpi_object *obj;
+	struct atk_data *data;
+
+	dev_dbg(&device->dev, "adding...\n");
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->acpi_dev = device;
+	data->atk_handle = device->handle;
+	INIT_LIST_HEAD(&data->sensor_list);
+
+	buf.length = ACPI_ALLOCATE_BUFFER;
+	ret = acpi_evaluate_object_typed(data->atk_handle, BOARD_ID, NULL,
+			&buf, ACPI_TYPE_PACKAGE);
+	if (ret != AE_OK) {
+		dev_dbg(&device->dev, "atk: method MBIF not found\n");
+		err = -ENODEV;
+		goto out;
+	}
+
+	obj = buf.pointer;
+	if (obj->package.count >= 2 &&
+			obj->package.elements[1].type == ACPI_TYPE_STRING) {
+		dev_dbg(&device->dev, "board ID = %s\n",
+				obj->package.elements[1].string.pointer);
+	}
+	ACPI_FREE(buf.pointer);
+
+	/* Check for hwmon methods: first check "old" style methods; note that
+	 * both may be present: in this case we stick to the old interface;
+	 * analysis of multiple DSDTs indicates that when both interfaces
+	 * are present the new one (GGRP/GITM) is not functional.
+	 */
+	err = atk_check_old_if(data);
+	if (!err) {
+		dev_dbg(&device->dev, "Using old hwmon interface\n");
+		data->old_interface = true;
+	} else {
+		err = atk_check_new_if(data);
+		if (err)
+			goto out;
+
+		dev_dbg(&device->dev, "Using new hwmon interface\n");
+		data->old_interface = false;
+	}
+
+	if (data->old_interface)
+		err = atk_enumerate_old_hwmon(data);
+	else
+		err = atk_enumerate_new_hwmon(data);
+	if (err < 0)
+		goto out;
+	if (err == 0) {
+		dev_info(&device->dev, "No usable sensor detected, bailing out\n");
+		err = -ENODEV;
+		goto out;
+	}
+
+	err = atk_register_hwmon(data);
+	if (err)
+		goto cleanup;
+
+	device->driver_data = data;
+	return 0;
+cleanup:
+	atk_free_sensors(data);
+out:
+	kfree(data);
+	return err;
+}
+
+static int atk_remove(struct acpi_device *device, int type)
+{
+	struct atk_data *data = device->driver_data;
+	dev_dbg(&device->dev, "removing...\n");
+
+	device->driver_data = NULL;
+
+	atk_remove_files(data);
+	atk_free_sensors(data);
+	hwmon_device_unregister(data->hwmon_dev);
+
+	kfree(data);
+
+	return 0;
+}
+
+static int atk0110_init(void)
+{
+	int ret;
+
+	ret = acpi_bus_register_driver(&atk_driver);
+	if (ret)
+		pr_info("atk: acpi_bus_register_driver failed: %d\n", ret);
+
+	return ret;
+}
+
+static void atk0110_exit(void)
+{
+	acpi_bus_unregister_driver(&atk_driver);
+}
+
+module_init(atk0110_init);
+module_exit(atk0110_exit);
+
+MODULE_LICENSE("GPL");
Index: b/drivers/hwmon/Kconfig
===================================================================
--- a/drivers/hwmon/Kconfig	2009-01-17 18:38:22.662881222 +0100
+++ b/drivers/hwmon/Kconfig	2009-01-17 19:05:14.926881425 +0100
@@ -248,6 +248,18 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called asb100.
 
+config SENSORS_ATK0110
+	tristate "ASUS ATK0110 ACPI hwmon"
+	depends on X86 && ACPI && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the ACPI hardware
+	  monitoring interface found in many ASUS motherboards. This
+	  driver will provide readings of fans, voltages and temperatures
+	  through the system firmware.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called asus_atk0110.
+
 config SENSORS_ATXP1
 	tristate "Attansic ATXP1 VID controller"
 	depends on I2C && EXPERIMENTAL
Index: b/drivers/hwmon/Makefile
===================================================================
--- a/drivers/hwmon/Makefile	2009-01-17 18:38:22.662881222 +0100
+++ b/drivers/hwmon/Makefile	2009-01-17 19:05:14.926881425 +0100
@@ -32,6 +32,7 @@
 
 obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
 obj-$(CONFIG_SENSORS_AMS)	+= ams/
+obj-$(CONFIG_SENSORS_ATK0110)	+= asus_atk0110.o
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
Index: b/drivers/acpi/acpica/nsxfeval.c
===================================================================
--- a/drivers/acpi/acpica/nsxfeval.c	2009-01-17 19:08:20.034881950 +0100
+++ b/drivers/acpi/acpica/nsxfeval.c	2009-01-17 19:08:23.694882707 +0100
@@ -53,7 +53,6 @@
 /* Local prototypes */
 static void acpi_ns_resolve_references(struct acpi_evaluate_info *info);
 
-#ifdef ACPI_FUTURE_USAGE
 /*******************************************************************************
  *
  * FUNCTION:    acpi_evaluate_object_typed
@@ -147,7 +146,7 @@
 }
 
 ACPI_EXPORT_SYMBOL(acpi_evaluate_object_typed)
-#endif				/*  ACPI_FUTURE_USAGE  */
+
 /*******************************************************************************
  *
  * FUNCTION:    acpi_evaluate_object

Luca
-- 
Ligabue canta: "Tutti vogliono viaggiare in primaaaa..."
Io ci ho provato e dopo un chilometro ho fuso il motore e bruciato
la frizione...

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Len Brown]

On Thu, 29 Jan 2009, Luca Tettamanti wrote:

> Aside from ASUS the problem is generic since there are two drivers
> poking at the same hardware; for example there are reports of native
> drivers interfering with normal TZ polling (see [1]).

FYI,

While it is slightly off-topic of the (I agree, real)
technical issue here, note that polling is not "normal" on ACPI systems.
[1] was on  SuSE Linux 10.0, which on their own decided to
over-ride the kernel and enable thermal zone polling by default.

This turned out to be a bad idea because it exposed BIOS bugs
in ACPI thermal zones that were never exposed on other operating
systems - evidently because the other operating systems never 
poll thermal zones, they are entirely event driven.

cheers,
Len Brown, Intel Open Source Technology Center

> Luca
> [1] http://www.lm-sensors.org/ticket/2072 and:
> http://thread.gmane.org/gmane.linux.drivers.sensors/12359

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Matthew Garrett]

On Wed, Feb 04, 2009 at 12:52:06AM -0500, Len Brown wrote:

> While it is slightly off-topic of the (I agree, real)
> technical issue here, note that polling is not "normal" on ACPI systems.
> [1] was on  SuSE Linux 10.0, which on their own decided to
> over-ride the kernel and enable thermal zone polling by default.

Checking the DSDTs I have to hand, it seems that polling is expected on 
about 5% of systems via an explicit _TZP and on almost all machines via 
_TSP. Even on systems where thermal notifications are provided, it's 
still up to the OS to poll the zone to find the current temperature and 
take appropriate action. There's still a window for native smbus drivers 
to screw everything up.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Hans de Goede]

Matthew Garrett wrote:
> On Wed, Feb 04, 2009 at 12:52:06AM -0500, Len Brown wrote:
> 
>> While it is slightly off-topic of the (I agree, real)
>> technical issue here, note that polling is not "normal" on ACPI systems.
>> [1] was on  SuSE Linux 10.0, which on their own decided to
>> over-ride the kernel and enable thermal zone polling by default.
> 
> Checking the DSDTs I have to hand, it seems that polling is expected on 
> about 5% of systems via an explicit _TZP and on almost all machines via 
> _TSP. Even on systems where thermal notifications are provided, it's 
> still up to the OS to poll the zone to find the current temperature and 
> take appropriate action. There's still a window for native smbus drivers 
> to screw everything up.
> 

Note that this not only applies to smbus devices but also to superio devices, 
in general these superio hwmon devices use 2 isa ports an index and a data one, 
image they mayhem which could happen if for example:
native driver sets index
acpi driver sets index
acpi driver reads data
native driver writes data (to a completely wrong register)

We *really* need to be fixing this.

Len, Matthew, what is you opinion of the proposed auto setting for 
acpi_enforce_resources, which is meant to mean strict on known problematic 
systems and lax on others?

Not I'm not asking what you think of the code (yet) just what you think of the 
principle. If we can atleast all agree on this as a compromise not breaking 
hwmon on quite a few systems (the strict setting) while stile providing 
something safer then the current lax, then I'm sure we can hash out any code 
problems soon enough.

Regards,

Hans

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Matthew Garrett]

On Wed, Feb 04, 2009 at 09:37:51AM +0100, Hans de Goede wrote:

> Len, Matthew, what is you opinion of the proposed auto setting for 
> acpi_enforce_resources, which is meant to mean strict on known problematic 
> systems and lax on others?

Personally, I'd rather that it was "strict" on everything. We might 
break some existing setups, but they're already working mostly by luck.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

Hi Matthew,

On Wed, 4 Feb 2009 13:17:09 +0000, Matthew Garrett wrote:
> On Wed, Feb 04, 2009 at 09:37:51AM +0100, Hans de Goede wrote:
> 
> > Len, Matthew, what is you opinion of the proposed auto setting for 
> > acpi_enforce_resources, which is meant to mean strict on known problematic 
> > systems and lax on others?
> 
> Personally, I'd rather that it was "strict" on everything. We might 
> break some existing setups, but they're already working mostly by luck.

Are you the new hwmon and i2c subsystems maintainer and I wasn't aware
of it?

-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Matthew Garrett]

On Wed, Feb 04, 2009 at 02:26:06PM +0100, Jean Delvare wrote:
> On Wed, 4 Feb 2009 13:17:09 +0000, Matthew Garrett wrote:
> > Personally, I'd rather that it was "strict" on everything. We might 
> > break some existing setups, but they're already working mostly by luck.
> 
> Are you the new hwmon and i2c subsystems maintainer and I wasn't aware
> of it?

If you've got some programmatic way to tell the difference between safe 
and dangerous reuse of ACPI resources then that would obviously be 
preferable, but I doubt that's practical. auto is a compromise that 
avoids one specific case of breakage, but it does nothing to protect us 
on the majority of systems. Allowing the firmware and the OS to attempt 
to access the same hardware without any locking is an invitation for 
disaster, and in the absence of any way to prevent the firmware from 
doing it...

But. Hans asked for my opinion - the maintainer's is obviously more 
relevant.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

Hi Matthew,

Sorry for the late answer.

On Wed, 4 Feb 2009 14:20:15 +0000, Matthew Garrett wrote:
> On Wed, Feb 04, 2009 at 02:26:06PM +0100, Jean Delvare wrote:
> > On Wed, 4 Feb 2009 13:17:09 +0000, Matthew Garrett wrote:
> > > Personally, I'd rather that it was "strict" on everything. We might 
> > > break some existing setups, but they're already working mostly by luck.
> > 
> > Are you the new hwmon and i2c subsystems maintainer and I wasn't aware
> > of it?
> 
> If you've got some programmatic way to tell the difference between safe 
> and dangerous reuse of ACPI resources then that would obviously be 
> preferable, but I doubt that's practical.

I wish we had such a way, but I don't know of any. If someone can think
of one, that would be someone who knows ACPI much better than I do.

> auto is a compromise that 
> avoids one specific case of breakage, but it does nothing to protect us 
> on the majority of systems. Allowing the firmware and the OS to attempt 
> to access the same hardware without any locking is an invitation for 
> disaster, and in the absence of any way to prevent the firmware from 
> doing it...

In theory you are, of course, perfectly right. The question is, how do
we get there without making people angry because of the regression? I
had yet another example a few days ago:

http://lists.lm-sensors.org/pipermail/lm-sensors/2009-February/025297.html

That system implements an ACPI thermal zone, which apparently reads its
values from the second temperature channel of the IT8716F Super-I/O
chip. This channels happens to be broken (not sure why but that's not
really the point) and returns a wrong temperature value.

The same chip can be driven by our native it87 driver, which, on this
specific board, provides support for 9 voltages, 3 fans, and 1 working
temperature. Do we really have to tell the user to not use the it87
driver and instead use the ACPI thermal driver "because that's what the
firmware wants"?

In this case, I really would like to be able to blacklist the ACPI
thermal device, and let the it87 driver handle the device. If we can
have such a blacklist in the kernel, then I am fine switching the
resource conflict check to "strict" by default. But is is possible? I
would like the ACPI people to tell me. Basically the logic would be as
follows:

if (not laptop) {
	if (ACPI thermal zone defined
	 && no custom ACPI code dealing with thermal conditions) {
		disable ACPI thermal zone and free its resources for native driver
	}
}

But I guess there is no way to know what exactly the ACPI thermal zone
is doing, except by looking at the DSDT, so this can't be automated?

Is it at least possible to disable the ACPI thermal zone either as a
command-line parameter or an internal blacklist?

> But. Hans asked for my opinion - the maintainer's is obviously more 
> relevant.

Well, officially I am not the maintainer of the hwmon subsystem, only
whose of the i2c subsystem. But the fact is that the resource conflict
checking policy affects both the same way. That being said, I'm not
sure how worth my opinion (about this specific matter) is these days...

What I want you (and everyone) to realize is that just switching the
default checking level to strict tomorrow isn't going to work. If we
want the default to be strict then we will have to add a number of
mechanisms to let the user override this in a way which is as easy and
transparent as possible for the user.

One approach that may work is to change the default based on the ACPI
implementation year (I think the info is available, right?) We could
default to strict for systems with year >= 2009. This may still prevent
users from getting the best out of their system, but at least won't
cause a regression for users of older systems where the native driver
has been used so far. I know it's not an ideal solution, but ACPI
implementations aren't ideal either.

-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Matthew Garrett]

On Tue, Feb 10, 2009 at 02:57:16PM +0100, Jean Delvare wrote:

> In theory you are, of course, perfectly right. The question is, how do
> we get there without making people angry because of the regression? 

The only thing we can do is add a printk that informs users that passing 
a boot argument will allow them to use the drivers as they used to.

> The same chip can be driven by our native it87 driver, which, on this
> specific board, provides support for 9 voltages, 3 fans, and 1 working
> temperature. Do we really have to tell the user to not use the it87
> driver and instead use the ACPI thermal driver "because that's what the
> firmware wants"?

It's valid (if dumb) for vendors to design their platforms such that 
enabling ACPI and then not using the thermal code may result in hardware 
damage. We have no way of determining that in advance, so all we can do 
is tell the user that they can pass an argument if they know it's safe 
to do so.

> But I guess there is no way to know what exactly the ACPI thermal zone
> is doing, except by looking at the DSDT, so this can't be automated?

Correct.

> Is it at least possible to disable the ACPI thermal zone either as a
> command-line parameter or an internal blacklist?

It's possible, and we could certainly add an argument to do so. However, 
removing support for the kernel use of the thermal zone doesn't prevent 
the firmware from making calls to the thermal code itself. There's no 
real way we can block that.

> One approach that may work is to change the default based on the ACPI
> implementation year (I think the info is available, right?) We could
> default to strict for systems with year >= 2009. This may still prevent
> users from getting the best out of their system, but at least won't
> cause a regression for users of older systems where the native driver
> has been used so far. I know it's not an ideal solution, but ACPI
> implementations aren't ideal either.

The problem with this approach is that we still end up with a large 
number of malfunctioning machines. Really, I don't think there's any way 
to handle this other than defaulting to strict, letting the default be 
changed at run and boot time and printing a message when a driver is 
refused permission to bind. Distributions that want to obtain the 
previous behaviour can change the default back.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Hans de Goede]

Matthew Garrett wrote:
> On Tue, Feb 10, 2009 at 02:57:16PM +0100, Jean Delvare wrote:
> 
  year (I think the info is available, right?) We could
>> default to strict for systems with year >= 2009. This may still prevent
>> users from getting the best out of their system, but at least won't
>> cause a regression for users of older systems where the native driver
>> has been used so far. I know it's not an ideal solution, but ACPI
>> implementations aren't ideal either.
> 
> The problem with this approach is that we still end up with a large 
> number of malfunctioning machines. Really, I don't think there's any way 
> to handle this other than defaulting to strict, letting the default be 
> changed at run and boot time and printing a message when a driver is 
> refused permission to bind. Distributions that want to obtain the 
> previous behaviour can change the default back.
> 

For the record we have changed the default to strict in Fedora's 
development branch, for 2 weeks or so now, including in the recently 
released Fedora 11 release and we've had 0 complaints so far.

Regards,

Hans

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

On Tue, 10 Feb 2009 16:32:24 +0100, Hans de Goede wrote:
> Matthew Garrett wrote:
> > On Tue, Feb 10, 2009 at 02:57:16PM +0100, Jean Delvare wrote:
> > 
>   year (I think the info is available, right?) We could
> >> default to strict for systems with year >= 2009. This may still prevent
> >> users from getting the best out of their system, but at least won't
> >> cause a regression for users of older systems where the native driver
> >> has been used so far. I know it's not an ideal solution, but ACPI
> >> implementations aren't ideal either.
> > 
> > The problem with this approach is that we still end up with a large 
> > number of malfunctioning machines. Really, I don't think there's any way 
> > to handle this other than defaulting to strict, letting the default be 
> > changed at run and boot time and printing a message when a driver is 
> > refused permission to bind. Distributions that want to obtain the 
> > previous behaviour can change the default back.

If we expect different distributions / user classes to set a different
default, then it might make sense to make acpi_enforce_resources's
default value a config option?

> For the record we have changed the default to strict in Fedora's 
> development branch, for 2 weeks or so now, including in the recently 
> released Fedora 11 release and we've had 0 complaints so far.

Well, if the number of affected systems is small, this is good news.
But this is only 2 weeks and one distribution, coverage isn't
sufficient to claim anything yet IMHO.

That being said... if there's a common consensus that switching to
strict and dealing with fallouts is the best thing to do, and I'm the
only one objecting to this, then I am ready to admit that I was wrong
and let you proceed.

-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Pavel Machek]

Hi!

> > For the record we have changed the default to strict in Fedora's 
> > development branch, for 2 weeks or so now, including in the recently 
> > released Fedora 11 release and we've had 0 complaints so far.
> 
> Well, if the number of affected systems is small, this is good news.
> But this is only 2 weeks and one distribution, coverage isn't
> sufficient to claim anything yet IMHO.
> 
> That being said... if there's a common consensus that switching to
> strict and dealing with fallouts is the best thing to do, and I'm the
> only one objecting to this, then I am ready to admit that I was wrong
> and let you proceed.

I believe that 'enable strict, deal with fallout' is the best
long-term strategy...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

On Fri, Feb 27, 2009 at 2:27 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > For the record we have changed the default to strict in Fedora's
>> > development branch, for 2 weeks or so now, including in the recently
>> > released Fedora 11 release and we've had 0 complaints so far.
>>
>> Well, if the number of affected systems is small, this is good news.
>> But this is only 2 weeks and one distribution, coverage isn't
>> sufficient to claim anything yet IMHO.
>>
>> That being said... if there's a common consensus that switching to
>> strict and dealing with fallouts is the best thing to do, and I'm the
>> only one objecting to this, then I am ready to admit that I was wrong
>> and let you proceed.
>
> I believe that 'enable strict, deal with fallout' is the best
> long-term strategy...

Hello,
the merge window for .30 is now open, what are we going to do with this issue?

Luca

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Hans de Goede]

On 03/24/2009 01:39 PM, Luca Tettamanti wrote:
> On Fri, Feb 27, 2009 at 2:27 PM, Pavel Machek<pavel@ucw.cz>  wrote:
>> Hi!
>>
>>>> For the record we have changed the default to strict in Fedora's
>>>> development branch, for 2 weeks or so now, including in the recently
>>>> released Fedora 11 release and we've had 0 complaints so far.
>>> Well, if the number of affected systems is small, this is good news.
>>> But this is only 2 weeks and one distribution, coverage isn't
>>> sufficient to claim anything yet IMHO.
>>>
>>> That being said... if there's a common consensus that switching to
>>> strict and dealing with fallouts is the best thing to do, and I'm the
>>> only one objecting to this, then I am ready to admit that I was wrong
>>> and let you proceed.
>> I believe that 'enable strict, deal with fallout' is the best
>> long-term strategy...
>
> Hello,
> the merge window for .30 is now open, what are we going to do with this issue?
>

I think the consensus was to make the default strict and to merge the atk0110
driver, right?

Note that we've been running this setup in Fedora kernels for quite a while now,
and have had only one bug report, which was solved by simply explaining why this
was done.

Regards,

Hans

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

Hi Hans, Luca,

On Tue, 24 Mar 2009 14:21:21 +0100, Hans de Goede wrote:
> On 03/24/2009 01:39 PM, Luca Tettamanti wrote:
> > the merge window for .30 is now open, what are we going to do with this issue?
> 
> I think the consensus was to make the default strict and to merge the atk0110
> driver, right?

Yes.

> Note that we've been running this setup in Fedora kernels for quite a while now,
> and have had only one bug report, which was solved by simply explaining why this
> was done.

Maybe the explanation in question could be added somewhere, either in
the help text of CONFIG_HWMON, or somewhere under Documentation/, or on
the lm-sensors.org wiki? I expect the question to be asked more
frequently once the default changes upstream, and I would like avoiding
wasting developer time replying again and again. Having the possibility
to point users to a clear explanation would be very pleasant.

-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Hans de Goede]

On 03/24/2009 02:43 PM, Jean Delvare wrote:
> Hi Hans, Luca,
>
> On Tue, 24 Mar 2009 14:21:21 +0100, Hans de Goede wrote:
>> On 03/24/2009 01:39 PM, Luca Tettamanti wrote:
>>> the merge window for .30 is now open, what are we going to do with this issue?
>> I think the consensus was to make the default strict and to merge the atk0110
>> driver, right?
>
> Yes.
>
>> Note that we've been running this setup in Fedora kernels for quite a while now,
>> and have had only one bug report, which was solved by simply explaining why this
>> was done.
>
> Maybe the explanation in question could be added somewhere, either in
> the help text of CONFIG_HWMON, or somewhere under Documentation/, or on
> the lm-sensors.org wiki?

I think having an explanation somewhere would be great, not sure if the explanation
in question was all that great though (and it is burried in between a gazillion other
kernel bugs, so a bit hard to find).

Regards,

Hans

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

Il Tue, Mar 24, 2009 at 02:21:21PM +0100, Hans de Goede ha scritto: 
> On 03/24/2009 01:39 PM, Luca Tettamanti wrote:
>> On Fri, Feb 27, 2009 at 2:27 PM, Pavel Machek<pavel@ucw.cz>  wrote:
>>> Hi!
>>>
>>>>> For the record we have changed the default to strict in Fedora's
>>>>> development branch, for 2 weeks or so now, including in the recently
>>>>> released Fedora 11 release and we've had 0 complaints so far.
>>>> Well, if the number of affected systems is small, this is good news.
>>>> But this is only 2 weeks and one distribution, coverage isn't
>>>> sufficient to claim anything yet IMHO.
>>>>
>>>> That being said... if there's a common consensus that switching to
>>>> strict and dealing with fallouts is the best thing to do, and I'm the
>>>> only one objecting to this, then I am ready to admit that I was wrong
>>>> and let you proceed.
>>> I believe that 'enable strict, deal with fallout' is the best
>>> long-term strategy...
>>
>> Hello,
>> the merge window for .30 is now open, what are we going to do with this issue?
>>
>
> I think the consensus was to make the default strict and to merge the atk0110
> driver, right?

Ok,
here's a patch:
---
The following patch changes the default value for option "acpi_enforce_resource"
to strict. It enforces strict resource checking - disallowing access by native
drivers to IO ports and memory regions claimed by ACPI firmware.

The patch is mainly aimed to block native hwmon drivers from touching
monitoring chips that ACPI thinks it own.

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
---
 Documentation/kernel-parameters.txt |   16 ++++++++++++++++
 drivers/acpi/osl.c                  |    6 +++---
 2 files changed, 19 insertions(+), 3 deletions(-)

Index: b/Documentation/kernel-parameters.txt
===================================================================
--- a/Documentation/kernel-parameters.txt	2009-03-29 15:47:28.000000000 +0200
+++ b/Documentation/kernel-parameters.txt	2009-03-29 15:51:30.000000000 +0200
@@ -259,6 +259,22 @@
 			to assume that this machine's pmtimer latches its value
 			and always returns good values.
 
+	acpi_enforce_resources=	[ACPI]
+			{ strict,  lax, no }
+			Check for resource conflicts between native drivers
+			and ACPI OperationRegions (SystemIO and SystemMemory
+			only). IO ports and memory declared in ACPI might be
+			used by the ACPI subsystem in arbitrary AML code and
+			can interfere with legacy drivers.
+			strict (default): access to resources claimed by ACPI
+			is denied; legacy drivers trying to access reserved
+			resources will fail to load.
+			lax: access to resources claimed by ACPI is allowed;
+			legacy drivers trying to access reserved resources
+			will load and a warning message is logged.
+			no: ACPI OperationRegions are not marked as reserved,
+			no further checks are performed.
+
 	agp=		[AGP]
 			{ off | try_unsupported }
 			off: disable AGP support
Index: b/drivers/acpi/osl.c
===================================================================
--- a/drivers/acpi/osl.c	2009-03-29 15:47:29.000000000 +0200
+++ b/drivers/acpi/osl.c	2009-03-29 15:51:30.000000000 +0200
@@ -1070,9 +1070,9 @@
  * in arbitrary AML code and can interfere with legacy drivers.
  * acpi_enforce_resources= can be set to:
  *
- *   - strict           (2)
+ *   - strict (default) (2)
  *     -> further driver trying to access the resources will not load
- *   - lax (default)    (1)
+ *   - lax              (1)
  *     -> further driver trying to access the resources will load, but you
  *     get a system message that something might go wrong...
  *
@@ -1084,7 +1084,7 @@
 #define ENFORCE_RESOURCES_LAX    1
 #define ENFORCE_RESOURCES_NO     0
 
-static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
+static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
 
 static int __init acpi_enforce_resources_setup(char *str)
 {


Luca
-- 
I went to God just to see
And I was looking at me
Saw heaven and hell were lies
When I'm God everyone dies

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Pavel Machek]

On Sun 2009-03-29 22:16:17, Luca Tettamanti wrote:
> Il Tue, Mar 24, 2009 at 02:21:21PM +0100, Hans de Goede ha scritto: 
> > On 03/24/2009 01:39 PM, Luca Tettamanti wrote:
> >> On Fri, Feb 27, 2009 at 2:27 PM, Pavel Machek<pavel@ucw.cz>  wrote:
> >>> Hi!
> >>>
> >>>>> For the record we have changed the default to strict in Fedora's
> >>>>> development branch, for 2 weeks or so now, including in the recently
> >>>>> released Fedora 11 release and we've had 0 complaints so far.
> >>>> Well, if the number of affected systems is small, this is good news.
> >>>> But this is only 2 weeks and one distribution, coverage isn't
> >>>> sufficient to claim anything yet IMHO.
> >>>>
> >>>> That being said... if there's a common consensus that switching to
> >>>> strict and dealing with fallouts is the best thing to do, and I'm the
> >>>> only one objecting to this, then I am ready to admit that I was wrong
> >>>> and let you proceed.
> >>> I believe that 'enable strict, deal with fallout' is the best
> >>> long-term strategy...
> >>
> >> Hello,
> >> the merge window for .30 is now open, what are we going to do with this issue?
> >>
> >
> > I think the consensus was to make the default strict and to merge the atk0110
> > driver, right?
> 
> Ok,
> here's a patch:
> ---
> The following patch changes the default value for option "acpi_enforce_resource"
> to strict. It enforces strict resource checking - disallowing access by native
> drivers to IO ports and memory regions claimed by ACPI firmware.
> 
> The patch is mainly aimed to block native hwmon drivers from touching
> monitoring chips that ACPI thinks it own.
> 
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>

Acked-by: Pavel Machek <pavel@suse.cz>

> Index: b/Documentation/kernel-parameters.txt
> ===================================================================
> --- a/Documentation/kernel-parameters.txt	2009-03-29 15:47:28.000000000 +0200
> +++ b/Documentation/kernel-parameters.txt	2009-03-29 15:51:30.000000000 +0200
> @@ -259,6 +259,22 @@
>  			to assume that this machine's pmtimer latches its value
>  			and always returns good values.
>  
> +	acpi_enforce_resources=	[ACPI]
> +			{ strict,  lax, no }
			          ~ extra space here... probably not
worth fixing.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

Hi Luca,

On Sun, 29 Mar 2009 22:16:17 +0200, Luca Tettamanti wrote:
> Il Tue, Mar 24, 2009 at 02:21:21PM +0100, Hans de Goede ha scritto: 
> > On 03/24/2009 01:39 PM, Luca Tettamanti wrote:
> >> On Fri, Feb 27, 2009 at 2:27 PM, Pavel Machek<pavel@ucw.cz>  wrote:
> >>> Hi!
> >>>
> >>>>> For the record we have changed the default to strict in Fedora's
> >>>>> development branch, for 2 weeks or so now, including in the recently
> >>>>> released Fedora 11 release and we've had 0 complaints so far.
> >>>> Well, if the number of affected systems is small, this is good news.
> >>>> But this is only 2 weeks and one distribution, coverage isn't
> >>>> sufficient to claim anything yet IMHO.
> >>>>
> >>>> That being said... if there's a common consensus that switching to
> >>>> strict and dealing with fallouts is the best thing to do, and I'm the
> >>>> only one objecting to this, then I am ready to admit that I was wrong
> >>>> and let you proceed.
> >>> I believe that 'enable strict, deal with fallout' is the best
> >>> long-term strategy...
> >>
> >> Hello,
> >> the merge window for .30 is now open, what are we going to do with this issue?
> >>
> >
> > I think the consensus was to make the default strict and to merge the atk0110
> > driver, right?
> 
> Ok,
> here's a patch:
> ---
> The following patch changes the default value for option "acpi_enforce_resource"
> to strict. It enforces strict resource checking - disallowing access by native
> drivers to IO ports and memory regions claimed by ACPI firmware.
> 
> The patch is mainly aimed to block native hwmon drivers from touching
> monitoring chips that ACPI thinks it own.
> 
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> ---
>  Documentation/kernel-parameters.txt |   16 ++++++++++++++++
>  drivers/acpi/osl.c                  |    6 +++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> Index: b/Documentation/kernel-parameters.txt
> ===================================================================
> --- a/Documentation/kernel-parameters.txt	2009-03-29 15:47:28.000000000 +0200
> +++ b/Documentation/kernel-parameters.txt	2009-03-29 15:51:30.000000000 +0200
> @@ -259,6 +259,22 @@
>  			to assume that this machine's pmtimer latches its value
>  			and always returns good values.
>  
> +	acpi_enforce_resources=	[ACPI]
> +			{ strict,  lax, no }

Other options use | as a delimited between allowed values.

> +			Check for resource conflicts between native drivers
> +			and ACPI OperationRegions (SystemIO and SystemMemory
> +			only). IO ports and memory declared in ACPI might be
> +			used by the ACPI subsystem in arbitrary AML code and
> +			can interfere with legacy drivers.
> +			strict (default): access to resources claimed by ACPI
> +			is denied; legacy drivers trying to access reserved
> +			resources will fail to load.

The driver will fail to _bind_ to the device, this doesn't mean that the
module will fail to load. In practice, PCI modules will still load
while "fake" platform modules will indeed fail to load. Many hwmon
drivers fall into the second category.

> +			lax: access to resources claimed by ACPI is allowed;
> +			legacy drivers trying to access reserved resources
> +			will load and a warning message is logged.

Ditto.

> +			no: ACPI OperationRegions are not marked as reserved,
> +			no further checks are performed.
> +
>  	agp=		[AGP]
>  			{ off | try_unsupported }
>  			off: disable AGP support
> Index: b/drivers/acpi/osl.c
> ===================================================================
> --- a/drivers/acpi/osl.c	2009-03-29 15:47:29.000000000 +0200
> +++ b/drivers/acpi/osl.c	2009-03-29 15:51:30.000000000 +0200
> @@ -1070,9 +1070,9 @@
>   * in arbitrary AML code and can interfere with legacy drivers.
>   * acpi_enforce_resources= can be set to:
>   *
> - *   - strict           (2)
> + *   - strict (default) (2)
>   *     -> further driver trying to access the resources will not load
> - *   - lax (default)    (1)
> + *   - lax              (1)
>   *     -> further driver trying to access the resources will load, but you
>   *     get a system message that something might go wrong...
>   *
> @@ -1084,7 +1084,7 @@
>  #define ENFORCE_RESOURCES_LAX    1
>  #define ENFORCE_RESOURCES_NO     0
>  
> -static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
>  
>  static int __init acpi_enforce_resources_setup(char *str)
>  {

Other that these minor details:

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti]

Il Sun, Mar 29, 2009 at 10:55:01PM +0200, Jean Delvare ha scritto: 
> Hi Luca,
> 
> On Sun, 29 Mar 2009 22:16:17 +0200, Luca Tettamanti wrote:
> > Il Tue, Mar 24, 2009 at 02:21:21PM +0100, Hans de Goede ha scritto: 
> > > On 03/24/2009 01:39 PM, Luca Tettamanti wrote:
> > >> On Fri, Feb 27, 2009 at 2:27 PM, Pavel Machek<pavel@ucw.cz>  wrote:
> > >>> Hi!
> > >>>
> > >>>>> For the record we have changed the default to strict in Fedora's
> > >>>>> development branch, for 2 weeks or so now, including in the recently
> > >>>>> released Fedora 11 release and we've had 0 complaints so far.
> > >>>> Well, if the number of affected systems is small, this is good news.
> > >>>> But this is only 2 weeks and one distribution, coverage isn't
> > >>>> sufficient to claim anything yet IMHO.
> > >>>>
> > >>>> That being said... if there's a common consensus that switching to
> > >>>> strict and dealing with fallouts is the best thing to do, and I'm the
> > >>>> only one objecting to this, then I am ready to admit that I was wrong
> > >>>> and let you proceed.
> > >>> I believe that 'enable strict, deal with fallout' is the best
> > >>> long-term strategy...
> > >>
> > >> Hello,
> > >> the merge window for .30 is now open, what are we going to do with this issue?
> > >>
> > >
> > > I think the consensus was to make the default strict and to merge the atk0110
> > > driver, right?
> > 
> > Ok,
> > here's a patch:
> > ---
> > The following patch changes the default value for option "acpi_enforce_resource"
> > to strict. It enforces strict resource checking - disallowing access by native
> > drivers to IO ports and memory regions claimed by ACPI firmware.
> > 
> > The patch is mainly aimed to block native hwmon drivers from touching
> > monitoring chips that ACPI thinks it own.
> > 
> > Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> > ---
> >  Documentation/kernel-parameters.txt |   16 ++++++++++++++++
> >  drivers/acpi/osl.c                  |    6 +++---
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > Index: b/Documentation/kernel-parameters.txt
> > ===================================================================
> > --- a/Documentation/kernel-parameters.txt	2009-03-29 15:47:28.000000000 +0200
> > +++ b/Documentation/kernel-parameters.txt	2009-03-29 15:51:30.000000000 +0200
> > @@ -259,6 +259,22 @@
> >  			to assume that this machine's pmtimer latches its value
> >  			and always returns good values.
> >  
> > +	acpi_enforce_resources=	[ACPI]
> > +			{ strict,  lax, no }
> 
> Other options use | as a delimited between allowed values.
[...]
> The driver will fail to _bind_ to the device, this doesn't mean that the
> module will fail to load. In practice, PCI modules will still load
> while "fake" platform modules will indeed fail to load. Many hwmon
> drivers fall into the second category.
[...]
> Other that these minor details:
> 
> Acked-by: Jean Delvare <khali@linux-fr.org>

Hi Jean,
here's a revised patch:
---

The following patch changes the default value for option "acpi_enforce_resource"
to strict. It enforces strict resource checking - disallowing access by native
drivers to IO ports and memory regions claimed by ACPI firmware.

The patch is mainly aimed to block native hwmon drivers from touching
monitoring chips that ACPI thinks it own.

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
---
 Documentation/kernel-parameters.txt |   16 ++++++++++++++++
 drivers/acpi/osl.c                  |    6 +++---
 2 files changed, 19 insertions(+), 3 deletions(-)

Index: b/Documentation/kernel-parameters.txt
===================================================================
--- a/Documentation/kernel-parameters.txt	2009-03-29 23:58:11.574893074 +0200
+++ b/Documentation/kernel-parameters.txt	2009-03-29 23:58:31.582894852 +0200
@@ -259,6 +259,22 @@
 			to assume that this machine's pmtimer latches its value
 			and always returns good values.
 
+	acpi_enforce_resources=	[ACPI]
+			{ strict | lax | no }
+			Check for resource conflicts between native drivers
+			and ACPI OperationRegions (SystemIO and SystemMemory
+			only). IO ports and memory declared in ACPI might be
+			used by the ACPI subsystem in arbitrary AML code and
+			can interfere with legacy drivers.
+			strict (default): access to resources claimed by ACPI
+			is denied; legacy drivers trying to access reserved
+			resources will fail to bind to device using them.
+			lax: access to resources claimed by ACPI is allowed;
+			legacy drivers trying to access reserved resources
+			will bind successfully but a warning message is logged.
+			no: ACPI OperationRegions are not marked as reserved,
+			no further checks are performed.
+
 	agp=		[AGP]
 			{ off | try_unsupported }
 			off: disable AGP support
Index: b/drivers/acpi/osl.c
===================================================================
--- a/drivers/acpi/osl.c	2009-03-29 23:58:11.942913535 +0200
+++ b/drivers/acpi/osl.c	2009-03-29 23:58:31.586892815 +0200
@@ -1070,9 +1070,9 @@
  * in arbitrary AML code and can interfere with legacy drivers.
  * acpi_enforce_resources= can be set to:
  *
- *   - strict           (2)
+ *   - strict (default) (2)
  *     -> further driver trying to access the resources will not load
- *   - lax (default)    (1)
+ *   - lax              (1)
  *     -> further driver trying to access the resources will load, but you
  *     get a system message that something might go wrong...
  *
@@ -1084,7 +1084,7 @@
 #define ENFORCE_RESOURCES_LAX    1
 #define ENFORCE_RESOURCES_NO     0
 
-static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
+static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
 
 static int __init acpi_enforce_resources_setup(char *str)
 {


Luca
-- 
La somma dell'intelligenza sulla terra e` una costante.
La popolazione e` in aumento.

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

On Mon, 30 Mar 2009 00:01:27 +0200, Luca Tettamanti wrote:
> here's a revised patch:
> ---
> 
> The following patch changes the default value for option "acpi_enforce_resource"
> to strict. It enforces strict resource checking - disallowing access by native
> drivers to IO ports and memory regions claimed by ACPI firmware.
> 
> The patch is mainly aimed to block native hwmon drivers from touching
> monitoring chips that ACPI thinks it own.
> 
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>

Acked-by: Jean Delvare <jdelvare@suse.de>

> ---
>  Documentation/kernel-parameters.txt |   16 ++++++++++++++++
>  drivers/acpi/osl.c                  |    6 +++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> Index: b/Documentation/kernel-parameters.txt
> ===================================================================
> --- a/Documentation/kernel-parameters.txt	2009-03-29 23:58:11.574893074 +0200
> +++ b/Documentation/kernel-parameters.txt	2009-03-29 23:58:31.582894852 +0200
> @@ -259,6 +259,22 @@
>  			to assume that this machine's pmtimer latches its value
>  			and always returns good values.
>  
> +	acpi_enforce_resources=	[ACPI]
> +			{ strict | lax | no }
> +			Check for resource conflicts between native drivers
> +			and ACPI OperationRegions (SystemIO and SystemMemory
> +			only). IO ports and memory declared in ACPI might be
> +			used by the ACPI subsystem in arbitrary AML code and
> +			can interfere with legacy drivers.
> +			strict (default): access to resources claimed by ACPI
> +			is denied; legacy drivers trying to access reserved
> +			resources will fail to bind to device using them.
> +			lax: access to resources claimed by ACPI is allowed;
> +			legacy drivers trying to access reserved resources
> +			will bind successfully but a warning message is logged.
> +			no: ACPI OperationRegions are not marked as reserved,
> +			no further checks are performed.
> +
>  	agp=		[AGP]
>  			{ off | try_unsupported }
>  			off: disable AGP support
> Index: b/drivers/acpi/osl.c
> ===================================================================
> --- a/drivers/acpi/osl.c	2009-03-29 23:58:11.942913535 +0200
> +++ b/drivers/acpi/osl.c	2009-03-29 23:58:31.586892815 +0200
> @@ -1070,9 +1070,9 @@
>   * in arbitrary AML code and can interfere with legacy drivers.
>   * acpi_enforce_resources= can be set to:
>   *
> - *   - strict           (2)
> + *   - strict (default) (2)
>   *     -> further driver trying to access the resources will not load
> - *   - lax (default)    (1)
> + *   - lax              (1)
>   *     -> further driver trying to access the resources will load, but you
>   *     get a system message that something might go wrong...
>   *
> @@ -1084,7 +1084,7 @@
>  #define ENFORCE_RESOURCES_LAX    1
>  #define ENFORCE_RESOURCES_NO     0
>  
> -static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
> +static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
>  
>  static int __init acpi_enforce_resources_setup(char *str)
>  {
> 
> 
> Luca


-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Len Brown]

>From 7e90560c50f754d65884e251e94c1efa2a4b5784 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti <kronos.it@gmail.com>
Date: Mon, 30 Mar 2009 00:01:27 +0200
Subject: [PATCH] ACPI: acpi_enforce_resource=strict by default
X-Patchwork-Hint: ignore

Enforce strict resource checking - disallowing access by native
drivers to IO ports and memory regions claimed by ACPI firmware.

The patch is mainly aimed to block native hwmon drivers from touching
monitoring chips that ACPI thinks it own.

If this causes a regression, boot with "acpi_enforce_resources=lax"
which was the previous default.

http://bugzilla.kernel.org/show_bug.cgi?id=12376
http://bugzilla.kernel.org/show_bug.cgi?id=12541

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
Acked-by: Pavel Machek <pavel@suse.cz>
Acked-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Len Brown <len.brown@intel.com>
---

as applied.

thanks,
-Len


 Documentation/kernel-parameters.txt |   16 ++++++++++++++++
 drivers/acpi/osl.c                  |    6 +++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 54f21a5..7068d0b 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -258,6 +258,22 @@ and is between 256 and 4096 characters. It is defined in the file
 			to assume that this machine's pmtimer latches its value
 			and always returns good values.
 
+	acpi_enforce_resources=	[ACPI]
+			{ strict | lax | no }
+			Check for resource conflicts between native drivers
+			and ACPI OperationRegions (SystemIO and SystemMemory
+			only). IO ports and memory declared in ACPI might be
+			used by the ACPI subsystem in arbitrary AML code and
+			can interfere with legacy drivers.
+			strict (default): access to resources claimed by ACPI
+			is denied; legacy drivers trying to access reserved
+			resources will fail to bind to device using them.
+			lax: access to resources claimed by ACPI is allowed;
+			legacy drivers trying to access reserved resources
+			will bind successfully but a warning message is logged.
+			no: ACPI OperationRegions are not marked as reserved,
+			no further checks are performed.
+
 	agp=		[AGP]
 			{ off | try_unsupported }
 			off: disable AGP support
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 1e35f34..f50ca1e 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1063,9 +1063,9 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
  * in arbitrary AML code and can interfere with legacy drivers.
  * acpi_enforce_resources= can be set to:
  *
- *   - strict           (2)
+ *   - strict (default) (2)
  *     -> further driver trying to access the resources will not load
- *   - lax (default)    (1)
+ *   - lax              (1)
  *     -> further driver trying to access the resources will load, but you
  *     get a system message that something might go wrong...
  *
@@ -1077,7 +1077,7 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
 #define ENFORCE_RESOURCES_LAX    1
 #define ENFORCE_RESOURCES_NO     0
 
-static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_LAX;
+static unsigned int acpi_enforce_resources = ENFORCE_RESOURCES_STRICT;
 
 static int __init acpi_enforce_resources_setup(char *str)
 {
-- 
1.6.0.6

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

On Thu, 02 Apr 2009 18:59:11 -0400 (EDT), Len Brown wrote:
> From 7e90560c50f754d65884e251e94c1efa2a4b5784 Mon Sep 17 00:00:00 2001
> From: Luca Tettamanti <kronos.it@gmail.com>
> Date: Mon, 30 Mar 2009 00:01:27 +0200
> Subject: [PATCH] ACPI: acpi_enforce_resource=strict by default
> X-Patchwork-Hint: ignore
> 
> Enforce strict resource checking - disallowing access by native
> drivers to IO ports and memory regions claimed by ACPI firmware.
> 
> The patch is mainly aimed to block native hwmon drivers from touching
> monitoring chips that ACPI thinks it own.
> 
> If this causes a regression, boot with "acpi_enforce_resources=lax"
> which was the previous default.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=12376
> http://bugzilla.kernel.org/show_bug.cgi?id=12541
> 
> Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> Acked-by: Pavel Machek <pavel@suse.cz>
> Acked-by: Jean Delvare <khali@linux-fr.org>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> 
> as applied.

Thanks Len! I'll look into pushing Luca's atk0110 driver upstream later
today (but it's a busy day at work...)

-- 
Jean Delvare

Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Jean Delvare]

On Tue, 10 Feb 2009 14:08:29 +0000, Matthew Garrett wrote:
> On Tue, Feb 10, 2009 at 02:57:16PM +0100, Jean Delvare wrote:
> 
> > In theory you are, of course, perfectly right. The question is, how do
> > we get there without making people angry because of the regression? 
> 
> The only thing we can do is add a printk that informs users that passing 
> a boot argument will allow them to use the drivers as they used to.

Good point.

> > The same chip can be driven by our native it87 driver, which, on this
> > specific board, provides support for 9 voltages, 3 fans, and 1 working
> > temperature. Do we really have to tell the user to not use the it87
> > driver and instead use the ACPI thermal driver "because that's what the
> > firmware wants"?
> 
> It's valid (if dumb) for vendors to design their platforms such that 
> enabling ACPI and then not using the thermal code may result in hardware 
> damage. We have no way of determining that in advance, so all we can do 
> is tell the user that they can pass an argument if they know it's safe 
> to do so.

OK, I understand.

> > But I guess there is no way to know what exactly the ACPI thermal zone
> > is doing, except by looking at the DSDT, so this can't be automated?
> 
> Correct.
> 
> > Is it at least possible to disable the ACPI thermal zone either as a
> > command-line parameter or an internal blacklist?
> 
> It's possible, and we could certainly add an argument to do so. However, 
> removing support for the kernel use of the thermal zone doesn't prevent 
> the firmware from making calls to the thermal code itself. There's no 
> real way we can block that.
> 
> > One approach that may work is to change the default based on the ACPI
> > implementation year (I think the info is available, right?) We could
> > default to strict for systems with year >= 2009. This may still prevent
> > users from getting the best out of their system, but at least won't
> > cause a regression for users of older systems where the native driver
> > has been used so far. I know it's not an ideal solution, but ACPI
> > implementations aren't ideal either.
> 
> The problem with this approach is that we still end up with a large 
> number of malfunctioning machines.

Well, that's what we have at the moment and the world didn't end.
Enabling strict checks for a subset of machines is always an
improvement compared to the current situation.

> Really, I don't think there's any way 
> to handle this other than defaulting to strict, letting the default be 
> changed at run and boot time and printing a message when a driver is 
> refused permission to bind. Distributions that want to obtain the 
> previous behaviour can change the default back.

Anyway, as I already wrote elsewhere in this thread, I no longer object
to the change you propose. I won't instigate it, but if it happens,
and care is taken to address the foreseeable downfalls, fine with me.

Thanks,
-- 
Jean Delvare

polling (Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources)

[Len Brown]

On Wed, 4 Feb 2009, Matthew Garrett wrote:

> On Wed, Feb 04, 2009 at 12:52:06AM -0500, Len Brown wrote:
> 
> > While it is slightly off-topic of the (I agree, real)
> > technical issue here, note that polling is not "normal" on ACPI systems.
> > [1] was on  SuSE Linux 10.0, which on their own decided to
> > over-ride the kernel and enable thermal zone polling by default.
> 
> Checking the DSDTs I have to hand, it seems that polling is expected on 
> about 5% of systems via an explicit _TZP and on almost all machines via 
> _TSP. Even on systems where thermal notifications are provided, it's 
> still up to the OS to poll the zone to find the current temperature and 
> take appropriate action. There's still a window for native smbus drivers 
> to screw everything up.

FWIW

In the last 6 years, I've seen exactly 3 systems with a non-zero _TZP.
An old Averatec laptop asked for 1 second, and two recent EEE-PC's ask for 
30 seconds.  Dunno why Asus has made this leap backwards.

_TSP is a different beast.  It only exists in the context of _PSV and 
_PSL.

-Len

ps. I just noticed something in the spec under _PSL...
"If a linear performance control register is not defined(..) for a 
processor defined in _PSL or for a processor device in the zone as 
indicated
by _TZM, then the processor must support processor performance states
(in other words, the processor's processor object must include _PCT, _PSS, 
and _PPC).

Interesting, here is an official tie in of P-states and passive trip 
points that I'd not noticed before....