Received-SPF: pass (google.com: domain of linux-kernel+bounces-159141-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 25 Apr 2024 22:31:13 +0200
From: Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To: Lyndon Sanche <lsanche@lyndeno.ca>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Ilpo =?utf-8?B?SsOkcnZpbmVu?= <ilpo.jarvinen@linux.intel.com>,
	platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
	Dell.Client.Kernel@dell.com
Subject: Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile
Message-ID: <20240425203113.eokpgdjj6hmxhdj3@pali>
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
 <20240425201213.apwdxazbdbjddtpr@pali>
 <d31486b6-d71d-4985-adb1-2425f19b0b5c@app.fastmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d31486b6-d71d-4985-adb1-2425f19b0b5c@app.fastmail.com>
User-Agent: NeoMutt/20180716

On Thursday 25 April 2024 14:27:32 Lyndon Sanche wrote:
> On Thu, Apr 25, 2024, at 2:12 PM, Pali Rohár wrote:
> > On Thursday 25 April 2024 11:27:57 Lyndon Sanche wrote:
> >> +int thermal_init(void)
> >> +{
> >> +	int ret;
> >> +	int supported_modes;
> >> +
> >> +	ret = thermal_get_supported_modes(&supported_modes);
> >> +
> >> +	if (ret != 0 || supported_modes == 0)
> >> +		return -ENXIO;
> >> +
> >> +	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) & 1)
> >> +		set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
> >> +	if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
> >> +		set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
> >> +	if ((supported_modes >> DELL_BALANCED) & 1)
> >> +		set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
> >> +	if ((supported_modes >> DELL_PERFORMANCE) & 1)
> >> +		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
> >> +
> >> +	platform_profile_register(thermal_handler);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +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");
> >
> > I think that -ENOMEM error should be failure of the loading the driver.
> > It does not make sense to continue of memory allocation failed.
> >
> > On the other hand when the thermal modes are not support (e.g. old
> > Latitude models) then there should not be a warning message. It is
> > expected that on systems without thermal modes the setup fails.
> 
> Thank you for your feedback.
> 
> I agree with your suggestion. -ENOMEM would indicate something bigger is amiss. I can add a check:
> 
> If -ENOMEM, fail driver.
> If anything other error, skip, but do not show a message.
> 
> Lyndon

Maybe you can simplify it more. thermal_init() could return 0 also for
the case when thermal modes are not supported. And dell_init() then can
unconditionally fail when thermal_init() returns non-zero value. It has
benefit that in case thermal_init() is extended in future for some other
fatal error, it would not be required to update also caller to handle
another error (and not just ENOMEM).