lkml.org 
[lkml]   [2009]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] Acer Aspire One Fan Control
Date
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


\
 
 \ /
  Last update: 2009-05-11 21:37    [W:0.163 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site