[lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Lubos Lunak]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]


 Hello,

 if system running fancontrol is suspended and resumed, fancontrol will abort, 
as writing the pwm value will fail because of pwm control being set to 
automatic control after the resume (google for 'fancontrol suspend' for 
various discussions about this). The attached patch fixes the problem.

 I'm not subscribed, so please CC me on any possible replies.

-- 
 Lubos Lunak

[-- Attachment #2: l-resume.patch --]
[-- Type: text/x-diff, Size: 594 bytes --]

--- prog/pwm/fancontrol.sav	2014-04-01 20:06:29.000000000 +0200
+++ prog/pwm/fancontrol	2014-04-02 12:40:52.376746067 +0200
@@ -504,8 +504,14 @@ function UpdateFanSpeeds
 		echo $pwmval > $pwmo # write new value to pwm output
 		if [ $? -ne 0 ]
 		then
-			echo "Error writing PWM value to $DIR/$pwmo" >&2
-			restorefans 1
+			# Try enabling the fan again (may be reset e.g. after a suspend/resume).
+			pwmenable $pwmo
+			echo $pwmval > $pwmo
+			if [ $? -ne 0 ]
+			then
+				echo "Error writing PWM value to $DIR/$pwmo" >&2
+				restorefans 1
+			fi
 		fi
 		if [ "$DEBUG" != "" ]
 		then

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Jean Delvare]

Hi Lubos,

On Wed, 2 Apr 2014 12:53:49 +0200, Lubos Lunak wrote:
>  if system running fancontrol is suspended and resumed, fancontrol will abort, 
> as writing the pwm value will fail because of pwm control being set to 
> automatic control after the resume (google for 'fancontrol suspend' for 
> various discussions about this). The attached patch fixes the problem.

I'm afraid this is fixing the problem in the wrong place. The BIOS is
responsible for restoring the hardware settings at resume time. Failing
that, the kernel should do it.

So you should first look for a BIOS update, even though the chances
that it fixes this specific problem are rather low. Then you should
tell us which hwmon driver you are using, so that we can add proper
suspend/resume code to it. If fan speed control mode settings are lost,
other settings may be lost as well and would need to be saved and
restored too.

That being said I have to admit that your approach, although not clean,
has the merit to work around the problem regardless of the BIOS or
hwmon chip in use. This is certainly attractive. My only worry is that
it _assumes_ that problem comes from an improperly handled
suspend/resume cycle, which may or may not be the case. It might as
well be the BIOS/ACPI changing the settings at run-time, or another fan
monitoring application running in parallel, or a driver bug. In which
case failing with an error is the right thing to do.

Guenter, do you have an opinion on this?

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Lubos Lunak]

On Thursday 03 of April 2014, Jean Delvare wrote:
> Hi Lubos,
>
> On Wed, 2 Apr 2014 12:53:49 +0200, Lubos Lunak wrote:
> >  if system running fancontrol is suspended and resumed, fancontrol will
> > abort, as writing the pwm value will fail because of pwm control being
> > set to automatic control after the resume (google for 'fancontrol
> > suspend' for various discussions about this). The attached patch fixes
> > the problem.
>
> I'm afraid this is fixing the problem in the wrong place. The BIOS is
> responsible for restoring the hardware settings at resume time. Failing
> that, the kernel should do it.
>
> So you should first look for a BIOS update, even though the chances
> that it fixes this specific problem are rather low. Then you should
> tell us which hwmon driver you are using, so that we can add proper
> suspend/resume code to it.

 hwmon_vid

> If fan speed control mode settings are lost, 
> other settings may be lost as well and would need to be saved and
> restored too.
>
> That being said I have to admit that your approach, although not clean,
> has the merit to work around the problem regardless of the BIOS or
> hwmon chip in use. This is certainly attractive. My only worry is that
> it _assumes_ that problem comes from an improperly handled
> suspend/resume cycle, which may or may not be the case.

 It doesn't assume, it handles the possibility. But it doesn't matter. If the 
write fails because of something else, it will presumably fail the second 
time as well.

 I can't see how this could realistically break any reasonable scenario. 
Fancontrol has already set up pwm control that way during start, why 
shouldn't it be allowed to do it again?

> It might as 
> well be the BIOS/ACPI changing the settings at run-time,

 In that case fancontrol would abort at any time this happens. Besides, that's 
rather against the idea of pwm being set to manual control, isn't it?

> or another fan monitoring application running in parallel,

 s/monitoring/controlling/ Well, that's certainly an interesting scenario that 
I expect is broken enough for nobody to be bothered enough to handle it.

> or a driver bug. In which 
> case failing with an error is the right thing to do.

 No. If the driver has a glitch that makes the write fail from time to time, 
it's still better if fancontrol copes with that. That is, again, if this 
scenario is actually worth thinking about.

-- 
 Lubos Lunak

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Guenter Roeck]

