Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Robert Hancock]

Mark M. Hoffman wrote:
> Linus, Rene, et al:
> 
> * Rene Herman <rene.herman@keyaccess.nl> [2008-06-23 17:06:14 +0200]:
>> drivers/thermal/thermal_sys.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>> index 6098787..c21e03c 100644
>> --- a/drivers/thermal/thermal_sys.c
>> +++ b/drivers/thermal/thermal_sys.c
>> @@ -357,7 +357,7 @@ thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
>>
>> 	INIT_LIST_HEAD(&hwmon->tz_list);
>> 	strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
>> -	hwmon->device = hwmon_device_register(NULL);
>> +	hwmon->device = hwmon_device_register(&tz->device);
>> 	if (IS_ERR(hwmon->device)) {
>> 		result = PTR_ERR(hwmon->device);
>> 		goto free_mem;
> 
> NACK.
> 
> 1) The bug is in libsensors (2.10), not the kernel.

This doesn't matter. Breaking userspace in such a fashion is severely 
frowned upon unless essentially unavoidable, even if it is just 
triggering a bug.

> 
> - but more importantly -
> 
> 2) This patch is broken.

You didn't indicate what was wrong with the patch.

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Linus Torvalds]

On Mon, 23 Jun 2008, Robert Hancock wrote:

> Mark M. Hoffman wrote:
> > 
> > 1) The bug is in libsensors (2.10), not the kernel.
> 
> This doesn't matter. Breaking userspace in such a fashion is severely frowned
> upon unless essentially unavoidable, even if it is just triggering a bug.

I do tend to agree. If it used to work, it should continue to work, even 
if it wasn't _meant_ to work.

That's not always 100% possible, but the fact is, neither is updating user 
space always possible. The kernel needs to try its best to maintain stable 
interfaces, and no, "..but the interface was broken" is _not_ an 
acceptable excuse for unbreaking it if it causes problems.

> > - but more importantly -
> > 
> > 2) This patch is broken.
> 
> You didn't indicate what was wrong with the patch.

Yes. Leaving everybody wondering whether it's just an opinionated 
expression of the former problem, or whether there is a real and 
understandable reason why it was NACK'ed.

		Linus

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Zhang Rui]

On Tue, 2008-06-24 at 11:07 +0800, Linus Torvalds wrote:
> 
> 
> On Mon, 23 Jun 2008, Robert Hancock wrote:
> 
> > Mark M. Hoffman wrote:
> > >
> > > 1) The bug is in libsensors (2.10), not the kernel.
> >
> > This doesn't matter. Breaking userspace in such a fashion is
> severely frowned
> > upon unless essentially unavoidable, even if it is just triggering a
> bug.
> 
> I do tend to agree. If it used to work, it should continue to work,
> even
> if it wasn't _meant_ to work.
> 
> That's not always 100% possible, but the fact is, neither is updating
> user
> space always possible. The kernel needs to try its best to maintain
> stable
> interfaces, and no, "..but the interface was broken" is _not_ an
> acceptable excuse for unbreaking it if it causes problems.
> 
> > > - but more importantly -
> > >
> > > 2) This patch is broken.
> >
> > You didn't indicate what was wrong with the patch.
> 
> Yes. Leaving everybody wondering whether it's just an opinionated
> expression of the former problem, or whether there is a real and
> understandable reason why it was NACK'ed.

Multiple thermal zone devices may share the same hwmon device.
here is an example,
# grep . /sys/class/hwmon/hwmon0/*
/sys/class/hwmon/hwmon0/name:acpitz
/sys/class/hwmon/hwmon0/temp1_crit:127000
/sys/class/hwmon/hwmon0/temp1_input:40000
/sys/class/hwmon/hwmon0/temp2_crit:100000
/sys/class/hwmon/hwmon0/temp2_input:44000

temp1 and temp2 stand for different ACPI thermal zones.

This patch will break if the first ACPI thermal zone is unregistered
before the others, although it only happens theoretically.
Plus, as this is not a 1:1 binding, a symbol link from the hwmon device
to thermal zone device doesn't make sense.

Following is a possible solution which is also from Rene,
while I think CONFIG_THERMAL_HWMON should be "default n".

thanks,
rui

