linux-kernel - Re: [PATCH v3] platform/x86: dell-laptop: Implement platform

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 30 Apr 2024 12:38:35 -0600
From: "Lyndon Sanche" <lsanche@...deno.ca>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: mario.limonciello@....com, pali@...nel.org, W_Armin@....de,
 srinivas.pandruvada@...ux.intel.com, "Matthew Garrett" <mjg59@...f.ucam.org>,
 "Hans de Goede" <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>, Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile

On Tue, Apr 30, 2024, at 4:31 AM, Ilpo Järvinen wrote:
> On Mon, 29 Apr 2024, Lyndon Sanche wrote:
>> + */
>> +
>> +#define DELL_ACC_GET_FIELD GENMASK(19, 16)
>> +#define DELL_ACC_SET_FIELD GENMASK(11, 8)
>> +#define DELL_THERMAL_SUPPORTED GENMASK(3, 0)
>
> Please align these with tabs.
>

Agreed.

>> +enum thermal_mode_bits {
>> +	DELL_BALANCED = BIT(0),
>> +	DELL_COOL_BOTTOM = BIT(1),
>> +	DELL_QUIET = BIT(2),
>> +	DELL_PERFORMANCE = BIT(3),
>
> You need #include <linux/bits.h> for BIT().
>

Agreed.

>> +	}
>> +}
>> +
>> +static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
>> +					enum platform_profile_option *profile)
>> +{
>> +	int ret = thermal_get_mode();
>> +
>> +	if (ret < 0)
>
> I think I already mentioned about this, change to:
>
> 	int ret;
>
> 	ret = thermal_get_mode();
> 	if (ret < 0)
>

I missed this.

>> +		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;
>
> I think you should propagate the error code differently from nothing 
> supported:
>
> 	if (ret < 0)
> 		return ret;
> 	if (!supported_modes)
> 		return 0;
>

Agreed.

Thank you for your feedback.

Lyndon