[drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Roel Kluin]

in drivers/misc/thinkpad_acpi.c: 4137-4142 it reads:

                /* safety net should the EC not support AUTO
                 * or FULLSPEED mode bits and just ignore them */
                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 7;     /* safety min speed 7 */
                else if (level & TP_EC_FAN_FULLSPEED)
                        level |= 4;     /* safety min speed 4 */

Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
this be replaced by

                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 7;     /* safety min speed 7 */
                else
                        level |= 4;     /* safety min speed 4 */

or 

                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 7;     /* safety min speed 7 */
                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 4;     /* safety min speed 4 */

?

Re: [drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Roland Dreier]

 >                 /* safety net should the EC not support AUTO
 >                  * or FULLSPEED mode bits and just ignore them */
 >                 if (level & TP_EC_FAN_FULLSPEED)
 >                         level |= 7;     /* safety min speed 7 */
 >                 else if (level & TP_EC_FAN_FULLSPEED)
 >                         level |= 4;     /* safety min speed 4 */
 > 
 > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
 > this be replaced by

Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
(based on the comment).

[PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Roel Kluin]

Roland Dreier wrote:
>  >                 /* safety net should the EC not support AUTO
>  >                  * or FULLSPEED mode bits and just ignore them */
>  >                 if (level & TP_EC_FAN_FULLSPEED)
>  >                         level |= 7;     /* safety min speed 7 */
>  >                 else if (level & TP_EC_FAN_FULLSPEED)
>  >                         level |= 4;     /* safety min speed 4 */
>  > 
>  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
>  > this be replaced by
> 
> Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
> (based on the comment).
> 

Thanks Roland, for your info

based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad,
I think this should be modified like below:

ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode
The Linux ThinkPad community is not positive that all ThinkPads that do
HFSP EC fan control do implement full-speed and auto modes, some of the
earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the
lower three bits that set the fan level. And as thinkpad-acpi was leaving
these set to zero, it would stop(!) the fan, which is Not A Good Thing.
So, as a safety net, we now make sure to also set the fan level part of the
HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto
mode.
--
second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO


Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index cf56647..3c323fe 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
 
 		if (!acpi_ec_write(fan_status_offset, level))

Re: [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Henrique de Moraes Holschuh]

On Tue, 05 Feb 2008, Roel Kluin wrote:
> Roland Dreier wrote:
> >  >                 /* safety net should the EC not support AUTO
> >  >                  * or FULLSPEED mode bits and just ignore them */
> >  >                 if (level & TP_EC_FAN_FULLSPEED)
> >  >                         level |= 7;     /* safety min speed 7 */
> >  >                 else if (level & TP_EC_FAN_FULLSPEED)
> >  >                         level |= 4;     /* safety min speed 4 */
> >  > 
> >  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
> >  > this be replaced by
> > 
> > Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
> > (based on the comment).
> 
> Thanks Roland, for your info
> 
> based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad,
> I think this should be modified like below:
> 
> ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode
> The Linux ThinkPad community is not positive that all ThinkPads that do
> HFSP EC fan control do implement full-speed and auto modes, some of the
> earlier ones supporting HFSP might not.
> 
> If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the
> lower three bits that set the fan level. And as thinkpad-acpi was leaving
> these set to zero, it would stop(!) the fan, which is Not A Good Thing.
> So, as a safety net, we now make sure to also set the fan level part of the
> HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto
> mode.
> --
> second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO
> 
> 
> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
> ---
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index cf56647..3c323fe 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
>  		 * or FULLSPEED mode bits and just ignore them */
>  		if (level & TP_EC_FAN_FULLSPEED)
>  			level |= 7;	/* safety min speed 7 */
> -		else if (level & TP_EC_FAN_FULLSPEED)
> +		else if (level & TP_EC_FAN_AUTO)
>  			level |= 4;	/* safety min speed 4 */
>  
>  		if (!acpi_ec_write(fan_status_offset, level))

ACK.  This needs to be sent to stable as well.  I think both 2.6.22 and
2.6.23 need this patch.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Roel Kluin]

Henrique de Moraes Holschuh wrote:
> On Tue, 05 Feb 2008, Roel Kluin wrote:
>> Roland Dreier wrote:
>>>  >                 /* safety net should the EC not support AUTO
>>>  >                  * or FULLSPEED mode bits and just ignore them */
>>>  >                 if (level & TP_EC_FAN_FULLSPEED)
>>>  >                         level |= 7;     /* safety min speed 7 */
>>>  >                 else if (level & TP_EC_FAN_FULLSPEED)
>>>  >                         level |= 4;     /* safety min speed 4 */
>>>  > 
>>>  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
>>>  > this be replaced by
>>>
>>> Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
>>> (based on the comment).
>> Thanks Roland, for your info
>>
>> based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad,
>> I think this should be modified like below:
>>
>> ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode
>> The Linux ThinkPad community is not positive that all ThinkPads that do
>> HFSP EC fan control do implement full-speed and auto modes, some of the
>> earlier ones supporting HFSP might not.
>>
>> If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the
>> lower three bits that set the fan level. And as thinkpad-acpi was leaving
>> these set to zero, it would stop(!) the fan, which is Not A Good Thing.
>> So, as a safety net, we now make sure to also set the fan level part of the
>> HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto
>> mode.
>> --
>> second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO
>>
>>
>> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
>> ---
>> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
>> index cf56647..3c323fe 100644
>> --- a/drivers/misc/thinkpad_acpi.c
>> +++ b/drivers/misc/thinkpad_acpi.c
>> @@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
>>  		 * or FULLSPEED mode bits and just ignore them */
>>  		if (level & TP_EC_FAN_FULLSPEED)
>>  			level |= 7;	/* safety min speed 7 */
>> -		else if (level & TP_EC_FAN_FULLSPEED)
>> +		else if (level & TP_EC_FAN_AUTO)
>>  			level |= 4;	/* safety min speed 4 */
>>  
>>  		if (!acpi_ec_write(fan_status_offset, level))
> 
> ACK.  This needs to be sent to stable as well.  I think both 2.6.22 and
> 2.6.23 need this patch.
> 

CC stable@kernel.org

Re: [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Greg KH]

On Tue, Feb 05, 2008 at 09:34:54AM +0100, Roel Kluin wrote:
> Henrique de Moraes Holschuh wrote:
> > On Tue, 05 Feb 2008, Roel Kluin wrote:
> >> Roland Dreier wrote:
> >>>  >                 /* safety net should the EC not support AUTO
> >>>  >                  * or FULLSPEED mode bits and just ignore them */
> >>>  >                 if (level & TP_EC_FAN_FULLSPEED)
> >>>  >                         level |= 7;     /* safety min speed 7 */
> >>>  >                 else if (level & TP_EC_FAN_FULLSPEED)
> >>>  >                         level |= 4;     /* safety min speed 4 */
> >>>  > 
> >>>  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
> >>>  > this be replaced by
> >>>
> >>> Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
> >>> (based on the comment).
> >> Thanks Roland, for your info
> >>
> >> based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad,
> >> I think this should be modified like below:
> >>
> >> ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode
> >> The Linux ThinkPad community is not positive that all ThinkPads that do
> >> HFSP EC fan control do implement full-speed and auto modes, some of the
> >> earlier ones supporting HFSP might not.
> >>
> >> If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the
> >> lower three bits that set the fan level. And as thinkpad-acpi was leaving
> >> these set to zero, it would stop(!) the fan, which is Not A Good Thing.
> >> So, as a safety net, we now make sure to also set the fan level part of the
> >> HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto
> >> mode.
> >> --
> >> second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO
> >>
> >>
> >> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
> >> ---
> >> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> >> index cf56647..3c323fe 100644
> >> --- a/drivers/misc/thinkpad_acpi.c
> >> +++ b/drivers/misc/thinkpad_acpi.c
> >> @@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
> >>  		 * or FULLSPEED mode bits and just ignore them */
> >>  		if (level & TP_EC_FAN_FULLSPEED)
> >>  			level |= 7;	/* safety min speed 7 */
> >> -		else if (level & TP_EC_FAN_FULLSPEED)
> >> +		else if (level & TP_EC_FAN_AUTO)
> >>  			level |= 4;	/* safety min speed 4 */
> >>  
> >>  		if (!acpi_ec_write(fan_status_offset, level))
> > 
> > ACK.  This needs to be sent to stable as well.  I think both 2.6.22 and
> > 2.6.23 need this patch.
> > 
> 
> CC stable@kernel.org

Please send us a real copy of the patch, when it goes into Linus's tree,
so we know what to apply, and that it is safe to do so.

thanks,

greg k-h

Re: [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Roel Kluin]

Greg KH wrote:
> On Tue, Feb 05, 2008 at 09:34:54AM +0100, Roel Kluin wrote:
>> Henrique de Moraes Holschuh wrote:
>>> On Tue, 05 Feb 2008, Roel Kluin wrote:
>>>> Roland Dreier wrote:

>>>>>  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
>>>>>  > this be replaced by
>>>>>
>>>>> Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
>>>>> (based on the comment).
>>>> Thanks Roland, for your info

>>> ACK.  This needs to be sent to stable as well.  I think both 2.6.22 and
>>> 2.6.23 need this patch.
>>>
>> CC stable@kernel.org
> 
> Please send us a real copy of the patch, when it goes into Linus's tree,
> so we know what to apply, and that it is safe to do so.

It's a oneliner and the patch is from linus' tree.
---
in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, a safety net for TPEC
fan control mode was added. Quoting that patch:

"The Linux ThinkPad community is not positive that all ThinkPads that do
HFSP EC fan control do implement full-speed and auto modes, some of the
earlier ones supporting HFSP might not.

If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the
lower three bits that set the fan level. And as thinkpad-acpi was leaving
these set to zero, it would stop(!) the fan, which is Not A Good Thing.
So, as a safety net, we now make sure to also set the fan level part of the
HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto
mode."

However, in the section below the test for the FULL-SPEED bits was not added:
the AUTO bits were tested twice. This patch corrects this and ensures that
the fan level part of the  HFSP register is set to a minimum of speed 4 for
auto mode.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index cf56647..3c323fe 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
 		 * or FULLSPEED mode bits and just ignore them */
 		if (level & TP_EC_FAN_FULLSPEED)
 			level |= 7;	/* safety min speed 7 */
-		else if (level & TP_EC_FAN_FULLSPEED)
+		else if (level & TP_EC_FAN_AUTO)
 			level |= 4;	/* safety min speed 4 */
 
 		if (!acpi_ec_write(fan_status_offset, level))

Re: [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Henrique de Moraes Holschuh]

On Thu, 07 Feb 2008, Roel Kluin wrote:
> It's a oneliner and the patch is from linus' tree.

Roel, better to just let me and Len Brown handle it.  I will add your patch
to my thinkpad-acpi queue and send it to Len soon, along with other simple
patches that are pending.  Either I or Len will notify stable@k.o when it
hits mainline.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [stable] [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Greg KH]

On Thu, Feb 07, 2008 at 12:48:24AM +0100, Roel Kluin wrote:
> Greg KH wrote:
> > On Tue, Feb 05, 2008 at 09:34:54AM +0100, Roel Kluin wrote:
> >> Henrique de Moraes Holschuh wrote:
> >>> On Tue, 05 Feb 2008, Roel Kluin wrote:
> >>>> Roland Dreier wrote:
> 
> >>>>>  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
> >>>>>  > this be replaced by
> >>>>>
> >>>>> Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
> >>>>> (based on the comment).
> >>>> Thanks Roland, for your info
> 
> >>> ACK.  This needs to be sent to stable as well.  I think both 2.6.22 and
> >>> 2.6.23 need this patch.
> >>>
> >> CC stable@kernel.org
> > 
> > Please send us a real copy of the patch, when it goes into Linus's tree,
> > so we know what to apply, and that it is safe to do so.
> 
> It's a oneliner and the patch is from linus' tree.
> ---
> in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad, a safety net for TPEC
> fan control mode was added. Quoting that patch:

I don't see this patch in Linus's tree, I see the patch you are
patching, but not this one :)

Can you tell me the git id of the patch you wish to go into the stable
tree, and send the whole thing to us?

thanks,

greg k-h

Re: [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Len Brown]

Applied.

thanks,
-Len

On Monday 04 February 2008 18:24, Roel Kluin wrote:
> Roland Dreier wrote:
> >  >                 /* safety net should the EC not support AUTO
> >  >                  * or FULLSPEED mode bits and just ignore them */
> >  >                 if (level & TP_EC_FAN_FULLSPEED)
> >  >                         level |= 7;     /* safety min speed 7 */
> >  >                 else if (level & TP_EC_FAN_FULLSPEED)
> >  >                         level |= 4;     /* safety min speed 4 */
> >  > 
> >  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
> >  > this be replaced by
> > 
> > Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
> > (based on the comment).
> > 
> 
> Thanks Roland, for your info
> 
> based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad,
> I think this should be modified like below:
> 
> ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode
> The Linux ThinkPad community is not positive that all ThinkPads that do
> HFSP EC fan control do implement full-speed and auto modes, some of the
> earlier ones supporting HFSP might not.
> 
> If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the
> lower three bits that set the fan level. And as thinkpad-acpi was leaving
> these set to zero, it would stop(!) the fan, which is Not A Good Thing.
> So, as a safety net, we now make sure to also set the fan level part of the
> HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto
> mode.
> --
> second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO
> 
> 
> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
> ---
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index cf56647..3c323fe 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
>  		 * or FULLSPEED mode bits and just ignore them */
>  		if (level & TP_EC_FAN_FULLSPEED)
>  			level |= 7;	/* safety min speed 7 */
> -		else if (level & TP_EC_FAN_FULLSPEED)
> +		else if (level & TP_EC_FAN_AUTO)
>  			level |= 4;	/* safety min speed 4 */
>  
>  		if (!acpi_ec_write(fan_status_offset, level))
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [stable] [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level & TP_EC_FAN_FULLSPEED)

[Greg KH]

On Wed, Feb 06, 2008 at 11:18:50PM -0200, Henrique de Moraes Holschuh wrote:
> On Thu, 07 Feb 2008, Roel Kluin wrote:
> > It's a oneliner and the patch is from linus' tree.
> 
> Roel, better to just let me and Len Brown handle it.  I will add your patch
> to my thinkpad-acpi queue and send it to Len soon, along with other simple
> patches that are pending.  Either I or Len will notify stable@k.o when it
> hits mainline.

Sounds good, I've dropped it from my queue, so please resend it when it
hits Linus's tree to stable@kernel.org if you want it there.

thanks,

greg k-h

[PATCH][drivers/misc/thinkpad_acpi.c] duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'

[Roel kluin]

in drivers/misc/thinkpad_acpi.c, lines 4137-4142 it reads:

                /* safety net should the EC not support AUTO
                 * or FULLSPEED mode bits and just ignore them */
                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 7;     /* safety min speed 7 */
                else if (level & TP_EC_FAN_FULLSPEED)
                        level |= 4;     /* safety min speed 4 */

note the nonsense duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'.
should this be changed to:

                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 7;     /* safety min speed 7 */
                else
                        level |= 4;     /* safety min speed 4 */

or maybe

                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 7;     /* safety min speed 7 */
                if (level & TP_EC_FAN_FULLSPEED)
                        level |= 4;     /* safety min speed 4 */

?