From: Armin Wolf <W_Armin@gmx.de>
To: "Pali Rohár" <pali@kernel.org>
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
Date: Tue, 15 Feb 2022 21:19:17 +0100 [thread overview]
Message-ID: <6c9a1f7a-004e-1991-abef-181cc117907e@gmx.de> (raw)
In-Reply-To: <20220215194909.py62gbsfwe2qxq2i@pali>
Am 15.02.22 um 20:49 schrieb Pali Rohár:
> 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.
Afaik, access to iopl() and ioperm() is blocked in this mode too.
This for example causes dell-bios-fan-control to fail when
the machine is booted in Secure Boot mode.
> 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.
>
That is the reason i limited the number of selectable code pairs to
two which are known to work on a wide range of machines.
So users are unable to specify SMM commands, which are controlled by
the driver. If this still is to much, then we may drop this patch.
>>>> 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.
Since there are two code pairs, i could not reuse the force param for that.
Since the newly added pair is unused, should i remove it and reuse the
force param?
>>>> +
>>>> 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
>>>>
next prev parent reply other threads:[~2022-02-15 20:19 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-15 19:11 [PATCH 0/7] hwmon: (dell-smm) Miscellaneous improvements Armin Wolf
2022-02-15 19:11 ` [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter Armin Wolf
2022-02-15 19:19 ` Pali Rohár
2022-02-15 19:45 ` Armin Wolf
[not found] ` <a450a2b6-92d3-d2cd-db63-b578480ff385@gmx.de>
2022-02-15 19:49 ` Pali Rohár
2022-02-15 20:19 ` Armin Wolf [this message]
2022-02-15 20:31 ` Pali Rohár
2022-02-15 21:00 ` Armin Wolf
2022-02-15 19:11 ` [PATCH 2/7] hwmon: (dell-smm) Add additional fan mode command combination Armin Wolf
2022-02-15 19:11 ` [PATCH 3/7] hwmon: (dell-smm) Make fan/temp sensor number a u8 Armin Wolf
2022-02-15 19:37 ` Pali Rohár
2022-02-19 14:47 ` Guenter Roeck
2022-02-15 19:11 ` [PATCH 4/7] hwmon: (dell-smm) Improve temperature sensors detection Armin Wolf
2022-02-19 14:51 ` Guenter Roeck
2022-02-15 19:11 ` [PATCH 5/7] hwmon: (dell-smm) Improve assembly code Armin Wolf
2022-02-16 0:09 ` kernel test robot
2022-02-15 19:11 ` [PATCH 6/7] hwmon: (dell-smm) Add SMM interface documentation Armin Wolf
2022-02-15 19:34 ` Pali Rohár
2022-02-19 14:46 ` Guenter Roeck
2022-02-15 19:11 ` [PATCH 7/7] hwmon: (dell-smm) Reword and mark parameter "force" as unsafe Armin Wolf
2022-02-15 19:35 ` Pali Rohár
2022-02-19 14:43 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6c9a1f7a-004e-1991-abef-181cc117907e@gmx.de \
--to=w_armin@gmx.de \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=pali@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).