linux-kernel - Re: [PATCH] Acer Aspire One Fan Control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 06 May 2009 21:41:14 +0200
From:	Peter Feuerer <peter@...e.net>
To:	petkovbb@...il.com
Cc:	LKML <linux-kernel@...r.kernel.org>, lenb@...nel.org,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Maxim Levitsky <maximlevitsky@...il.com>
Subject: Re: [PATCH] Acer Aspire One Fan Control

Hi Boris,

thank you very much! A modified patch with your suggestions will follow 
as soon as I tested it. Just some things I would like to discuss:

Borislav Petkov writes:
>> +/* change current fan state - is overwritten when running in kernel mode */
>> +static int set_cur_state(struct thermal_cooling_device *cdev,
>> +		unsigned long state)
>> +{
>> +	int old_state;
>> +
>> +	/* let the thermal layer disable kernelmode. This ensures that
>> +	 * the thermal layer doesn't switch off the fan again */
>> +	if (disable_kernelmode) {
>> +		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
>> +		disable_kernelmode = 0;
>> +		kernelmode = 0;
>> +		return 0;
>> +	}
>> +
>> +	if (!kernelmode) {
>> +		acerhdf_change_fanstate(state);
>> +		return 0;
>> +	}
> 
> why are we changing the fan state if kernelmode is off? If I'm not
> mistaken, the BIOS should be controlling the fan here.

The user can control the fan in userspace (by e.g. echoing 0 to the 
/sys/class/thermal.../cur_state file) then this function is called. This is 
useful if somebody wants to program an userspace tool to handle the 
temperature and the fan.

>> +	old_state = acerhdf_get_fanstate();
>> +
>> +	/* if reading the fan's state returns unexpected value, there's a
>> +	 * problem with the ec register. -> let the BIOS take control of
>> +	 * the fan to prevent hardware damage */
>> +	if (old_state != fanstate) {
> 
> you should be checking 
> 
> 	old_state == ACERHDF_ERROR
> 
> here instead.

Comparing to fanstate is better. It implys the ACERHDF_ERROR check, as 
fanstate can only be ACERHDF_FAN_AUTO or ACERHDF_FAN_OFF. And on the other 
Hand there will be thrown an error too, if the ec register contains an 
unexpected value. So it is more failsafe this way.

What do you think will be the next steps to get the kernel into mainline? 
Matthew said I should CC Len Brown, as he is responsible to include 
the module. But Len didn't write anything yet :-(

kind regards,
--peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/