Received-SPF: pass (google.com: domain of linux-kernel+bounces-159401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: i1719461a:Fastmail
Date: Thu, 25 Apr 2024 18:54:03 -0600
From: Lyndon Sanche <lsanche@lyndeno.ca>
Subject: Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile
To: Armin Wolf <W_Armin@gmx.de>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>, Pali =?iso-8859-1?b?Um9o4XI=?=
	<pali@kernel.org>, Hans de Goede <hdegoede@redhat.com>,
	Ilpo =?iso-8859-1?q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
	Dell.Client.Kernel@dell.com
Message-Id: <3IYICS.H9INGKTE6BW73@lyndeno.ca>
In-Reply-To: <7a215986-4702-443a-9e1d-a2fb1172276a@gmx.de>
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
	<7a215986-4702-443a-9e1d-a2fb1172276a@gmx.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed


On Thu, Apr 25 2024 at 11:07:22 PM +02:00:00, Armin Wolf 
<W_Armin@gmx.de> wrote:
> Am 25.04.24 um 19:27 schrieb Lyndon Sanche:
>> 
>> +//         1  AAC mode enabled
>> +//
>> +//     If AAC Configuration Type is USTT mode specific (multiple 
>> bits may be set for this parameter),
> 
> Hi,
> 
> checkpatch reports: WARNING: line length of 101 exceeds 100 columns

I can wrap this last line.

> 
>> +//         Bit 0 AAC (Balanced)
>> +//         Bit 1 AAC (Cool Bottom
>> +//         Bit 2 AAC (Quiet)
>> +//         Bit 3 AAC (Performance)
>> +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) & 1)
>> +		return DELL_BALANCED;
>> +	else if ((state >> DELL_COOL_BOTTOM) & 1)
>> +		return DELL_COOL_BOTTOM;
>> +	else if ((state >> DELL_QUIET) & 1)
>> +		return DELL_QUIET;
>> +	else if ((state >> DELL_PERFORMANCE) & 1)
>> +		return DELL_PERFORMANCE;
>> +	else
>> +		return 0;
> 
> This would return DELL_BALANCED if no option is set. Please return an 
> appropriate error code.

Thanks for catching this, I will return a proper error code.

> 
>> +}
>> +
>> +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 = buffer.output[1] & 0xF;
>> +	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 = ((buffer.output[3] >> 8) & 0xFF);
>> +	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, (acc_mode << 8) | BIT(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)
>> +{
>> +	int ret;
>> +
>> +	switch (profile) {
>> +	case PLATFORM_PROFILE_BALANCED:
>> +		ret = thermal_set_mode(DELL_BALANCED);
>> +		break;
> 
> Maybe using "return thermal_set_mode()" would be better in this cases.

Good idea, I can clean up the code with this suggestion.

> 
>> +	case PLATFORM_PROFILE_PERFORMANCE:
>> +		ret = thermal_set_mode(DELL_PERFORMANCE);
>> +		break;
>> +	case PLATFORM_PROFILE_QUIET:
>> +		ret = thermal_set_mode(DELL_QUIET);
>> +		break;
>> +	case PLATFORM_PROFILE_COOL:
>> +		ret = thermal_set_mode(DELL_COOL_BOTTOM);
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int thermal_platform_profile_get(struct 
>> platform_profile_handler *pprof,
>> +					enum platform_profile_option *profile)
>> +{
>> +	switch (thermal_get_mode()) {
> 
> Please check if thermal_get_mode() returned an error code and return 
> it in this case.

Will add error checking.

>> +		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
>> +
>> +	platform_profile_register(thermal_handler);
>> +
>> +	return 0;
> 
> Please check the return value of platform_profile_register() and 
> return the error if this function fails,
> see commit fe0e04cf66a1 ("platform/surface: platform_profile: 
> Propagate error if profile registration fails")
> for an explanation.

Thank you for catching this. I forgot to handle the return value.

> 
>> +}
>> +
>> +void thermal_cleanup(void)
>> +{
>> +	platform_profile_remove();
>> +	kfree(thermal_handler);
>> +}
>> +
>>   static struct led_classdev mute_led_cdev = {
>>   	.name = "platform::mute",
>>   	.max_brightness = 1,
>> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
>>   		mute_led_registered = true;
>>   	}
>> 
>> +	// Do not fail module if thermal modes not supported,
>> +	// just skip
>> +	if (thermal_init() != 0)
>> +		pr_warn("Unable to setup platform_profile, skipping");
>> +
>>   	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>>   		return 0;
>> 
>> @@ -2344,6 +2563,7 @@ static void __exit dell_exit(void)
>>   		platform_device_unregister(platform_device);
>>   		platform_driver_unregister(&platform_driver);
>>   	}
>> +	thermal_cleanup();
> 
> Should only be called when thermal_init() was successful.

I do not believe it is incorrect to skip checking for this.

platform_profile_remove() does not return anything and does not panic 
when a profile is not currently registered. My understanding from 
reading the source is it handles the case of no profile gracefully. 
Please let me know if my understanding is incorrect however.

kfree does not care of the thermal handler is allocated or not. Please 
let me know if calling kfree on NULL pointers is poor form for this 
application.

Thank you for your feedback, I do appreciate it.

Lyndon