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]
Message-ID: <cone.1241638874.190271.13804.1000@onepiie>
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