>From 7b41d17346cdd1f9db1223c024bd9f4604ee5c82 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@gmail.com>
Date: Mon, 23 Jun 2008 21:50:25 +0200
Subject: [PATCH] acpi/thermal: allow disabling of thermal zone hwmon support

2.6.26-rc gained a hwmon interface to the Thermal Zone driver
which unfortunately breaks lm-sensors 2 userspace and renders
all other (subsequent) hwmon sensors inoperable also.

Many systems, current slackware and derivative systems among
them, are using lm-sensors 2 and would be affected. A new
lm-sensors 2.10.7 release is needed to fix this from the user
side.

This makes the hwmon support optional (enabled by default) and
comments that it needs a new enough lm-sensors userspace. This
also immediately schedules the option for removal again as this
is only intended to not break systems over the 2.6.26 upgrade
when new enough lm-sensors is not yet available or widely
deployed.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
CC: Len Brown <lenb@kernel.org>
CC: Hans de Goede <j.w.r.degoede@hhs.nl>
CC: Zhang Rui <rui.zhang@intel.com>
CC: Mark M. Hoffman <mhoffman@lightlink.com>
CC: Jean Delvare <khali@linux-fr.org>
CC: linux-acpi@vger.kernel.org
CC: lm-sensors@lm-sensors.org
CC: linux-kernel@vger.kernel.org
---
 Documentation/feature-removal-schedule.txt |    9 +++++++++
 drivers/thermal/Kconfig                    |   10 ++++++++++
 drivers/thermal/thermal_sys.c              |    4 ++--
 include/linux/thermal.h                    |    6 ++----
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 5b3f31f..46ece3f 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -312,3 +312,12 @@ When:	2.6.26
 Why:	Implementation became generic; users should now include
 	linux/semaphore.h instead.
 Who:	Matthew Wilcox <willy@linux.intel.com>
+
+---------------------------
+
+What:	CONFIG_THERMAL_HWMON
+When:	January 2009
+Why:	This option was introduced just to allow older lm-sensors userspace
+	to keep working over the upgrade to 2.6.26. At the scheduled time of
+	removal fixed lm-sensors (2.x or 3.x) should be readily available.
+Who:	Rene Herman <rene.herman@gmail.com>
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 4b62852..0b359ab 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -12,3 +12,13 @@ menuconfig THERMAL
 	  cooling devices.
 	  All platforms with ACPI thermal support can use this driver.
 	  If you want this support, you should say Y or M here.
