lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <4f1a853a-7903-40a7-96a1-f5a00056fa31@app.fastmail.com>
Date: Thu, 30 Jan 2025 11:35:14 -0500
From: "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To: "Hans de Goede" <hdegoede@...hat.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 "platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Support for V9 DYTC platform profiles

Hi Hans,

Thanks for the review.

On Thu, Jan 30, 2025, at 11:25 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 30-Jan-25 4:45 PM, Mark Pearson wrote:
>> Newer Thinkpad AMD platforms are using V9 DYTC and this changes the
>> profiles used for PSC mode. Add support for this update.
>> Tested on P14s G5 AMD
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 33 ++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 1fcb0f99695a..cae457bc0b07 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -10319,6 +10319,10 @@ static struct ibm_struct proxsensor_driver_data = {
>>  #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
>>  #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
>>  
>> +#define DYTC_MODE_PSCV9_LOWPOWER 1  /* Low power mode */
>> +#define DYTC_MODE_PSCV9_BALANCE  3  /* Default mode aka balanced */
>> +#define DYTC_MODE_PSCV9_PERFORM  4  /* High power mode aka performance */
>> +
>>  #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
>>  #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
>>  
>> @@ -10339,6 +10343,10 @@ static int dytc_capabilities;
>>  static bool dytc_mmc_get_available;
>>  static int profile_force;
>>  
>> +static int platform_psc_profile_lowpower = DYTC_MODE_PSC_LOWPOWER;
>> +static int platform_psc_profile_balanced = DYTC_MODE_PSC_BALANCE;
>> +static int platform_psc_profile_performance = DYTC_MODE_PSC_PERFORM;
>> +
>>  static int convert_dytc_to_profile(int funcmode, int dytcmode,
>>  		enum platform_profile_option *profile)
>>  {
>> @@ -10360,19 +10368,14 @@ static int convert_dytc_to_profile(int funcmode, int dytcmode,
>>  		}
>>  		return 0;
>>  	case DYTC_FUNCTION_PSC:
>> -		switch (dytcmode) {
>> -		case DYTC_MODE_PSC_LOWPOWER:
>> +		if (dytcmode == platform_psc_profile_lowpower)
>>  			*profile = PLATFORM_PROFILE_LOW_POWER;
>> -			break;
>> -		case DYTC_MODE_PSC_BALANCE:
>> +		else if (dytcmode == platform_psc_profile_balanced)
>>  			*profile =  PLATFORM_PROFILE_BALANCED;
>> -			break;
>> -		case DYTC_MODE_PSC_PERFORM:
>> +		else if (dytcmode == platform_psc_profile_performance)
>>  			*profile =  PLATFORM_PROFILE_PERFORMANCE;
>> -			break;
>> -		default: /* Unknown mode */
>> +		else
>>  			return -EINVAL;
>> -		}
>>  		return 0;
>
> Maybe replace the removed '}' with an empty line instead of
> removing the entire line?
>
> Currently after your patch the new code looks like this:
>
> 			...
> 		else
> 			return -EINVAL;
> 		return 0;
>
> which look a bit weird, personally I would prefer:
>
> 			...
> 		else
> 			return -EINVAL;
>
> 		return 0;

Agreed - it does look oddly ugly doesn't it.

I'll wait and see if there is any other feedback, and then make that change for v2

>
> Otherwise this looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@...hat.com>
>
Thanks
Mark

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