On 04/03/2014 07:31 AM, Lubos Lunak wrote:
> On Thursday 03 of April 2014, Jean Delvare wrote:
>> Hi Lubos,
>>
>> On Wed, 2 Apr 2014 12:53:49 +0200, Lubos Lunak wrote:
>>>   if system running fancontrol is suspended and resumed, fancontrol will
>>> abort, as writing the pwm value will fail because of pwm control being
>>> set to automatic control after the resume (google for 'fancontrol
>>> suspend' for various discussions about this). The attached patch fixes
>>> the problem.
>>
>> I'm afraid this is fixing the problem in the wrong place. The BIOS is
>> responsible for restoring the hardware settings at resume time. Failing
>> that, the kernel should do it.
>>
>> So you should first look for a BIOS update, even though the chances
>> that it fixes this specific problem are rather low. Then you should
>> tell us which hwmon driver you are using, so that we can add proper
>> suspend/resume code to it.
>
>   hwmon_vid
>
Whatever it is, it is not hwmon_vid, sorry. hwmon_vid is just a small module
which translates VID data into voltages.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Guenter Roeck]

On 04/03/2014 12:42 AM, Jean Delvare wrote:
> Hi Lubos,
>
> On Wed, 2 Apr 2014 12:53:49 +0200, Lubos Lunak wrote:
>>   if system running fancontrol is suspended and resumed, fancontrol will abort,
>> as writing the pwm value will fail because of pwm control being set to
>> automatic control after the resume (google for 'fancontrol suspend' for
>> various discussions about this). The attached patch fixes the problem.
>
> I'm afraid this is fixing the problem in the wrong place. The BIOS is
> responsible for restoring the hardware settings at resume time. Failing
> that, the kernel should do it.
>
> So you should first look for a BIOS update, even though the chances
> that it fixes this specific problem are rather low. Then you should
> tell us which hwmon driver you are using, so that we can add proper
> suspend/resume code to it. If fan speed control mode settings are lost,
> other settings may be lost as well and would need to be saved and
> restored too.
>
> That being said I have to admit that your approach, although not clean,
> has the merit to work around the problem regardless of the BIOS or
> hwmon chip in use. This is certainly attractive. My only worry is that
> it _assumes_ that problem comes from an improperly handled
> suspend/resume cycle, which may or may not be the case. It might as
> well be the BIOS/ACPI changing the settings at run-time, or another fan
> monitoring application running in parallel, or a driver bug. In which
> case failing with an error is the right thing to do.
>
> Guenter, do you have an opinion on this?
>
I am never a friend of patching over a problem. Yes, it is kind of
attractive, but on the downside the impact will be that we won't even
hear about such problems. This also solves only part of the problem -
if fan data is not restored, we have to assume that limits are not
restored either. And it won't solve the problem if automatic fan speed
control is used.

So my first choice would be to fix it in the driver.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Lubos Lunak]

On Thursday 03 of April 2014, Guenter Roeck wrote:
> On 04/03/2014 07:31 AM, Lubos Lunak wrote:
> > On Thursday 03 of April 2014, Jean Delvare wrote:
> >> Hi Lubos,
> >>
> >> On Wed, 2 Apr 2014 12:53:49 +0200, Lubos Lunak wrote:
> >>>   if system running fancontrol is suspended and resumed, fancontrol
> >>> will abort, as writing the pwm value will fail because of pwm control
> >>> being set to automatic control after the resume (google for 'fancontrol
> >>> suspend' for various discussions about this). The attached patch fixes
> >>> the problem.
> >>
> >> I'm afraid this is fixing the problem in the wrong place. The BIOS is
> >> responsible for restoring the hardware settings at resume time. Failing
> >> that, the kernel should do it.
> >>
> >> So you should first look for a BIOS update, even though the chances
> >> that it fixes this specific problem are rather low. Then you should
> >> tell us which hwmon driver you are using, so that we can add proper
> >> suspend/resume code to it.
> >
> >   hwmon_vid
>
> Whatever it is, it is not hwmon_vid, sorry. hwmon_vid is just a small
> module which translates VID data into voltages.

 it87 then?

-- 
 Lubos Lunak

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Guenter Roeck]

