[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cone.1245331895.30957.31537.1000@arca>
Date: Thu, 18 Jun 2009 15:31:35 +0200
From: Peter Feuerer <pfe@...e.net>
To: Borislav Petkov <petkovbb@...glemail.com>
Cc: Andreas Mohr <andi@...as.de>, Ed Tomlinson <edt@....ca>,
akpm@...ux-foundation.org, Len Brown <len.brown@...el.com>,
Matthew Garrett <mjg59@...f.ucam.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Request driver inclusion - acer aspire one fan control
Hi,
Borislav Petkov writes:
> Hi,
>
> On Thu, Jun 18, 2009 at 1:49 PM, Peter Feuerer<peter@...e.net> wrote:
>>>>>> Actually I think pre_suspend_kernelmode is needed, so it won't be
>>>>>> dropped.
>>>>>
>>>>> and it is needed, because...?
>>>>
>>>> It's needed because we do now a clean revert to bios mode before we
>>>> suspend.
>>>> And after resume we have to switch to kernelmode again, if the driver was
>>>> in
>>>> kernelmode before suspend. So we need to keep track of in what state the
>>>> driver was before suspending. That's what's this variable is for.
>>>
>>> You've got that state in the 'kernelmode' variable. See full comment:
>>> http://marc.info/?l=linux-kernel&m=124482114200865
>>
>> We are talking about patch 0.5.9 and not 0.5.8, are we?
>> http://patchwork.kernel.org/patch/30733/mbox/
>>
>> have a look at at line 543:
>> + /* remember previous setting */
>> + pre_suspend_kernelmode = kernelmode;
>> +
>> + if (kernelmode) {
>> + acerhdf_revert_to_bios_mode();
>> + if (acerhdf_thz_dev)
>> + thermal_zone_device_update(acerhdf_thz_dev);
>> + }
>
> ok, this starts to look quite a bit overengineered for no reason. First,
> acerhdf_revert_to_bios_mode() sets the fan to auto. Then, you've added
> a thermal_zone_device_update() call in there which does set the fan to
> auto indirectly _again_. And we end up with _three_ variables which
> represent only _one_ state. Here's what it should do:
You are partly right, setting the fan to auto in
"acerhdf_revert_to_bios_mode" can be removed, as this is done by the thermal
layer when calling "acerhdf_set_cur_state" with disable_kernelmode=1.
But besides that I think it is a clean way.
This way we _completely_ disable the thermal polling before going suspend
and ensure the thermal layer doesn't handle the fan anymore. When we resume
we let the thermal layer take over the fan again. In my opinion this is much
cleaner than just switching the fan to auto without keeping the polling of
the thermal layer in mind.
The big problem with the polling is, that you don't know when the next
thermal polling shot arrives. Is it after acerhdf_suspend was called, or
after system-resume but before acerhdf_resume was called, or after
acerhdf_resume was called… You can't know! That's why in my opinion
completely disabling our kernelmode is the only clean solution.
--peter
P.S. I built the official 2.6.30 release (because current git is freezing
all the time), patched it with acerhdf 0.5.9 and was testing suspend/resume
about 40 times now. Was always hitting the suspend button while working ;).
I wasn't able to reproduce the "unexpected fanstate" issue, <=0.5.8 had.
--
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