Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Feb 2022 20:49:09 +0100
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Armin Wolf <W_Armin@gmx.de>
Cc:     jdelvare@suse.com, linux@roeck-us.net, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control
 method as module parameter
Message-ID: <20220215194909.py62gbsfwe2qxq2i@pali>
References: <20220215191113.16640-1-W_Armin@gmx.de>
 <20220215191113.16640-2-W_Armin@gmx.de>
 <20220215191941.azk5gpcn42ahcnna@pali>
 <a450a2b6-92d3-d2cd-db63-b578480ff385@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a450a2b6-92d3-d2cd-db63-b578480ff385@gmx.de>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Tuesday 15 February 2022 20:44:20 Armin Wolf wrote:
> Am 15.02.22 um 20:19 schrieb Pali Rohár:
> > On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
> > > Right now, the only way to test if setting manual/auto fan control works
> > > is to edit and recompile the module, which may be too cumbersome for
> > > the average user.
> > There is also another way suitable for testing purposes which do not
> > requires any kernel patch. Call iopl(3) syscall which changes I/O
> > privilege level to 3 and which allows to poke I/O registers.
> > 
> This is not possible under kernel lockdown/Secure Boot.

Under Secure Boot it should be still possible.

With kernel lockdown, no kernel testing/debugging at HW level is
acceptable due to security reasons, as it is against what kernel
lockdown should achieve.

> > > Allow for specifying the desired fan mode control method when loading
> > > the module, but taint the kernel if so since there is the possibility
> > > for strange side effects on non-whitelisted models.
> > > Also update docs and kernel-parameters.txt accordingly.
> > > 
> > > Tested on a Dell Inspiron 3505.
> > > 
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > ---
> > >   .../admin-guide/kernel-parameters.txt         |  3 +++
> > >   Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
> > >   drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
> > >   3 files changed, 35 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index d68053db21cc..4f1b6c2b7ed1 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -968,6 +968,9 @@
> > >   	dell_smm_hwmon.fan_max=
> > >   			[HW] Maximum configurable fan speed.
> > > 
> > > +	dell_smm_hwmon.fan_mode_method=
> > > +			[HW] Method to use for changing fan mode.
> > > +
> > >   	dfltcc=		[HW,S390]
> > >   			Format: { on | off | def_only | inf_only | always }
> > >   			on:       s390 zlib hardware support for compression on
> > > diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> > > index beec88491171..564d99cda869 100644
> > > --- a/Documentation/hwmon/dell-smm-hwmon.rst
> > > +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> > > @@ -67,13 +67,16 @@ for your hardware. It is possible that codes that work for other
> > >   laptops actually work for yours as well, or that you have to discover
> > >   new codes.
> > > 
> > > -Check the list ``i8k_whitelist_fan_control`` in file
> > > -``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree: as a first
> > > -attempt you can try to add your machine and use an already-known code
> > > -pair. If, after recompiling the kernel, you see that ``pwm1_enable``
> > > -is present and works (i.e., you can manually control the fan speed),
> > > -then please submit your finding as a kernel patch, so that other users
> > > -can benefit from it. Please see
> > > +As a first step, you can load the module with the module parameter
> > > +``fan_mode_method`` set to 1 to test if your hardware works with
> > > +an already know method for disabling automatic BIOS fan control.
> > > +If ``pwm1_enable`` is now present and works (i.e., you can
> > > +manually control the fan speed), then please submit your finding
> > > +as a kernel patch, so that other users can benefit from it.
> > > +Just add your model to the list ``i8k_whitelist_fan_control`` in
> > > +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
> > > +the already known code pair.
> > > +Please read
> > >   :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
> > >   for information on submitting patches.
> > > 
> > > @@ -120,6 +123,10 @@ Module parameters
> > >                      Maximum configurable fan speed. (default:
> > >                      autodetect)
> > > 
> > > +* fan_mode_method:uint
> > > +                   Method to use for changing fan mode (default:
> > > +                   from whitelist)
> > > +
> > >   Legacy ``/proc`` interface
> > >   --------------------------
> > > 
> > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > index 9949eeb79378..1c4cc516c8b2 100644
> > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > @@ -111,6 +111,10 @@ static uint fan_max;
> > >   module_param(fan_max, uint, 0);
> > >   MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
> > > 
> > > +static uint fan_mode_method;
> > > +module_param_unsafe(fan_mode_method, uint, 0);
> > > +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
> > No, please really do not introduce another kernel parameter for this
> > driver. There are already many and we do not need to extend this list.
> > 
> > > +
> > >   struct smm_regs {
> > >   	unsigned int eax;
> > >   	unsigned int ebx;
> > > @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> > > 
> > >   			break;
> > >   		case hwmon_pwm_enable:
> > > -			if (data->auto_fan)
> > > +			if (data->auto_fan && data->manual_fan)
> > >   				/*
> > >   				 * There is no command for retrieve the current status
> > >   				 * from BIOS, and userspace/firmware itself can change
> > > @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
> > >   	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
> > >   	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
> > > 
> > > -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > -	if (fan_control && fan_control->driver_data) {
> > > -		const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > +	/* value specified via module param overrides whitelist */
> > > +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
> > > +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
> > > +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
> > > +	} else {
> > > +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > +		if (fan_control && fan_control->driver_data) {
> > > +			const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > 
> > > -		data->manual_fan = control->manual_fan;
> > > -		data->auto_fan = control->auto_fan;
> > > -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > +			data->manual_fan = control->manual_fan;
> > > +			data->auto_fan = control->auto_fan;
> > > +		}
> > >   	}
> > > +	if (data->manual_fan && data->auto_fan)
> > > +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > 
> > >   	if (!fan_mult) {
> > >   		/*
> > > --
> > > 2.30.2
> > >