Re: [PATCH] Acer Aspire One Fan Control

[Peter Feuerer <peter@piie.net>]

Hello,

Borislav Petkov writes:
> 
> the more I'm looking at the driver, the more I get annoyed by that
> user/kernel mode operation split. Remind me again why the driver should
> be loaded and not started automatically but the user should be required
> to activate it explicitly?

The idea of not starting the module in kernel mode was from Matthew. And he 
stated that it could harm the hardware when software controls the fan 
instead of the BIOS. It may also be possible, that the warranty gets invalid 
when you do that. Not sure about how acer would handle a defect which could 
be caused by overheating and when they detect that software was controlling 
the fan. That's why I think Matthew is right. 

> That's not so optimal, I'd say. The kernel module should _replace_
> the userspace program, not work alongside it, since the last is flaky
> and unreliable, and this was the main reason the kernel module was
> introduced in the first place - to control the fan from kernel space,
> which is the more sane choice.

The main reason to do this in kernel was the availabilty of atomic ec- read 
and write functions. But I agree with you that either kernel or BIOS 
should control the fan and not a userspace tool. I added the user mode just 
because it wasn't really much more code than just an implementation of the 
enable/disable functionality.

> What is more, if the userspace program would fail, there's no way for
> the module to get activated again and jump in instead of the userspace
> program to the rescue. Which goes more to show that you don't need
> userspace control _at_ _all_ and the only two agents controlling the fan
> should be the BIOS or the kernel module.

After reading and thinking about all this a while, I agree with you. In the 
next patch I won't allow the user to switch on/off the fan anymore.

> 
> So I think that the kernel module should take over fan control upon
> load. This way you'll be able to get rid of all that needless complexity
> around kernelmode/disable_kernelmode and have a simple clean design.

I would really like to do that, but what do you think about the hardware damage / warranty things written above?

>> +
>> +	if (!force_bios[0]) {
>> +		/* check if product is a AO - Aspire One */
>> +		if (strncmp(product, "AO", 2)) {
> 
> I suppose this check will have to be updated if Acer release newer
> Aspire One versions which product names are not AOXXXX anymore, right?
> But who knows, they might fix the fan in their A1 versions :)

This may really be updated if they release newer AO version. But the time 
being all supported Aspire One versions begin with AO.

>> +	/* search BIOS version and BIOS vendor in BIOS settings table */
>> +	for (i = 0; bios_settings[i].version[0]; ++i) {
>> +		if (!strcmp(bios_settings[i].vendor, vendor) &&
>> +				!strcmp(bios_settings[i].version, version)) {
>> +			bios_version = i;
>> +			break;
>> +		}
>> +	}
>> +	if (bios_version == -1) {
>> +		acerhdf_error("cannot find BIOS version\n");
>> +		ret_val = -ENODEV;
>> +		goto EXIT;
>> +	}
> 
> you could split this function since logically you do basic checking of hardware
> and below this line you register with the thermal core so the rest of that
> function could go into another one, something like:
> 
> static int acerhdf_register(int mode) {
> 
> 	...
> }
> 
> and then call it in the acerhdf_init() like that:
> 
> 	return acerhdf_register(kernelmode);

Good idea.

kind regards,
--peter