+
+config THERMAL_HWMON
+	bool "Hardware monitoring support"
+	depends on HWMON=y || HWMON=THERMAL
+	default y
+	help
+	  The generic thermal sysfs driver's hardware monitoring support
+	  requires a 2.10.7/3.0.2 or later lm-sensors userspace.
+
+	  Say Y unless you need support for older lm-sensors.
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 6098787..fe07462 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -295,8 +295,8 @@ thermal_cooling_device_trip_point_show(struct device *dev,
 
 /* Device management */
 
-#if defined(CONFIG_HWMON) ||	\
-	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
+#if defined(CONFIG_THERMAL_HWMON)
+
 /* hwmon sys I/F */
 #include <linux/hwmon.h>
 static LIST_HEAD(thermal_hwmon_list);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 06d3e6e..917707e 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -66,8 +66,7 @@ struct thermal_cooling_device {
 				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
 #define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
 
-#if defined(CONFIG_HWMON) ||	\
-	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
+#if defined(CONFIG_THERMAL_HWMON)
 /* thermal zone devices with the same type share one hwmon device */
 struct thermal_hwmon_device {
 	char type[THERMAL_NAME_LENGTH];
@@ -94,8 +93,7 @@ struct thermal_zone_device {
 	struct idr idr;
 	struct mutex lock;	/* protect cooling devices list */
 	struct list_head node;
-#if defined(CONFIG_HWMON) ||	\
-	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
+#if defined(CONFIG_THERMAL_HWMON)
 	struct list_head hwmon_node;
 	struct thermal_hwmon_device *hwmon;
 	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
-- 
1.5.5

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Mark M. Hoffman]

Hi:

* Zhang Rui <rui.zhang@intel.com> [2008-06-24 11:47:38 +0800]:

(snip)

> Following is a possible solution which is also from Rene,
> while I think CONFIG_THERMAL_HWMON should be "default n".

I agree that it should default off.  With that change...

Acked-by: Mark M. Hoffman <mhoffman@lightlink.com>

> thanks,
> rui
> 
> >From 7b41d17346cdd1f9db1223c024bd9f4604ee5c82 Mon Sep 17 00:00:00 2001
> From: Rene Herman <rene.herman@gmail.com>
> Date: Mon, 23 Jun 2008 21:50:25 +0200
> Subject: [PATCH] acpi/thermal: allow disabling of thermal zone hwmon support
> 
> 2.6.26-rc gained a hwmon interface to the Thermal Zone driver
> which unfortunately breaks lm-sensors 2 userspace and renders
> all other (subsequent) hwmon sensors inoperable also.
> 
> Many systems, current slackware and derivative systems among
> them, are using lm-sensors 2 and would be affected. A new
> lm-sensors 2.10.7 release is needed to fix this from the user
> side.
> 
> This makes the hwmon support optional (enabled by default) and
> comments that it needs a new enough lm-sensors userspace. This
> also immediately schedules the option for removal again as this
> is only intended to not break systems over the 2.6.26 upgrade
> when new enough lm-sensors is not yet available or widely
> deployed.
> 
> Signed-off-by: Rene Herman <rene.herman@gmail.com>
> CC: Len Brown <lenb@kernel.org>
> CC: Hans de Goede <j.w.r.degoede@hhs.nl>
> CC: Zhang Rui <rui.zhang@intel.com>
> CC: Mark M. Hoffman <mhoffman@lightlink.com>
> CC: Jean Delvare <khali@linux-fr.org>
> CC: linux-acpi@vger.kernel.org
> CC: lm-sensors@lm-sensors.org
> CC: linux-kernel@vger.kernel.org
> ---
>  Documentation/feature-removal-schedule.txt |    9 +++++++++
>  drivers/thermal/Kconfig                    |   10 ++++++++++
>  drivers/thermal/thermal_sys.c              |    4 ++--
>  include/linux/thermal.h                    |    6 ++----
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> index 5b3f31f..46ece3f 100644
> --- a/Documentation/feature-removal-schedule.txt
> +++ b/Documentation/feature-removal-schedule.txt
> @@ -312,3 +312,12 @@ When:	2.6.26
>  Why:	Implementation became generic; users should now include
>  	linux/semaphore.h instead.
>  Who:	Matthew Wilcox <willy@linux.intel.com>
> +
> +---------------------------
> +
> +What:	CONFIG_THERMAL_HWMON
> +When:	January 2009
> +Why:	This option was introduced just to allow older lm-sensors userspace
> +	to keep working over the upgrade to 2.6.26. At the scheduled time of
> +	removal fixed lm-sensors (2.x or 3.x) should be readily available.
> +Who:	Rene Herman <rene.herman@gmail.com>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 4b62852..0b359ab 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -12,3 +12,13 @@ menuconfig THERMAL
>  	  cooling devices.
>  	  All platforms with ACPI thermal support can use this driver.
>  	  If you want this support, you should say Y or M here.
> +
> +config THERMAL_HWMON
> +	bool "Hardware monitoring support"
> +	depends on HWMON=y || HWMON=THERMAL
> +	default y
> +	help
> +	  The generic thermal sysfs driver's hardware monitoring support
> +	  requires a 2.10.7/3.0.2 or later lm-sensors userspace.
> +
> +	  Say Y unless you need support for older lm-sensors.
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 6098787..fe07462 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -295,8 +295,8 @@ thermal_cooling_device_trip_point_show(struct device *dev,
>  
>  /* Device management */
>  
> -#if defined(CONFIG_HWMON) ||	\
> -	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
> +#if defined(CONFIG_THERMAL_HWMON)
> +
>  /* hwmon sys I/F */
>  #include <linux/hwmon.h>
>  static LIST_HEAD(thermal_hwmon_list);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 06d3e6e..917707e 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -66,8 +66,7 @@ struct thermal_cooling_device {
>  				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
>  #define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
>  
> -#if defined(CONFIG_HWMON) ||	\
> -	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
> +#if defined(CONFIG_THERMAL_HWMON)
>  /* thermal zone devices with the same type share one hwmon device */
>  struct thermal_hwmon_device {
>  	char type[THERMAL_NAME_LENGTH];
> @@ -94,8 +93,7 @@ struct thermal_zone_device {
>  	struct idr idr;
>  	struct mutex lock;	/* protect cooling devices list */
>  	struct list_head node;
> -#if defined(CONFIG_HWMON) ||	\
> -	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
> +#if defined(CONFIG_THERMAL_HWMON)
>  	struct list_head hwmon_node;
>  	struct thermal_hwmon_device *hwmon;
>  	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
> -- 
> 1.5.5
> 
> 

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Rene Herman]

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

On 24-06-08 14:14, Mark M. Hoffman wrote:

> * Zhang Rui <rui.zhang@intel.com> [2008-06-24 11:47:38 +0800]:
> 
> (snip)
> 
>> Following is a possible solution which is also from Rene,
>> while I think CONFIG_THERMAL_HWMON should be "default n".
> 
> I agree that it should default off.  With that change...
> 
> Acked-by: Mark M. Hoffman <mhoffman@lightlink.com>

Len suggested the default be hwmon support enabled; I'm okay either way. 
Here it is again with only the 'default y' dropped (and such noted in 
the changelog).

Before it slips from the thread -- this again while noting that with a 
fixed libsensors 2 on the way just not fixing it is an option.

Rene

[-- Attachment #2: 0001-acpi-thermal-allow-disabling-of-thermal-zone-hwmon.patch --]
[-- Type: text/plain, Size: 4408 bytes --]

>From 85b13a5ad5b6561f9ca7c2dd6889f0d1bf0af2f4 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@gmail.com>
Date: Mon, 23 Jun 2008 21:50:25 +0200
Subject: [PATCH] acpi/thermal: allow disabling of thermal zone hwmon support

2.6.26-rc gained a hwmon interface to the Thermal Zone driver
which unfortunately breaks lm-sensors 2 userspace and renders
all other (subsequent) hwmon sensors inoperable also.

Many systems, current slackware and derivative systems among
them, are using lm-sensors 2 and would be affected. A new
lm-sensors 2.10.7 release is needed to fix this from the user
side.

This makes the hwmon support optional, disabled by default and
comments that it needs a new enough lm-sensors userspace. This
also immediately schedules the option for removal again as this
is only intended to not break systems over the 2.6.26 upgrade
when new enough lm-sensors is not yet available or widely
deployed.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
Acked-by: Mark M. Hoffman <mhoffman@lightlink.com>
CC: Len Brown <lenb@kernel.org>
CC: Hans de Goede <j.w.r.degoede@hhs.nl>
CC: Zhang Rui <rui.zhang@intel.com>
CC: Jean Delvare <khali@linux-fr.org>
CC: linux-acpi@vger.kernel.org
CC: lm-sensors@lm-sensors.org
CC: linux-kernel@vger.kernel.org
---
 Documentation/feature-removal-schedule.txt |    9 +++++++++
 drivers/thermal/Kconfig                    |    9 +++++++++
 drivers/thermal/thermal_sys.c              |    4 ++--
 include/linux/thermal.h                    |    6 ++----
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 5b3f31f..46ece3f 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -312,3 +312,12 @@ When:	2.6.26
 Why:	Implementation became generic; users should now include
 	linux/semaphore.h instead.
 Who:	Matthew Wilcox <willy@linux.intel.com>
+
+---------------------------
+
+What:	CONFIG_THERMAL_HWMON
+When:	January 2009
+Why:	This option was introduced just to allow older lm-sensors userspace
+	to keep working over the upgrade to 2.6.26. At the scheduled time of
+	removal fixed lm-sensors (2.x or 3.x) should be readily available.
+Who:	Rene Herman <rene.herman@gmail.com>
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 4b62852..6787354 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -12,3 +12,12 @@ menuconfig THERMAL
 	  cooling devices.
 	  All platforms with ACPI thermal support can use this driver.
 	  If you want this support, you should say Y or M here.
+
+config THERMAL_HWMON
+	bool "Hardware monitoring support"
+	depends on HWMON=y || HWMON=THERMAL
+	help
+	  The generic thermal sysfs driver's hardware monitoring support
+	  requires a 2.10.7/3.0.2 or later lm-sensors userspace.
+
+	  Say Y unless you need support for older lm-sensors.
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 6098787..fe07462 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -295,8 +295,8 @@ thermal_cooling_device_trip_point_show(struct device *dev,
 
 /* Device management */
 
-#if defined(CONFIG_HWMON) ||	\
-	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
+#if defined(CONFIG_THERMAL_HWMON)
+
 /* hwmon sys I/F */
 #include <linux/hwmon.h>
 static LIST_HEAD(thermal_hwmon_list);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 06d3e6e..917707e 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -66,8 +66,7 @@ struct thermal_cooling_device {
 				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
 #define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
 
-#if defined(CONFIG_HWMON) ||	\
-	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
+#if defined(CONFIG_THERMAL_HWMON)
 /* thermal zone devices with the same type share one hwmon device */
 struct thermal_hwmon_device {
 	char type[THERMAL_NAME_LENGTH];
@@ -94,8 +93,7 @@ struct thermal_zone_device {
 	struct idr idr;
 	struct mutex lock;	/* protect cooling devices list */
 	struct list_head node;
-#if defined(CONFIG_HWMON) ||	\
-	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
+#if defined(CONFIG_THERMAL_HWMON)
 	struct list_head hwmon_node;
 	struct thermal_hwmon_device *hwmon;
 	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
-- 
1.5.5

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Len Brown]

> Len suggested the default be hwmon support enabled

I think you were right and I was wrong on that part,
lets default =n.

What is new here is thermal I/F to hwmon.
Distros what want it will know they need libsensors 2.10.7
or libsensors 3.x before they turn it on.

libsensors <= 2.10.6 users who upgrade their kernel to 2.6.26
without distro support will get no bad surprises.

The down-side is simply that we are being less aggressive about
exposing these temperature readings via hwmon.
I'm not going to lose any sleep over that.

-Len

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Matthew Garrett]

On Wed, Jun 25, 2008 at 07:12:12PM -0400, Len Brown wrote:

> libsensors <= 2.10.6 users who upgrade their kernel to 2.6.26
> without distro support will get no bad surprises.
> 
> The down-side is simply that we are being less aggressive about
> exposing these temperature readings via hwmon.
> I'm not going to lose any sleep over that.

I'd agree. I don't think this is strictly necessary, but it makes life 
easier for users rolling their own kernel. Exposing these values via 
hwmon is nice, but not vital. They're not going to be able to make use 
of it without the new code.

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

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Len Brown]

> +config THERMAL_HWMON
> +	bool "Hardware monitoring support"
> +	depends on HWMON=y || HWMON=THERMAL


for
CONFIG_HWMON=m
CONFIG_THERMAL=y
CONFIG_THERMAL_HWMON=n, always.

This is not a change to the logic of the existing ifdefs,
but was wondering if this was your intent, Rui.

-Len

> -#if defined(CONFIG_HWMON) ||	\
> -	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
> +#if defined(CONFIG_THERMAL_HWMON)
> +
>  /* hwmon sys I/F */

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Rene Herman]

On 26-06-08 01:16, Len Brown wrote:
> 
>> +config THERMAL_HWMON
>> +	bool "Hardware monitoring support"
>> +	depends on HWMON=y || HWMON=THERMAL
> 
> 
> for
> CONFIG_HWMON=m
> CONFIG_THERMAL=y
> CONFIG_THERMAL_HWMON=n, always.
> 
> This is not a change to the logic of the existing ifdefs,
> but was wondering if this was your intent, Rui.

My intentioon in this case. Yes, this should be. THERMAL_HWMON is a bool 
(as in, not a tristate) that just changes THERMAL.

If THERMAL=y, we need HWMON=y or we'd get a link failure. If THERMAL=m, 
we're okay with either HWMON=y and HWMON=m and if THERMAL=n, we don't 
care one hoot about THERMAL_HWMON...

Rene.

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Zhang Rui]

On Thu, 2008-06-26 at 07:38 +0800, Rene Herman wrote:
> On 26-06-08 01:16, Len Brown wrote:
> >
> >> +config THERMAL_HWMON
> >> +    bool "Hardware monitoring support"
> >> +    depends on HWMON=y || HWMON=THERMAL
> >
> >
> > for
> > CONFIG_HWMON=m
> > CONFIG_THERMAL=y
> > CONFIG_THERMAL_HWMON=n, always.
> >
> > This is not a change to the logic of the existing ifdefs,
> > but was wondering if this was your intent, Rui.
> 
> My intentioon in this case. Yes, this should be. THERMAL_HWMON is a
> bool
> (as in, not a tristate) that just changes THERMAL.
> 
> If THERMAL=y, we need HWMON=y or we'd get a link failure. If
> THERMAL=m,
> we're okay with either HWMON=y and HWMON=m and if THERMAL=n, we don't
> care one hoot about THERMAL_HWMON...
Agree.

Acked-by: Zhang Rui <rui.zhang@intel.com>

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Jean Delvare]

Hi Linus,

On Mon, 23 Jun 2008 20:07:15 -0700 (PDT), Linus Torvalds wrote:
> On Mon, 23 Jun 2008, Robert Hancock wrote:
> > Mark M. Hoffman wrote:
> > > 2) This patch is broken.
> > 
> > You didn't indicate what was wrong with the patch.
> 
> Yes. Leaving everybody wondering whether it's just an opinionated 
> expression of the former problem, or whether there is a real and 
> understandable reason why it was NACK'ed.

I guess Mark assumed that everybody had read the discussion thread Rene
referenced, where I explained why his patch was broken:

http://lkml.org/lkml/2008/6/23/175

-- 
Jean Delvare

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Rene Herman]

On 24-06-08 08:59, Jean Delvare wrote:

> On Mon, 23 Jun 2008 20:07:15 -0700 (PDT), Linus Torvalds wrote:
>> On Mon, 23 Jun 2008, Robert Hancock wrote:
>>> Mark M. Hoffman wrote:
>>>> 2) This patch is broken.
>>> You didn't indicate what was wrong with the patch.
>> Yes. Leaving everybody wondering whether it's just an opinionated 
>> expression of the former problem, or whether there is a real and 
>> understandable reason why it was NACK'ed.
> 
> I guess Mark assumed that everybody had read the discussion thread Rene
> referenced, where I explained why his patch was broken:
> 
> http://lkml.org/lkml/2008/6/23/175

You could've known that he himself didn't because in that same thread I 
explained to you why it wasn't broken in actual real life.

That said, I agree that with the issue of multiple thermal zones sharing 
a single hwmon interface (as also indicated in the changelog) it's not a 
nice solution. Len Brown also indicated that the "make it optional" 
patch (originally nacked as ugly by Hans, which was the only reason I 
tried getting you people something else again...) that I saw Zhang Rui 
repost just now would be better than that.

And, as said, now that lm-sensors 2.10.7 is close, I don't actually 
think it's a huge deal anymore anyway. The breakage is still pretty 
unfortunate, but that one should be painless enough as an upgrade to 
shrug this off really. It was the upgrade path to 3.x which would've 
been much too painful to have this done without any prior warning.

So we're waiting for 2.10.7 now.

Rene.

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Mark M. Hoffman]

Hi Linus:

* Linus Torvalds <torvalds@linux-foundation.org> [2008-06-23 20:07:15 -0700]:
> 
> 
> On Mon, 23 Jun 2008, Robert Hancock wrote:
> 
> > Mark M. Hoffman wrote:
> > > 
> > > 1) The bug is in libsensors (2.10), not the kernel.
> > 
> > This doesn't matter. Breaking userspace in such a fashion is severely frowned
> > upon unless essentially unavoidable, even if it is just triggering a bug.
> 
> I do tend to agree. If it used to work, it should continue to work, even 
> if it wasn't _meant_ to work.
> 
> That's not always 100% possible, but the fact is, neither is updating user 
> space always possible. The kernel needs to try its best to maintain stable 
> interfaces, and no, "..but the interface was broken" is _not_ an 
> acceptable excuse for unbreaking it if it causes problems.

