[lm-sensors] [PATCH] Make fancontrol more robust to kernel changes

[Jean Delvare <khali@linux-fr.org>]

Hi all,

As newer kernels have improved support for ACPI-based hardware
monitoring (be it using the generic thermal_sys driver, or
asus_atk0110), it can happen that a kernel upgrade causes hardware
monitoring class devices (/sys/class/hwmon/*) to change without any
user interaction. While libsensors doesn't care too much, fancontrol
does, because it uses the hwmon class device numbers for
identification. This can result in a totally broken fan control
behavior, as was reported here:

https://bugzilla.novell.com/show_bug.cgi?idR9483

At this point, best would probably be to admit that fancontrol was just
a quick hack and was never meant for production, and rewrite it in a
language that can make use of libsensors. This is however more work
than I can afford spending on this myself.

So, as a temporary workaround, I propose to extend the /etc/fancontrol
configuration file syntax to record the physical device path and name
of each used hwmon class device. pwmconfig will be in charge of writing
the information at configuration time, and fancontrol will have to
check it at run time. If the values do not match, fancontrol will exit
immediately: better no fan control at all than fan control based on
broken assumptions.

I decided to record both the path and the name because:
* Not all hwmon devices have a physical device path. Purely virtual
  devices (for example thermal_sys) don't. So checking only the path
  wouldn't be sufficient.
* The device name is not unique. It is perfectly possible for two
  devices to have the same name (for example "coretemp").
So neither the path nor the name is sufficient. I hope that having both
will be good enough, even though I am well aware it isn't bullet-proof.

One drawback is that _everyone_ will have to run pwmconfig again when
using this new version of fancontrol. But without a time machine, I
just can't think of a way to avoid it.

Here's the patch, comments welcome:

Index: prog/pwm/fancontrol
=================================--- prog/pwm/fancontrol	(révision 5766)
+++ prog/pwm/fancontrol	(copie de travail)
@@ -3,12 +3,12 @@
 # Simple script implementing a temperature dependent fan speed control
 # Supported Linux kernel versions: 2.6.5 and later
 #
-# Version 0.69
+# Version 0.70
 #
 # Usage: fancontrol [CONFIGFILE]
 #
 # Dependencies:
-#   bash, egrep, sed, cut, sleep, lm_sensors :)
+#   bash, egrep, sed, cut, sleep, readlink, lm_sensors :)
 #
 # Please send any questions, comments or success stories to
 # marius.reiner@hdev.de
@@ -55,6 +55,8 @@
 
 	# grep configuration from file
 	INTERVAL=`egrep '^INTERVAL=.*$' $1 | sed -e 's/INTERVAL=//g'`
+	DEVPATH=`egrep '^DEVPATH=.*$' $1 | sed -e 's/DEVPATH= *//g'`
+	DEVNAME=`egrep '^DEVNAME=.*$' $1 | sed -e 's/DEVNAME= *//g'`
 	FCTEMPS=`egrep '^FCTEMPS=.*$' $1 | sed -e 's/FCTEMPS=//g'`
 	MINTEMP=`egrep '^MINTEMP=.*$' $1 | sed -e 's/MINTEMP=//g'`
 	MAXTEMP=`egrep '^MAXTEMP=.*$' $1 | sed -e 's/MAXTEMP=//g'`
@@ -152,6 +154,57 @@
 	echo
 }
 
