Re: [PATCH] platform/x86: thinkpad_acpi: check dytc version for lapmode sysfs

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

Hi,

On 3/11/21 6:48 PM, Mark Pearson wrote:
> Lenovo platforms with DYTC versions earlier than version 5 don't set
> the lapmode interface correctly, causing issues with thermald on
> older platforms.
> 
> Add checking to only create the dytc_lapmode interface for version
> 5 and later.
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>

I've added a:

Fixes: 1ac09656bded ("platform/x86: thinkpad_acpi: Add palm sensor support")

Tag, this will help with the patch automatically getting selected for
stable kernel-series which contain the patch pointed to by the Fixes: tag.

In this case this won't work since this patch seems to depend on code
introduced in:

commit c3bfcd4c676238 ("platform/x86: thinkpad_acpi: lap or desk mode interface")

So you will need to manually backport this and submit it to stable@vger.kernel.org
if you want it to be included in any of the stable kernel series.

Still it is always good to have the Fixes: tag present when a commit is actually
fixing a previous commit.

###

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.

Regards,

Hans





> ---
>  drivers/platform/x86/thinkpad_acpi.c | 91 ++++++++++++++++++++--------
>  1 file changed, 65 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index b881044b31b0..f7de90a47e28 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9845,6 +9845,11 @@ static struct ibm_struct lcdshadow_driver_data = {
>   * Thinkpad sensor interfaces
>   */
>  
> +#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
> +#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
> +#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> +#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
> +
>  #define DYTC_CMD_GET          2 /* To get current IC function and mode */
>  #define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
>  
> @@ -9855,6 +9860,7 @@ static bool has_palmsensor;
>  static bool has_lapsensor;
>  static bool palm_state;
>  static bool lap_state;
> +static int dytc_version;
>  
>  static int dytc_command(int command, int *output)
>  {
> @@ -9869,6 +9875,33 @@ static int dytc_command(int command, int *output)
>  	return 0;
>  }
>  
> +static int dytc_get_version(void)
> +{
> +	int err, output;
> +
> +	/* Check if we've been called before - and just return cached value */
> +	if (dytc_version)
> +		return dytc_version;
> +
> +	/* Otherwise query DYTC and extract version information */
> +	err = dytc_command(DYTC_CMD_QUERY, &output);
> +	/*
> +	 * If support isn't available (ENODEV) then don't return an error
> +	 * and don't create the sysfs group
> +	 */
> +	if (err == -ENODEV)
> +		return 0;
> +	/* For all other errors we can flag the failure */
> +	if (err)
> +		return err;
> +
> +	/* Check DYTC is enabled and supports mode setting */
> +	if (output & BIT(DYTC_QUERY_ENABLE_BIT))
> +		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
> +
> +	return 0;
> +}
> +
>  static int lapsensor_get(bool *present, bool *state)
>  {
>  	int output, err;
> @@ -9974,7 +10007,18 @@ static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm)
>  		if (err)
>  			return err;
>  	}
> -	if (has_lapsensor) {
> +
> +	/* Check if we know the DYTC version, if we don't then get it */
> +	if (!dytc_version) {
> +		err = dytc_get_version();
> +		if (err)
> +			return err;
> +	}
> +	/*
> +	 * Platforms before DYTC version 5 claim to have a lap sensor, but it doesn't work, so we
> +	 * ignore them
> +	 */
> +	if (has_lapsensor && (dytc_version >= 5)) {
>  		err = sysfs_create_file(&tpacpi_pdev->dev.kobj, &dev_attr_dytc_lapmode.attr);
>  		if (err)
>  			return err;
> @@ -9999,14 +10043,9 @@ static struct ibm_struct proxsensor_driver_data = {
>   * DYTC Platform Profile interface
>   */
>  
> -#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
>  #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
>  #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
>  
> -#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
> -#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> -#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
> -
>  #define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
>  #define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
>  
> @@ -10211,28 +10250,28 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>  	if (err)
>  		return err;
>  
> +	/* Check if we know the DYTC version, if we don't then get it */
> +	if (!dytc_version) {
> +		err = dytc_get_version();
> +		if (err)
> +			return err;
> +	}
>  	/* Check DYTC is enabled and supports mode setting */
> -	if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
> -		/* Only DYTC v5.0 and later has this feature. */
> -		int dytc_version;
> -
> -		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
> -		if (dytc_version >= 5) {
> -			dbg_printk(TPACPI_DBG_INIT,
> -				   "DYTC version %d: thermal mode available\n", dytc_version);
> -			/* Create platform_profile structure and register */
> -			err = platform_profile_register(&dytc_profile);
> -			/*
> -			 * If for some reason platform_profiles aren't enabled
> -			 * don't quit terminally.
> -			 */
> -			if (err)
> -				return 0;
> +	if (dytc_version >= 5) {
> +		dbg_printk(TPACPI_DBG_INIT,
> +				"DYTC version %d: thermal mode available\n", dytc_version);
> +		/* Create platform_profile structure and register */
> +		err = platform_profile_register(&dytc_profile);
> +		/*
> +		 * If for some reason platform_profiles aren't enabled
> +		 * don't quit terminally.
> +		 */
> +		if (err)
> +			return 0;
>  
> -			dytc_profile_available = true;
> -			/* Ensure initial values are correct */
> -			dytc_profile_refresh();
> -		}
> +		dytc_profile_available = true;
> +		/* Ensure initial values are correct */
> +		dytc_profile_refresh();
>  	}
>  	return 0;
>  }
>