No excuses... it's just a simple case of userspace making a bad assumption.
(Queue rant about how sysfs interfaces are prone to this sort of thing.)

> > > - but more importantly -
> > > 
> > > 2) This patch is broken.
> > 
> > You didn't indicate what was wrong with the patch.
> 
> Yes. Leaving everybody wondering whether it's just an opinionated 
> expression of the former problem, or whether there is a real and 
> understandable reason why it was NACK'ed.

IMO Jean Delvare and Zhang Rui had explained this well enough elsewhere in the
thread, but...

In summary: the patch adds a meaningless symbolic link to an arbitrary thermal
zone device where the was none before.  This keeps (one version of) libsensors
from tripping over itself.  But if the TZ devices are unregistered in the wrong
order, OOPS.  Apparently this can't happen in practice; but it's a bad trade
to add an implicit dependency on the order of device reg/unreg just to wall-
paper over a bug in userspace (IMHO).

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

[PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Rene Herman]

Hi Linus.

2.6.26-rc gained a hwmon interface to the ACPI Thermal Zone driver
which unfortunately breaks lm-sensors 2 userspace and renders all
other (subsequent) hwmon sensors inoperable also.
 
Many systems, current slackware and derivative systems among them,
are using lm-sensors 2 and would be affected. A new lm-sensors
2.10.7 release is needed to fix this from the user side.