+function DevicePath()
+{
+	if [ -d "$1/device" ]
+	then
+		readlink -f "$1/device" | sed -e 's/^\/sys\///'
+	fi
+}
+
+function DeviceName()
+{
+	if [ -r "$1/name" ]
+	then
+		cat "$1/name" | sed -e 's/[[:space:]=]/_/g'
+	elif [ -r "$1/device/name" ]
+	then
+		cat "$1/device/name" | sed -e 's/[[:space:]=]/_/g'
+	fi
+}
+
+function ValidateDevices()
+{
+	local OLD_DEVPATH="$1" OLD_DEVNAME="$2" outdated=0
+	local entry device name path
+
+	for entry in $OLD_DEVPATH
+	do
+		device=`echo "$entry" | sed -e 's/=[^=]*$//'`
+		path=`echo "$entry" | sed -e 's/^[^=]*=//'`
+
+		if [ "`DevicePath "$device"`" != "$path" ]
+		then
+			echo "Device path of $device has changed"
+			outdated=1
+		fi
+	done
+
+	for entry in $OLD_DEVNAME
+	do
+		device=`echo "$entry" | sed -e 's/=[^=]*$//'`
+		name=`echo "$entry" | sed -e 's/^[^=]*=//'`
+
+		if [ "`DeviceName "$device"`" != "$name" ]
+		then
+			echo "Device name of $device has changed"
+			outdated=1
+		fi
+	done
+
+	return $outdated
+}
+
 # Check that all referenced sysfs files exist
 function CheckFiles {
 	local outdated=0
@@ -232,6 +285,17 @@
 fi
 cd $DIR
 
+# Check for configuration change
+if [ -z "$DEVPATH" -o -z "$DEVNAME" ]
+then
+	echo "Configuration is too old, please run pwmconfig again"
+	exit 1
+fi
+if ! ValidateDevices "$DEVPATH" "$DEVNAME"
+then
+	echo "Configuration appears to be outdated, please run pwmconfig again"
+	exit 1
+fi
 CheckFiles || exit 1
 
 if [ -f "$PIDFILE" ]
Index: prog/pwm/pwmconfig
=================================--- prog/pwm/pwmconfig	(révision 5767)
+++ prog/pwm/pwmconfig	(copie de travail)
@@ -539,6 +539,57 @@
 	exit 1
 fi
 
+function DevicePath()
+{
+	if [ -d "$1/device" ]
+	then
+		readlink -f "$1/device" | sed -e 's/^\/sys\///'
+	fi
+}
+
+function DeviceName()
+{
+	if [ -r "$1/name" ]
+	then
+		cat "$1/name" | sed -e 's/[[:space:]=]/_/g'
+	elif [ -r "$1/device/name" ]
+	then
+		cat "$1/device/name" | sed -e 's/[[:space:]=]/_/g'
+	fi
+}
+
+function ValidateDevices()
+{
+	local OLD_DEVPATH="$1" OLD_DEVNAME="$2" outdated=0
+	local entry device name path
+
+	for entry in $OLD_DEVPATH
+	do
+		device=`echo "$entry" | sed -e 's/=[^=]*$//'`
+		path=`echo "$entry" | sed -e 's/^[^=]*=//'`
+
+		if [ "`DevicePath "$device"`" != "$path" ]
+		then
+			echo "Device path of $device has changed"
+			outdated=1
+		fi
+	done
+
+	for entry in $OLD_DEVNAME
+	do
+		device=`echo "$entry" | sed -e 's/=[^=]*$//'`
+		name=`echo "$entry" | sed -e 's/^[^=]*=//'`
+
+		if [ "`DeviceName "$device"`" != "$name" ]
+		then
+			echo "Device name of $device has changed"
+			outdated=1
+		fi
+	done
+
+	return $outdated
+}
+
 function AskPath()
 {
 	echo -n 'What should be the path to your fancontrol config file (/etc/fancontrol)? '
@@ -566,6 +617,8 @@
 
 function LoadConfig()
 {
+	local OLD_DEVPATH OLD_DEVNAME
+
 	# Nothing to do
 	if [ ! -f "$1" ]
 	then
@@ -575,6 +628,8 @@
 
 	echo "Loading configuration from $1 ..."
 	INTERVAL=`egrep '^INTERVAL=.*$' $1 | sed -e 's/INTERVAL= *//g'`
+	OLD_DEVPATH=`egrep '^DEVPATH=.*$' $1 | sed -e 's/DEVPATH= *//g'`
+	OLD_DEVNAME=`egrep '^DEVNAME=.*$' $1 | sed -e 's/DEVNAME= *//g'`
 	FCTEMPS=`egrep '^FCTEMPS=.*$' $1 | sed -e 's/FCTEMPS= *//g'`
 	FCFANS=`egrep '^FCFANS=.*$' $1 | sed -e 's/FCFANS= *//g'`
 	MINTEMP=`egrep '^MINTEMP=.*$' $1 | sed -e 's/MINTEMP= *//g'`
@@ -585,16 +640,12 @@
 	MAXPWM=`egrep '^MAXPWM=.*$' $1 | sed -e 's/MAXPWM= *//g'`
 
 	# Check for configuration change
-	local item
-	for item in $FCFANS
-	do
-		if [ ! -f "`echo $item | sed -e 's/=.*$//'`" ]
-		then
-			echo "Configuration appears to be outdated, discarded"
-			ClearConfig
-			return 0
-		fi
-	done
+	if ! ValidateDevices "$OLD_DEVPATH" "$OLD_DEVNAME"
+	then
+		echo "Configuration appears to be outdated, discarded"
+		ClearConfig
+		return 0
+	fi
 }
 
 LoadConfig $FCCONFIG
@@ -675,14 +726,68 @@
 	echo "OK, using $fanval"
 }
 
+# Remember the path and name of each device with at least one
+# reference (pwm, temp or fan) in the configuration file.
+# This function sets globals DEVPATH and DEVNAME as a side effect.
+function RememberDevices()
+{
+	local used entry device name path tempfandev pwmdev
+	DEVPATH=""
+	DEVNAME=""
+
+	for device in $DEVICES
+	do
+		device=`echo "$device" | sed -e 's/\/.*$//'`
+
+		used=0
+		for entry in $1 $2
+		do
+			pwmdev=`echo "$entry" | sed -e 's/\/.*$//'`
+			tempfandev=`echo "$entry" | sed -e 's/^[^=]*=//' -e 's/\/.*$//'`
+
+			if [ "$device" = "$pwmdev" -o "$device" = "$tempfandev" ]
+			then
+				used=1
+			fi
+		done
+		if [ "$used" -eq 0 ]
+		then
+			continue
+		fi
+
+		# Record the device path and name. This lets the fancontrol
+		# script check that they didn't change. If they did, then the
+		# configuration file can no longer be trusted.
+		path=`DevicePath "$device"`
+		if [ -z "$DEVPATH" ]
+		then
+			DEVPATH="$device=$path"
+		else
+			DEVPATH="$DEVPATH $device=$path"
+		fi
+
+		name=`DeviceName "$device"`
+		if [ -z "$DEVNAME" ]
+		then
+			DEVNAME="$device=$name"
+		else
+			DEVNAME="$DEVNAME $device=$name"
+		fi
+	done
+}
+
 function SaveConfig()
 {
+	RememberDevices "$FCTEMPS" "$FCFANS"
+
 	echo
 	echo "Saving configuration to $FCCONFIG..."
 	tmpfile=`mktemp -t pwmcfg.XXXXXXXXXX` || { echo "$0: Cannot create temporary file" >&2; exit 1;  }
 	trap " [ -f \"$tmpfile\" ] && /bin/rm -f -- \"$tmpfile\"" 0 1 2 3 13 15
 	echo "# Configuration file generated by pwmconfig, changes will be lost" >$tmpfile
 	echo "INTERVAL=$INTERVAL" >>$tmpfile
+	echo "DEVPATH=$DEVPATH" >>$tmpfile
+	echo "DEVNAME=$DEVNAME" >>$tmpfile
 	echo "FCTEMPS=$FCTEMPS" >>$tmpfile
 	echo "FCFANS=$FCFANS" >>$tmpfile
 	echo "MINTEMP=$MINTEMP" >>$tmpfile
Index: prog/pwm/fancontrol.8
=================================--- prog/pwm/fancontrol.8	(révision 5765)
+++ prog/pwm/fancontrol.8	(copie de travail)
@@ -92,9 +92,11 @@
 setup I recommend using the \fBpwmconfig\fP script. Small changes can be made by
 editing the config file directly following the rules above.
 
-Upon starting, fancontrol will make sure that all referenced sysfs files
-do exist. If not, it will quit immediately, upon the assumption that the
-configuration file may be out-of-sync with the loaded kernel drivers.
+Upon starting, fancontrol will make sure that all referenced devices
+do exist and match what they were at configuration time, and that all
+referenced sysfs files do exist. If not, it will quit immediately, upon
+the assumption that the configuration file may be out-of-sync with the
+loaded kernel drivers.
 
 .SH THE ALGORITHM
 


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors