Re: [PATCH] platform/x86: thinkpad_acpi: Correct thermal sensor allocation

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/7/21 11:20 PM, Mark Pearson wrote:
> On recent Thinkpad platforms it was reported that temp sensor 11 was
> always incorrectly displaying 66C. It turns out the reason for this is
> that this location in EC RAM is not a temperature sensor but is the
> power supply ID (offset 0xC2).
> 
> Based on feedback from the Lenovo firmware team the EC RAM version can
> be determined and for the current version (3) only the 0x78 to 0x7F
> range is used for temp sensors. I don't have any details for earlier
> versions so I have left the implementation unaltered there.
> 
> Note - in this block only 0x78 and 0x79 are officially designated (CPU &
> GPU sensors). The use of the other locations in the block will vary from
> platform to platform; but the existing logic to detect a sensor presence
> holds.
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>

I've merged this, note I've added one small fixup to initialize ver to
0 when it is declared, also see a remark inline below.

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.


> ---
>  drivers/platform/x86/thinkpad_acpi.c | 31 ++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 07de21941..8fbe4d3d9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6260,6 +6260,7 @@ enum thermal_access_mode {
>  enum { /* TPACPI_THERMAL_TPEC_* */
>  	TP_EC_THERMAL_TMP0 = 0x78,	/* ACPI EC regs TMP 0..7 */
>  	TP_EC_THERMAL_TMP8 = 0xC0,	/* ACPI EC regs TMP 8..15 */
> +	TP_EC_FUNCREV      = 0xEF,      /* ACPI EC Functional revision */
>  	TP_EC_THERMAL_TMP_NA = -128,	/* ACPI EC sensor not available */
>  
>  	TPACPI_THERMAL_SENSOR_NA = -128000, /* Sensor not available */
> @@ -6458,7 +6459,7 @@ static const struct attribute_group thermal_temp_input8_group = {
>  
>  static int __init thermal_init(struct ibm_init_struct *iibm)
>  {
> -	u8 t, ta1, ta2;
> +	u8 t, ta1, ta2, ver;
>  	int i;
>  	int acpi_tmp7;
>  	int res;
> @@ -6473,7 +6474,14 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>  		 * 0x78-0x7F, 0xC0-0xC7.  Registers return 0x00 for
>  		 * non-implemented, thermal sensors return 0x80 when
>  		 * not available
> +		 * The above rule is unfortunately flawed. This has been seen with
> +		 * 0xC2 (power supply ID) causing thermal control problems.
> +		 * The EC version can be determined by offset 0xEF and at least for
> +		 * version 3 the Lenovo firmware team confirmed that registers 0xC0-0xC7
> +		 * are not thermal registers.
>  		 */
> +		if (!acpi_ec_read(TP_EC_FUNCREV, &ver))
> +			pr_warn("Thinkpad ACPI EC unable to access EC version\n");

So what happens with ver in case the pr_warn here triggers?

As said above I've added a small change to initialize ver to 0 when declared,
so that we keep the old behavior when this pr_warn triggers.

>  
>  		ta1 = ta2 = 0;
>  		for (i = 0; i < 8; i++) {
> @@ -6483,11 +6491,13 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>  				ta1 = 0;
>  				break;
>  			}
> -			if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) {
> -				ta2 |= t;
> -			} else {
> -				ta1 = 0;
> -				break;
> +			if (ver < 3) {
> +				if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) {
> +					ta2 |= t;
> +				} else {
> +					ta1 = 0;
> +					break;
> +				}
>  			}
>  		}
>  		if (ta1 == 0) {
> @@ -6500,9 +6510,12 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>  				thermal_read_mode = TPACPI_THERMAL_NONE;
>  			}
>  		} else {
> -			thermal_read_mode =
> -			    (ta2 != 0) ?
> -			    TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
> +			if (ver >= 3)
> +				thermal_read_mode = TPACPI_THERMAL_TPEC_8;
> +			else
> +				thermal_read_mode =
> +					(ta2 != 0) ?
> +					TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
>  		}
>  	} else if (acpi_tmp7) {
>  		if (tpacpi_is_ibm() &&
> 

Regards,

Hans