There's disagreement on how bad the problem is, what it should be
called and what correct levels of respect and gratitude are.
Thread at:

http://lkml.org/lkml/2008/6/21/173

Admittedly, with lm-sensors 2.10.7 around the corner now, there's
opportunity to not care all that deeply but I want to make sure
that everyone is aware of this as it is definitely going to be
reported and most importantly -- losing sensors can be a vital
mistake under some circumstances. Fortunately, it's not silent
breakage.

A previous patch just made the support optional:

http://lkml.org/lkml/2008/6/22/83

which also works but is not-nicer still.

Off to my shrubbery now...

Rene.

>From 303b6d3f407def5aa15bedc1bc7ff3f090025fe5 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@gmail.com>
Date: Sun, 22 Jun 2008 23:45:04 +0200
Subject: [PATCH] thermal: link the virtual/thermal/thermal_zoneN device for hwmon

2.6.26-rc gained a hwmon interface to the ACPI Thermal Zone
driver which unfortunately breaks lm-sensors 2 userspace and
renders all other (subsequent) hwmon sensors inoperable also.

Many systems, current slackware and derivative systems among them,
are using lm-sensors 2 and would be affected. A new lm-sensors
2.10.7 release is needed to fix this from the user side.

The problem is userspace tripping over an absent "device"
link in the ACPI Thermal Zone sysfs /sys/class/hwmon/hwmonN
directory. This just adds the virtual "thermal/thermal_zoneN"
device as a device backlink which satisfies lm-sensors again.