On 04/03/2014 08:25 AM, Lubos Lunak wrote:
> On Thursday 03 of April 2014, Guenter Roeck wrote:
>> On 04/03/2014 07:31 AM, Lubos Lunak wrote:
>>> On Thursday 03 of April 2014, Jean Delvare wrote:
>>>> Hi Lubos,
>>>>
>>>> On Wed, 2 Apr 2014 12:53:49 +0200, Lubos Lunak wrote:
>>>>>    if system running fancontrol is suspended and resumed, fancontrol
>>>>> will abort, as writing the pwm value will fail because of pwm control
>>>>> being set to automatic control after the resume (google for 'fancontrol
>>>>> suspend' for various discussions about this). The attached patch fixes
>>>>> the problem.
>>>>
>>>> I'm afraid this is fixing the problem in the wrong place. The BIOS is
>>>> responsible for restoring the hardware settings at resume time. Failing
>>>> that, the kernel should do it.
>>>>
>>>> So you should first look for a BIOS update, even though the chances
>>>> that it fixes this specific problem are rather low. Then you should
>>>> tell us which hwmon driver you are using, so that we can add proper
>>>> suspend/resume code to it.
>>>
>>>    hwmon_vid
>>
>> Whatever it is, it is not hwmon_vid, sorry. hwmon_vid is just a small
>> module which translates VID data into voltages.
>
>   it87 then?
>
Yes, that would be it, if you have that module loaded. The driver does not
currently implement suspend/resume, so the problem is not surprising if your
configuration changes default settings (which appears to be the case).

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] fancontrol aborts after suspend/resume

[Jean Delvare]

On Thu, 3 Apr 2014 16:31:15 +0200, Lubos Lunak wrote:
> On Thursday 03 of April 2014, Jean Delvare wrote:
> > That being said I have to admit that your approach, although not clean,
> > has the merit to work around the problem regardless of the BIOS or
> > hwmon chip in use. This is certainly attractive. My only worry is that
> > it _assumes_ that problem comes from an improperly handled
> > suspend/resume cycle, which may or may not be the case.

On second thought: your proposed patch wouldn't work on all chips
anyway. A number of hwmon drivers accept pre-programming the manual fan
speed control value before actually switching to manual mode. In that
case, you won't get a write failure so you won't be able to detect the
problem.

>  It doesn't assume, it handles the possibility. But it doesn't matter. If the 
> write fails because of something else, it will presumably fail the second 
> time as well.

Not necessarily. Setting manual fan control mode might be sufficient
for the second write to succeed, because they happen in a short sequence.

>  I can't see how this could realistically break any reasonable scenario. 
> Fancontrol has already set up pwm control that way during start, why 
> shouldn't it be allowed to do it again?

Because it shouldn't have to, so if it has to, that means something is
wrong, and the user should know about it.

> > It might as 
> > well be the BIOS/ACPI changing the settings at run-time,
> 
>  In that case fancontrol would abort at any time this happens.

Yes it would, before your change. After your change, no longer. This is
the problem I am underlining.

> Besides, that's 
> rather against the idea of pwm being set to manual control, isn't it?

Again, that's what I am trying to explain. If the user's view of how
the fan should be controlled differs from the BIOS/ACPI's view, it
should be reported, not ignored.

> > or another fan monitoring application running in parallel,
> 
>  s/monitoring/controlling/ Well, that's certainly an interesting scenario that 
> I expect is broken enough for nobody to be bothered enough to handle it.

Yes, I meant fan controlling application, sorry. Yes, that would be a broken
scenario, which again needs to be reported to the user. A plain error
message is better than letting fancontrol run and misbehave in ways the
user can't explain.

> > or a driver bug. In which 
> > case failing with an error is the right thing to do.
> 
>  No. If the driver has a glitch that makes the write fail from time to time, 
> it's still better if fancontrol copes with that. That is, again, if this 
> scenario is actually worth thinking about.

I was thinking of a bug where changing another setting would
accidentally overwrite the fan control mode registers. We've seen this
before.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors