Received-SPF: pass (google.com: domain of linux-kernel+bounces-162805-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <bcd4fc9f-8900-46bc-9577-d95fa67adc25@amd.com>
Date: Mon, 29 Apr 2024 12:51:31 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
From: Mario Limonciello <mario.limonciello@amd.com>
To: Lyndon Sanche <lsanche@lyndeno.ca>
Cc: pali@kernel.org, W_Armin@gmx.de, srinivas.pandruvada@linux.intel.com,
 ilpo.jarvinen@linux.intel.com, Matthew Garrett <mjg59@srcf.ucam.org>,
 Hans de Goede <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org,
 linux-kernel@vger.kernel.org, Dell.Client.Kernel@dell.com
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
 <20240429164844.7544-2-lsanche@lyndeno.ca>
 <5cbb8981-5e24-4dce-a78f-1cabc29f08e3@amd.com>
Content-Language: en-US
In-Reply-To: <5cbb8981-5e24-4dce-a78f-1cabc29f08e3@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?M0IrUnBLajRWZTVkRHdISE9ocDZkTkt6a2R2ZVhaMDNpVFhBOXQ1bWY4T0k1?=
 =?utf-8?B?Uk04K3RYR25YclpzdGwyamtqcGpoZzRqU2x0elRMcWoybVFob2NFeFdhRVBl?=
 =?utf-8?B?dCttSU5OUHkySDU1elgwUjl4NFdobnFqUFpLYVB1NGZNRkZMbkN6SmpWNnhm?=
 =?utf-8?B?ZWhvb0szOW1oL2x5UTFCakg5NGVaVnZhUmJQMTZaelR4RlU3ak5KQlowVlZw?=
 =?utf-8?B?VjExVXFmaUZOSnJjdDlqcElRM3JDMlhNbDVGZU9sbnNueUl4c3NCMzhmRENv?=
 =?utf-8?B?b3RNZTRULy9GdHJyWnd1dkxWdzhWMjlMUy8xbEwwUk1IYnMycCtoVkltMmRt?=
 =?utf-8?B?ek5wcXZLcHZpZUQwYUVmV3NBZ3doaFRvM0dDS1FlWWM2bjlYSlliSDh1Y2JJ?=
 =?utf-8?B?SkNtaU03N2hRMGxJbFdDR1FxRldhZnFKOWZvN1NRaXoyc0MwREZOaHBnS1Zh?=
 =?utf-8?B?eGJZdGFSQ1l2b0NNOUkrdmU4ano3NmlYMzFIbTBoeEVubnI2RC9SenJJQkFG?=
 =?utf-8?B?ajM0NkkxaGhiRHhiemdTVWVCWnhvK0RtS3dNcEdIYkN4VFZkcm5yUXJ2Qlh6?=
 =?utf-8?B?Y2tVYWNFSXY2ZXg5RVZONGR3UElrWEl3Wm1iUHIyb09nTm1GZjBicGhlMnp2?=
 =?utf-8?B?eTFyZTVYK3V6STRUcDVJZjVEVHRCRGVMdndVWGl1bFZHSDNSajhJTFZMNUc2?=
 =?utf-8?B?QW9jMEg3VklVaVdPdzkzQ28xNFU0dTR0R003dkhKNm1pN252bDVEWUJOMU40?=
 =?utf-8?B?YzNIVjhPVk5va1Q5Mk5WZFRxakowOGxWaFc3bTQ3eDdaVyt5Unc3bWljeTRU?=
 =?utf-8?B?amdLMlU5bjZleU00RVFZVFgzUUxjdVYrRG5RWWlrODlOcWdwSHc5QmxMSXkx?=
 =?utf-8?B?L1lZVVFqSURTTFpCM3oyM1Jod3hnejJKcjVrT0gzV01FcVhYREFBZ0RYbGNu?=
 =?utf-8?B?VGhSWlZ2eE1YcjJyUWtIbUlraGtuNE9GWTFnWHlqUW5tTnJ6WnE5UVUxclQ1?=
 =?utf-8?B?Q1dQNkFheFIvRTdhUFFGZ3llMlNUMXBqTkNRYmhtR3JrcDFlOEgwcTFpVEpD?=
 =?utf-8?B?Ujhnd1MyT2VhNk4rdU5EcFdDMzhTdkhZWURJUGxHZE8zTHB2YXdqRnR0SjM0?=
 =?utf-8?B?cWlBamRMc3Z5QXh3dVZLTmZsdU1ycFAvSmRpcmJQRHBWT1FUOWJROFkrQ1Js?=
 =?utf-8?B?b1FFRElOZXYxckhTcjNGNEdhU1pBMnB0REpOVmRKblJDcnNQamxPWHFORFh3?=
 =?utf-8?B?c3JrZ24zTUhwbUpNN2RkK0dwcVFHbEY4SkpuSXo0amRuUExVV2tORlRrMjla?=
 =?utf-8?B?WHhGRHFPczVLWWFkaERrVUpvbm5OZDVtaHhLTmxDMVRWdVdNaWtZS0lWUWdz?=
 =?utf-8?B?NGIwRjZ1ZTloa0FFWktsMmlFQ1hqbldzR0JyTXlPVXNWajcyNXJBdzMySUI0?=
 =?utf-8?B?QXppSFBwYUlBU1hMUStYK1Z0Smh4b202aENMTXVIWmF2UEJKb2xkN0Yzc3dS?=
 =?utf-8?B?NWhkZmtHazB4QWZBOXQrckt6K3BxMnVGblRrTCtJaVJIS3JFR2liT09jMGZC?=
 =?utf-8?B?bTZpMm9yYnVFbGdUakk0eEZNUUtuY1FlMjhZV0NBdGpvV210UUYrbFVSQ0V2?=
 =?utf-8?B?clAraytVNjlhZk9waFBKMTJucGY2MGtKVXd6QXZQbUorLzVmNmdKaFoveVUz?=
 =?utf-8?B?dVRRanVMSTJuejdkVEJla1I5Z1pMclVGd1NDbXVRQkdDcUpHdWxTV085UFZ6?=
 =?utf-8?B?cjN3WW5acXA0dDhlbVJVZUdEWUQxaVNESG5sUlpRcTFmb2s3WVZPbWNGNG5U?=
 =?utf-8?B?djRrby8xWWtpL0pHVWtzRnNqU2pVMmR1SzIvTlcwSHByWHMzK2ZVYlhaUGF0?=
 =?utf-8?B?bWNidGJsWXByUmpVYW50by8rV1FnbjcxcWlrckNHalpLLzVFRnNHM0d1SUUr?=
 =?utf-8?B?VnZMMDliT0F5SGcxZ1R4ZjVBYjh3MDNYc2o1SktyeStMNm5UNzNxd0VxYnZP?=
 =?utf-8?B?MVNaRkg3L1hONkUrUytRSVlCUXczT01ySlZiZm84R29hTXdxNnBEWVN5WU5Q?=
 =?utf-8?B?NlU1Vm1ITTd0VUZFWHJZRTJMWUovc0JGTzcyR2FSVnRqSGhqSXRPNEtCVEVH?=
 =?utf-8?Q?oHOJWM15BJtkyJri4ZXdWR2wt?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b70ac3cd-879c-45db-b985-08dc68750270
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 17:51:34.2196
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zzxp2+LuXJ0KI9NNvqgp9WGHrhBlKyKMADnSeDEHeiruA8xqKD6I4KaASsq7al/QtkdBkwU6o4+15mciGP1thA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB5606

On 4/29/2024 12:45, Mario Limonciello wrote:
> On 4/29/2024 11:48, Lyndon Sanche wrote:
>> Some Dell laptops support configuration of preset fan modes through
>> smbios tables.
>>
>> If the platform supports these fan modes, set up platform_profile to
>> change these modes. If not supported, skip enabling platform_profile.
>>
>> Signed-off-by: Lyndon Sanche <lsanche@lyndeno.ca>
>> ---
>> v3:
>>   - Convert smbios-thermal-ctl docs to multiline comment and wrap
>>   - Change thermal_mode_bits enum to directly be BIT() values
>>    - Convert related code to use this
>>   - Use FIELD_GET/PREP and GENNMASK for getting/setting thermal modes
>>    - Correct offset for getting current ACC mode, setting offset
>>         unchanged
>>   - Check if thermal_handler is allocated before freeing and
>>      unregistering platform_profile
>> v2:
>>   - Wrap smbios-thermal-ctl comment
>>   - Return proper error code when invalid state returned
>>   - Simplify platform_profile_get returns
>>   - Propogate ENOMEM error
>> ---
>>   drivers/platform/x86/dell/dell-laptop.c | 232 ++++++++++++++++++++++++
>>   drivers/platform/x86/dell/dell-smbios.h |   1 +
>>   2 files changed, 233 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell/dell-laptop.c 
>> b/drivers/platform/x86/dell/dell-laptop.c
>> index 42f7de2b4522..fa58e7751d06 100644
>> --- a/drivers/platform/x86/dell/dell-laptop.c
>> +++ b/drivers/platform/x86/dell/dell-laptop.c
>> @@ -27,6 +27,8 @@
>>   #include <linux/i8042.h>
>>   #include <linux/debugfs.h>
>>   #include <linux/seq_file.h>
>> +#include <linux/platform_profile.h>
>> +#include <linux/bitfield.h>
> 
> These should be inserted in alphabetical order.
> 
>>   #include <acpi/video.h>
>>   #include "dell-rbtn.h"
>>   #include "dell-smbios.h"
>> @@ -95,6 +97,7 @@ static struct backlight_device *dell_backlight_device;
>>   static struct rfkill *wifi_rfkill;
>>   static struct rfkill *bluetooth_rfkill;
>>   static struct rfkill *wwan_rfkill;
>> +static struct platform_profile_handler *thermal_handler;
>>   static bool force_rfkill;
>>   static bool micmute_led_registered;
>>   static bool mute_led_registered;
>> @@ -2199,6 +2202,227 @@ static int mute_led_set(struct led_classdev 
>> *led_cdev,
>>       return 0;
>>   }
>> +/* Derived from smbios-thermal-ctl
>> + *
>> + * cbClass 17
>> + * cbSelect 19
>> + * User Selectable Thermal Tables(USTT)
>> + * cbArg1 determines the function to be performed
>> + * cbArg1 0x0 = Get Thermal Information
>> + *  cbRES1         Standard return codes (0, -1, -2)
>> + *  cbRES2, byte 0  Bitmap of supported thermal modes. A mode is 
>> supported if
>> + *                  its bit is set to 1
>> + *     Bit 0 Balanced
>> + *     Bit 1 Cool Bottom
>> + *     Bit 2 Quiet
>> + *     Bit 3 Performance
>> + *  cbRES2, byte 1 Bitmap of supported Active Acoustic Controller 
>> (AAC) modes.
>> + *                 Each mode corresponds to the supported thermal 
>> modes in
>> + *                  byte 0. A mode is supported if its bit is set to 1.
>> + *     Bit 0 AAC (Balanced)
>> + *     Bit 1 AAC (Cool Bottom
>> + *     Bit 2 AAC (Quiet)
>> + *     Bit 3 AAC (Performance)
>> + *  cbRes3, byte 0 Current Thermal Mode
>> + *     Bit 0 Balanced
>> + *     Bit 1 Cool Bottom
>> + *     Bit 2 Quiet
>> + *     Bit 3 Performanc
>> + *  cbRes3, byte 1  AAC Configuration type
>> + *          0       Global (AAC enable/disable applies to all 
>> supported USTT modes)
>> + *          1       USTT mode specific
>> + *  cbRes3, byte 2  Current Active Acoustic Controller (AAC) Mode
>> + *     If AAC Configuration Type is Global,
>> + *          0       AAC mode disabled
>> + *          1       AAC mode enabled
>> + *     If AAC Configuration Type is USTT mode specific (multiple bits 
>> may be set),
>> + *          Bit 0 AAC (Balanced)
>> + *          Bit 1 AAC (Cool Bottom
>> + *          Bit 2 AAC (Quiet)
>> + *          Bit 3 AAC (Performance)
>> + *  cbRes3, byte 3  Current Fan Failure Mode
>> + *     Bit 0 Minimal Fan Failure (at least one fan has failed, one 
>> fan working)
>> + *     Bit 1 Catastrophic Fan Failure (all fans have failed)
>> + *  cbArg1 0x1   (Set Thermal Information), both desired thermal mode 
>> and
>> + *               desired AAC mode shall be applied
>> + *  cbArg2, byte 0  Desired Thermal Mode to set
>> + *                  (only one bit may be set for this parameter)
>> + *     Bit 0 Balanced
>> + *     Bit 1 Cool Bottom
>> + *     Bit 2 Quiet
>> + *     Bit 3 Performance
>> + *  cbArg2, byte 1  Desired Active Acoustic Controller (AAC) Mode to set
>> + *     If AAC Configuration Type is Global,
>> + *         0  AAC mode disabled
>> + *         1  AAC mode enabled
>> + *
>> + *     If AAC Configuration Type is USTT mode specific
>> + *     (multiple bits may be set for this parameter),
>> + *         Bit 0 AAC (Balanced)
>> + *         Bit 1 AAC (Cool Bottom
>> + *         Bit 2 AAC (Quiet)
>> + *         Bit 3 AAC (Performance)
>> + */
>> +
>> +#define DELL_ACC_GET_FIELD GENMASK(19, 16)
>> +#define DELL_ACC_SET_FIELD GENMASK(11, 8)
>> +#define DELL_THERMAL_SUPPORTED GENMASK(3, 0)
>> +
>> +enum thermal_mode_bits {
>> +    DELL_BALANCED = BIT(0),
>> +    DELL_COOL_BOTTOM = BIT(1),
>> +    DELL_QUIET = BIT(2),
>> +    DELL_PERFORMANCE = BIT(3),
>> +};
>> +
>> +static int thermal_get_mode(void)
>> +{
>> +    struct calling_interface_buffer buffer;
>> +    int state;
>> +    int ret;
>> +
>> +    dell_fill_request(&buffer, 0x0, 0, 0, 0);
>> +    ret = dell_send_request(&buffer, CLASS_INFO, 
>> SELECT_THERMAL_MANAGEMENT);
>> +    if (ret)
>> +        return ret;
>> +    state = buffer.output[2];
>> +    if (state & DELL_BALANCED)
>> +        return DELL_BALANCED;
>> +    else if (state & DELL_COOL_BOTTOM)
>> +        return DELL_COOL_BOTTOM;
>> +    else if (state & DELL_QUIET)
>> +        return DELL_QUIET;
>> +    else if (state & DELL_PERFORMANCE)
>> +        return DELL_PERFORMANCE;
>> +    else
>> +        return -ENXIO;
>> +}
>> +
>> +static int thermal_get_supported_modes(int *supported_bits)
>> +{
>> +    struct calling_interface_buffer buffer;
>> +    int ret;
>> +
>> +    dell_fill_request(&buffer, 0x0, 0, 0, 0);
>> +    ret = dell_send_request(&buffer, CLASS_INFO, 
>> SELECT_THERMAL_MANAGEMENT);
>> +    if (ret)
>> +        return ret;
>> +    *supported_bits = FIELD_GET(DELL_THERMAL_SUPPORTED, 
>> buffer.output[1]);
>> +    return 0;
>> +}
>> +
>> +static int thermal_get_acc_mode(int *acc_mode)
>> +{
>> +    struct calling_interface_buffer buffer;
>> +    int ret;
>> +
>> +    dell_fill_request(&buffer, 0x0, 0, 0, 0);
>> +    ret = dell_send_request(&buffer, CLASS_INFO, 
>> SELECT_THERMAL_MANAGEMENT);
>> +    if (ret)
>> +        return ret;
>> +    *acc_mode = FIELD_GET(DELL_ACC_GET_FIELD, buffer.output[3]);
>> +    return 0;
>> +}
>> +
>> +static int thermal_set_mode(enum thermal_mode_bits state)
>> +{
>> +    struct calling_interface_buffer buffer;
>> +    int ret;
>> +    int acc_mode;
>> +
>> +    ret = thermal_get_acc_mode(&acc_mode);
>> +    if (ret)
>> +        return ret;
>> +
>> +    dell_fill_request(&buffer, 0x1, FIELD_PREP(DELL_ACC_SET_FIELD, 
>> acc_mode) | state, 0, 0);
>> +    ret = dell_send_request(&buffer, CLASS_INFO, 
>> SELECT_THERMAL_MANAGEMENT);
>> +    return ret;
>> +}
>> +
>> +static int thermal_platform_profile_set(struct 
>> platform_profile_handler *pprof,
>> +                    enum platform_profile_option profile)
>> +{
>> +    switch (profile) {
>> +    case PLATFORM_PROFILE_BALANCED:
>> +        return thermal_set_mode(DELL_BALANCED);
>> +    case PLATFORM_PROFILE_PERFORMANCE:
>> +        return thermal_set_mode(DELL_PERFORMANCE);
>> +    case PLATFORM_PROFILE_QUIET:
>> +        return thermal_set_mode(DELL_QUIET);
>> +    case PLATFORM_PROFILE_COOL:
>> +        return thermal_set_mode(DELL_COOL_BOTTOM);
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +}
>> +
>> +static int thermal_platform_profile_get(struct 
>> platform_profile_handler *pprof,
>> +                    enum platform_profile_option *profile)
>> +{
>> +    int ret = thermal_get_mode();
>> +
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    switch (ret) {
>> +    case DELL_BALANCED:
>> +        *profile = PLATFORM_PROFILE_BALANCED;
>> +        break;
>> +    case DELL_PERFORMANCE:
>> +        *profile = PLATFORM_PROFILE_PERFORMANCE;
>> +        break;
>> +    case DELL_COOL_BOTTOM:
>> +        *profile = PLATFORM_PROFILE_COOL;
>> +        break;
>> +    case DELL_QUIET:
>> +        *profile = PLATFORM_PROFILE_QUIET;
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int thermal_init(void)
>> +{
>> +    int ret;
>> +    int supported_modes;
>> +
>> +    ret = thermal_get_supported_modes(&supported_modes);
>> +    if (ret || !supported_modes)
>> +        return 0;
>> +
>> +    thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
>> +    if (!thermal_handler)
>> +        return -ENOMEM;
>> +    thermal_handler->profile_get = thermal_platform_profile_get;
>> +    thermal_handler->profile_set = thermal_platform_profile_set;
>> +
>> +    if (supported_modes & DELL_QUIET)
>> +        set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
>> +    if (supported_modes & DELL_COOL_BOTTOM)
>> +        set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
>> +    if (supported_modes & DELL_BALANCED)
>> +        set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
>> +    if (supported_modes & DELL_PERFORMANCE)
>> +        set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
>> +
>> +    // Clean up but do not fail
> 
> Switch comment style to /* */
> 
>> +    if (platform_profile_register(thermal_handler))
>> +        kfree(thermal_handler);
> 
> Don't you also want to return an error in this case?  Because this means 
> that the platform supports thermal modes but it failed to setup due to 
> other issues.
> 
> It's different than the case of no supported modes in which case 
> returning 0 makes sense.
> 
> Maybe like this:
> 
> 
> ret = platform_profile_register(thermal_handler);
> if (ret)
>      kfree(thermal_handler);
> 
> return ret;
> 
> 
>> +
>> +    return 0;
>> +}
>> +
>> +void thermal_cleanup(void)
>> +{
>> +    if (thermal_handler) {
>> +        platform_profile_remove();
>> +        kfree(thermal_handler);
>> +    }
>> +}
>> +
>>   static struct led_classdev mute_led_cdev = {
>>       .name = "platform::mute",
>>       .max_brightness = 1,
>> @@ -2238,6 +2462,11 @@ static int __init dell_init(void)
>>           goto fail_rfkill;
>>       }
>> +    // Do not fail module if thermal modes not supported, just skip
> 
> Switch comment style to /* */
> 
>> +    ret = thermal_init();
>> +    if (ret)
>> +        goto fail_thermal;
>> +
>>       if (quirks && quirks->touchpad_led)
>>           touchpad_led_init(&platform_device->dev);
>> @@ -2317,6 +2546,8 @@ static int __init dell_init(void)
>>           led_classdev_unregister(&mute_led_cdev);
>>   fail_led:
>>       dell_cleanup_rfkill();
>> +fail_thermal:
>> +    thermal_cleanup();
>>   fail_rfkill:
>>       platform_device_del(platform_device);
>>   fail_platform_device2:
>> @@ -2344,6 +2575,7 @@ static void __exit dell_exit(void)
>>           platform_device_unregister(platform_device);
>>           platform_driver_unregister(&platform_driver);
>>       }
>> +    thermal_cleanup();
>>   }
>>   /* dell-rbtn.c driver export functions which will not work correctly 
>> (and could
>> diff --git a/drivers/platform/x86/dell/dell-smbios.h 
>> b/drivers/platform/x86/dell/dell-smbios.h
>> index eb341bf000c6..585d042f1779 100644
>> --- a/drivers/platform/x86/dell/dell-smbios.h
>> +++ b/drivers/platform/x86/dell/dell-smbios.h
>> @@ -19,6 +19,7 @@
>>   /* Classes and selects used only in kernel drivers */
>>   #define CLASS_KBD_BACKLIGHT 4
>>   #define SELECT_KBD_BACKLIGHT 11
>> +#define SELECT_THERMAL_MANAGEMENT 19

I think you should insert this into dell-smbios-base.c under 
call_blacklist.  This will prevent userspace from fighting with the 
kernel on the same data when this code goes in.

>>   /* Tokens used in kernel drivers, any of these
>>    * should be filtered from userspace access
>