[PATCH 1/3] CPU frequency scaling addon

[Dave Jones]

On Mon, Aug 14, 2006 at 05:58:55PM +0200, Holger Macht wrote:

 > > Thanks, this is better. Will change at corresponding places.
 > 
 > Done. New patch attached.

+/** @brief set a speed with traversing all intermediary speeds */
+static int set_speed(struct userspace_interface *iface, int target_speed)
+{
+	int delta;
+	int current_speed = iface->current_speed;
+
+	if (current_speed == target_speed)
+		return -1;
+
+	if (current_speed > target_speed)
+		delta = -1;
+	else
+		delta = 1;
+
+	do {
+		current_speed += delta;
+		write_speed(g_a_i(iface->speeds_kHz, current_speed), iface->base_cpu);
+	} while (current_speed != target_speed);
+
+	return current_speed;
+}

This looks seriously nasty.  If we're running at 2GHz and have a
target of 800MHz, that's 1200 writes to sysfs we're going to do,
which is just insane.

The kernel exports the available steps for a reason, so that you
don't have to guess at speeds like this.


+/** writes the new_governor string into the sysfs interface */ 
+gboolean write_governor(char *new_governor, int cpu_id)
+{
+	gboolean	ret		= TRUE;
+	char		*governor_file  = NULL;
+	char		governor[MAX_LINE_SIZE + 1];
+
+	if (!cpu_online(cpu_id))
+		goto Out;
+
+	governor_file = g_strdup_printf(SYSFS_GOVERNOR_FILE, cpu_id); 
+	dbg("Trying ot write governor %s", new_governor);
+
+	if (!write_line(governor_file, "%s", new_governor)) {
+		ret = FALSE;
+		goto Out;
+	}
+	
+	/* check if governor has been set */
+	usleep(1000);
+	read_line(governor_file, governor, MAX_LINE_SIZE);
+	if (strstr(governor, new_governor))
+		ret = TRUE;
+	else
+		ret = FALSE;
+Out:
+	g_free(governor_file);
+	return ret;
+}

The usleep should be completely unnecessary.
A write to sysfs is atomic, we won't return to userspace
until the governor has been set.

Oh, and you're still including the /proc/cpuinfo definition, even
though nothing uses it now that you've switched over to using _SC_NR_PROCESSORS

		Dave

-- 
http://www.codemonkey.org.uk