Tested and fairly obvious; a downside is that the ACPI Thermal
Zone driver gathers multiple thermal zones into one hwmon
interface which means the device link is sensible only for
the first thermal zone. Point here is just to not trip up
existing lm-sensors 2 systems though.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
CC: Hans de Goede <j.w.r.degoede@hhs.nl>
CC: Zhang Rui <rui.zhang@intel.com>
CC: Mark M. Hoffman <mhoffman@lightlink.com>
CC: Jean Delvare <khali@linux-fr.org>
CC: linux-acpi@vger.kernel.org
CC: lm-sensors@lm-sensors.org
CC: linux-kernel@vger.kernel.org
---
 drivers/thermal/thermal_sys.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 6098787..c21e03c 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -357,7 +357,7 @@ thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 
 	INIT_LIST_HEAD(&hwmon->tz_list);
 	strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
-	hwmon->device = hwmon_device_register(NULL);
+	hwmon->device = hwmon_device_register(&tz->device);
 	if (IS_ERR(hwmon->device)) {
 		result = PTR_ERR(hwmon->device);
 		goto free_mem;
-- 
1.5.5

Re: [PATCH 2.6.26-rc] ACPI Thermal Zone driver breaks lm-sensors 2 userspace

[Mark M. Hoffman]

Linus, Rene, et al:

* Rene Herman <rene.herman@keyaccess.nl> [2008-06-23 17:06:14 +0200]:
> drivers/thermal/thermal_sys.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 6098787..c21e03c 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -357,7 +357,7 @@ thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
> 
> 	INIT_LIST_HEAD(&hwmon->tz_list);
> 	strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> -	hwmon->device = hwmon_device_register(NULL);
> +	hwmon->device = hwmon_device_register(&tz->device);
> 	if (IS_ERR(hwmon->device)) {
> 		result = PTR_ERR(hwmon->device);
> 		goto free_mem;

NACK.

1) The bug is in libsensors (2.10), not the kernel.

- but more importantly -

2) This patch is broken.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com