[PATCH] thermal: Drop CONFIG_THERMAL_HWMON

[PATCH] thermal: Drop CONFIG_THERMAL_HWMON

[Jean Delvare]

It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
Anybody running a kernel >= 2.6.40 would also be running a recent
enough version of lm-sensors.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Rene Herman <rene.herman@gmail.com>
Cc: Len Brown <len.brown@intel.com>
---
Len, will you pick this in one of your trees? There doesn't seem to be
a maintainer for the thermal subsystem.

 Documentation/feature-removal-schedule.txt |    9 ---------
 drivers/thermal/Kconfig                    |   10 ----------
 drivers/thermal/thermal_sys.c              |    4 ++--
 include/linux/thermal.h                    |    6 ++++--
 4 files changed, 6 insertions(+), 23 deletions(-)

--- linux-2.6.39-rc4.orig/Documentation/feature-removal-schedule.txt	2011-04-23 10:38:10.000000000 +0200
+++ linux-2.6.39-rc4/Documentation/feature-removal-schedule.txt	2011-04-23 14:47:59.000000000 +0200
@@ -295,15 +295,6 @@ Who:	Ravikiran Thirumalai <kiran@scalex8
 
 ---------------------------
 
-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>
-
----------------------------
-
 What:	Code that is now under CONFIG_WIRELESS_EXT_SYSFS
 	(in net/core/net-sysfs.c)
 When:	After the only user (hal) has seen a release with the patches
--- linux-2.6.39-rc4.orig/drivers/thermal/Kconfig	2010-08-02 00:11:14.000000000 +0200
+++ linux-2.6.39-rc4/drivers/thermal/Kconfig	2011-04-23 14:48:54.000000000 +0200
@@ -12,13 +12,3 @@ 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 THERMAL
-	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 if your user-space is new enough.
--- linux-2.6.39-rc4.orig/drivers/thermal/thermal_sys.c	2011-03-30 10:57:31.000000000 +0200
+++ linux-2.6.39-rc4/drivers/thermal/thermal_sys.c	2011-04-23 14:47:40.000000000 +0200
@@ -416,8 +416,8 @@ thermal_cooling_device_trip_point_show(s
 
 /* Device management */
 
-#if defined(CONFIG_THERMAL_HWMON)
-
+#if defined(CONFIG_HWMON) ||	\
+	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
 /* hwmon sys I/F */
 #include <linux/hwmon.h>
 static LIST_HEAD(thermal_hwmon_list);
--- linux-2.6.39-rc4.orig/include/linux/thermal.h	2011-03-15 22:53:23.000000000 +0100
+++ linux-2.6.39-rc4/include/linux/thermal.h	2011-04-23 14:50:05.000000000 +0200
@@ -85,7 +85,8 @@ 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_THERMAL_HWMON)
+#if defined(CONFIG_HWMON) ||	\
+	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
 /* thermal zone devices with the same type share one hwmon device */
 struct thermal_hwmon_device {
 	char type[THERMAL_NAME_LENGTH];
@@ -120,7 +121,8 @@ struct thermal_zone_device {
 	struct mutex lock;	/* protect cooling devices list */
 	struct list_head node;
 	struct delayed_work poll_queue;
-#if defined(CONFIG_THERMAL_HWMON)
+#if defined(CONFIG_HWMON) ||	\
+	(defined(CONFIG_HWMON_MODULE) && defined(CONFIG_THERMAL_MODULE))
 	struct list_head hwmon_node;
 	struct thermal_hwmon_device *hwmon;
 	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */


-- 
Jean Delvare

Re: [PATCH] thermal: Drop CONFIG_THERMAL_HWMON

[Rene Herman]

On 04/23/2011 03:07 PM, Jean Delvare wrote:

> It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
> Anybody running a kernel>= 2.6.40 would also be running a recent
> enough version of lm-sensors.
>
> Signed-off-by: Jean Delvare<khali@linux-fr.org>
> Cc: Rene Herman<rene.herman@gmail.com>
> Cc: Len Brown<len.brown@intel.com>

Acked-by: Rene Herman <rene.herman@gmail.com>

Rene.

Re: [lm-sensors] [PATCH] thermal: Drop CONFIG_THERMAL_HWMON

[Guenter Roeck]

On Sat, Apr 23, 2011 at 09:07:03AM -0400, Jean Delvare wrote:
> It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
> Anybody running a kernel >= 2.6.40 would also be running a recent
> enough version of lm-sensors.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Rene Herman <rene.herman@gmail.com>
> Cc: Len Brown <len.brown@intel.com>

Two other and less complex options:

- Add
	select THERMAL_HWMON if HWMON=y || HWMON=THERMAL
  to "menuconfig THERMAL", and keep THERMAL_HWMON as non-selectable bool
  [ I don't know what HWMON=THERMAL means, though ]
- Add
	select HWMON
  to "menuconfig THERMAL", and remove THERMAL_HWMON.

Guenter

Re: [lm-sensors] [PATCH] thermal: Drop CONFIG_THERMAL_HWMON

[Jean Delvare]

Hi Guenter,

On Sat, 23 Apr 2011 18:58:53 -0700, Guenter Roeck wrote:
> On Sat, Apr 23, 2011 at 09:07:03AM -0400, Jean Delvare wrote:
> > It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
> > Anybody running a kernel >= 2.6.40 would also be running a recent
> > enough version of lm-sensors.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Rene Herman <rene.herman@gmail.com>
> > Cc: Len Brown <len.brown@intel.com>
> 
> Two other and less complex options:
> 
> - Add
> 	select THERMAL_HWMON if HWMON=y || HWMON=THERMAL
>   to "menuconfig THERMAL", and keep THERMAL_HWMON as non-selectable bool
>   [ I don't know what HWMON=THERMAL means, though ]

HWMON=THERMAL means: both HWMON and THERMAL built as modules, or both
built into the kernel, or neither built at all. Given that the "y" case
is already mentioned explicitly, and the whole thing isn't even
evaluated if THERMAL=n, HWMON=THERMAL here is essentially a shortcut
for "THERMAL=m && HWMON=m".

> - Add
> 	select HWMON
>   to "menuconfig THERMAL", and remove THERMAL_HWMON.

Thanks for the suggestions. I admit I blindly reverted the original
commit without putting much thought into possible alternatives.

Your second suggestion is by far the most simple. However it has a
drawback, it makes it impossible to select THERMAL without HWMON. This
is no concern to me, but maybe some embedded systems will be unhappy
about this. HWMON has no interest in the absence of local users.

Your first suggestion makes a lot of sense, as it avoids repeating the
same non-trivial conditional construct in 3 different places. But OTOH
it leaves in Kconfig an option that shouldn't be there, as nobody
should depend on its value. I consider it an abuse of the Kconfig
system.

In fact, my impression is that the repeated tests, and the fact that two
of them are in <linux/thermal.h>, are the consequence of bad code
design. The fact that THERMAL_HWMON affects a public structure when the
implementation is completely internal to the thermal driver makes no
sense. It should be possible to implement things differently.

Len, please hold on until we agree on the best way to address the issue.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH] thermal: Drop CONFIG_THERMAL_HWMON

[Guenter Roeck]

On Sun, Apr 24, 2011 at 03:40:53PM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Sat, 23 Apr 2011 18:58:53 -0700, Guenter Roeck wrote:
> > On Sat, Apr 23, 2011 at 09:07:03AM -0400, Jean Delvare wrote:
> > > It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
> > > Anybody running a kernel >= 2.6.40 would also be running a recent
> > > enough version of lm-sensors.
> > > 
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > > Cc: Rene Herman <rene.herman@gmail.com>
> > > Cc: Len Brown <len.brown@intel.com>
> > 
> > Two other and less complex options:
> > 
> > - Add
> > 	select THERMAL_HWMON if HWMON=y || HWMON=THERMAL
> >   to "menuconfig THERMAL", and keep THERMAL_HWMON as non-selectable bool
> >   [ I don't know what HWMON=THERMAL means, though ]
> 
> HWMON=THERMAL means: both HWMON and THERMAL built as modules, or both
> built into the kernel, or neither built at all. Given that the "y" case
> is already mentioned explicitly, and the whole thing isn't even
> evaluated if THERMAL=n, HWMON=THERMAL here is essentially a shortcut
> for "THERMAL=m && HWMON=m".
> 
> > - Add
> > 	select HWMON
> >   to "menuconfig THERMAL", and remove THERMAL_HWMON.
> 
> Thanks for the suggestions. I admit I blindly reverted the original
> commit without putting much thought into possible alternatives.
> 
> Your second suggestion is by far the most simple. However it has a
> drawback, it makes it impossible to select THERMAL without HWMON. This
> is no concern to me, but maybe some embedded systems will be unhappy
> about this. HWMON has no interest in the absence of local users.
> 
Yes, that is why I suggested it as second option. I don't think the assertion
is correct, though. There could well be a SNMP hardware monitoring MIB
implementation (or something similar) using the hwmon sysfs ABI to interface
with the kernel; such an interface to the outside world would make a lot of sense
in an embedded system.

> Your first suggestion makes a lot of sense, as it avoids repeating the
> same non-trivial conditional construct in 3 different places. But OTOH
> it leaves in Kconfig an option that shouldn't be there, as nobody
> should depend on its value. I consider it an abuse of the Kconfig
> system.
> 
Not really; that kind of construct is used all over the place. Sure, 
it is commonly used if the same option is auto-enabled from more than one
other options, but in my opinion it makes sense here as well: If one wants 
to change the condition for enabling THERMAL_HWMON, it is easier to change
in one place instead of having to track down all the places in the code.

Yet another possibility, which is used all over the place, would be

config THERMAL_HWMON
	bool
	depends on THERMAL
	depends on HWMON=y || HWMON=THERMAL
	default y

ie remove the ability to select it manually. Maybe that would be more appropriate.

> In fact, my impression is that the repeated tests, and the fact that two
> of them are in <linux/thermal.h>, are the consequence of bad code
> design. The fact that THERMAL_HWMON affects a public structure when the
> implementation is completely internal to the thermal driver makes no
> sense. It should be possible to implement things differently.
> 
Yes, it definitely violates the notion of "no ifdefs in the code". It might be 
more difficult to get that kind of code into the kernel today.

Thanks,
Guenter

Re: [PATCH] thermal: Drop CONFIG_THERMAL_HWMON

[Jean Delvare]

Hi Guenter,

On Sun, 24 Apr 2011 13:56:47 -0700, Guenter Roeck wrote:
> On Sun, Apr 24, 2011 at 03:40:53PM -0400, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > On Sat, 23 Apr 2011 18:58:53 -0700, Guenter Roeck wrote:
> > > On Sat, Apr 23, 2011 at 09:07:03AM -0400, Jean Delvare wrote:
> > > > It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
> > > > Anybody running a kernel >= 2.6.40 would also be running a recent
> > > > enough version of lm-sensors.
> > > > 
> > > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > > > Cc: Rene Herman <rene.herman@gmail.com>
> > > > Cc: Len Brown <len.brown@intel.com>
> > > 
> > > Two other and less complex options:
> > > 
> > > - Add
> > > 	select THERMAL_HWMON if HWMON=y || HWMON=THERMAL
> > >   to "menuconfig THERMAL", and keep THERMAL_HWMON as non-selectable bool
> > >   [ I don't know what HWMON=THERMAL means, though ]
> > 
> > HWMON=THERMAL means: both HWMON and THERMAL built as modules, or both
> > built into the kernel, or neither built at all. Given that the "y" case
> > is already mentioned explicitly, and the whole thing isn't even
> > evaluated if THERMAL=n, HWMON=THERMAL here is essentially a shortcut
> > for "THERMAL=m && HWMON=m".
> > 
> > > - Add
> > > 	select HWMON
> > >   to "menuconfig THERMAL", and remove THERMAL_HWMON.
> > 
> > Thanks for the suggestions. I admit I blindly reverted the original
> > commit without putting much thought into possible alternatives.
> > 
> > Your second suggestion is by far the most simple. However it has a
> > drawback, it makes it impossible to select THERMAL without HWMON. This
> > is no concern to me, but maybe some embedded systems will be unhappy
> > about this. HWMON has no interest in the absence of local users.
> > 
> Yes, that is why I suggested it as second option. I don't think the assertion
> is correct, though. There could well be a SNMP hardware monitoring MIB
> implementation (or something similar) using the hwmon sysfs ABI to interface
> with the kernel; such an interface to the outside world would make a lot of sense
> in an embedded system.

What I had in mind was more like my Linux-based portable media player.
No local user and no networking capabilities, so hardware monitoring is
pointless, while thermal management may be desirable.

> 
> > Your first suggestion makes a lot of sense, as it avoids repeating the
> > same non-trivial conditional construct in 3 different places. But OTOH
> > it leaves in Kconfig an option that shouldn't be there, as nobody
> > should depend on its value. I consider it an abuse of the Kconfig
> > system.
>
> Not really; that kind of construct is used all over the place. Sure, 

Doesn't mean it's right ;)

> it is commonly used if the same option is auto-enabled from more than one
> other options, but in my opinion it makes sense here as well: If one wants 
> to change the condition for enabling THERMAL_HWMON, it is easier to change
> in one place instead of having to track down all the places in the code.

Good point.

> Yet another possibility, which is used all over the place, would be
> 
> config THERMAL_HWMON
> 	bool
> 	depends on THERMAL
> 	depends on HWMON=y || HWMON=THERMAL
> 	default y
> 
> ie remove the ability to select it manually. Maybe that would be more appropriate.

I like it, I think I'll do that, thanks for the suggestion.

> > In fact, my impression is that the repeated tests, and the fact that two
> > of them are in <linux/thermal.h>, are the consequence of bad code
> > design. The fact that THERMAL_HWMON affects a public structure when the
> > implementation is completely internal to the thermal driver makes no
> > sense. It should be possible to implement things differently.
>
> Yes, it definitely violates the notion of "no ifdefs in the code". It might be 
> more difficult to get that kind of code into the kernel today.

Not sure what "notion" you are referring to, but this particular piece
of code isn't that old. There are a lot of config-dependent pieces of
code in the kernel tree, and I don't think this is a problem. The
problem I have here is with the implementation details and their
consequences. Same as including header files which you don't need, it
causes useless rebuilds. Furthermore, the fact that unsetting (or
setting) CONFIG_HWMON will cause a kABI breakage for all users of
thermal is pretty nasty for distributions. I would like to fix this (in
a different patch, of course.)

-- 
Jean Delvare