| 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
| ||
|
Message-Id: <D8JGDXSAXU6C.39P8V5E3YQR8Q@gmail.com>
Date: Tue, 18 Mar 2025 09:14:46 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Werner Sembach" <wse@...edocomputers.com>, <hdegoede@...hat.com>,
<ilpo.jarvinen@...ux.intel.com>, <jdelvare@...e.com>, <linux@...ck-us.net>
Cc: <linux-kernel@...r.kernel.org>, <platform-driver-x86@...r.kernel.org>,
<linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH v3 1/1] platform/x86/tuxedo: Implement TUXEDO TUXI ACPI
TFAN via hwmon
On Tue Mar 18, 2025 at 8:08 AM -05, Werner Sembach wrote:
> Hi Kurt,
>
> Am 18.03.25 um 06:32 schrieb Kurt Borja:
>> Hi Werner,
>>
>> On Thu Mar 13, 2025 at 5:09 PM -05, Werner Sembach wrote:
>>> The TUXEDO Sirius 16 Gen1 & Gen2 have the custom TUXEDO Interface (TUXI)
>>> ACPI interface which currently consists of the TFAN device. This has ACPI
>>> functions to control the built in fans and monitor fan speeds and CPU and
>>> GPU temperature.
>>>
>>> This driver implements this TFAN device via the hwmon subsystem with an
>>> added temperature check that ensure a minimum fanspeed at certain
>>> temperatures. This allows userspace controlled, but hardware safe, custom
>>> fan curves.
>>>
>>> Signed-off-by: Werner Sembach <wse@...edocomputers.com>
>>> ---
>>> MAINTAINERS | 6 +
>>> drivers/platform/x86/Kconfig | 2 +
>>> drivers/platform/x86/Makefile | 3 +
>>> drivers/platform/x86/tuxedo/Kbuild | 8 +
>>> drivers/platform/x86/tuxedo/Kconfig | 8 +
>>> drivers/platform/x86/tuxedo/nbxx/Kbuild | 9 +
>>> drivers/platform/x86/tuxedo/nbxx/Kconfig | 15 +
>>> drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c | 591 +++++++++++++++++++
>>> 8 files changed, 642 insertions(+)
>>> create mode 100644 drivers/platform/x86/tuxedo/Kbuild
>>> create mode 100644 drivers/platform/x86/tuxedo/Kconfig
>>> create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kbuild
>>> create mode 100644 drivers/platform/x86/tuxedo/nbxx/Kconfig
>>> create mode 100644 drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 8e0736dc2ee0e..7139c32e96dc7 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -24190,6 +24190,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
>>> F: tools/power/x86/turbostat/
>>> F: tools/testing/selftests/turbostat/
>>>
>>> +TUXEDO DRIVERS
>>> +M: Werner Sembach <wse@...edocomputers.com>
>>> +L: platform-driver-x86@...r.kernel.org
>>> +S: Supported
>>> +F: drivers/platform/x86/tuxedo/
>>> +
>>> TW5864 VIDEO4LINUX DRIVER
>>> M: Bluecherry Maintainers <maintainers@...echerrydvr.com>
>>> M: Andrey Utkin <andrey.utkin@...p.bluecherry.net>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 0258dd879d64b..58b258cde4fdb 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -1186,6 +1186,8 @@ config SEL3350_PLATFORM
>>> To compile this driver as a module, choose M here: the module
>>> will be called sel3350-platform.
>>>
>>> +source "drivers/platform/x86/tuxedo/Kconfig"
>>> +
>>> endif # X86_PLATFORM_DEVICES
>>>
>>> config P2SB
>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>> index e1b1429470674..1562dcd7ad9a5 100644
>>> --- a/drivers/platform/x86/Makefile
>>> +++ b/drivers/platform/x86/Makefile
>>> @@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS) += winmate-fm07-keys.o
>>>
>>> # SEL
>>> obj-$(CONFIG_SEL3350_PLATFORM) += sel3350-platform.o
>>> +
>>> +# TUXEDO
>>> +obj-y += tuxedo/
>>> diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
>> I think this should be named Makefile instead of Kbuild.
>
> Ack
>
> I thought since I did not use anything that is makefile syntax I should use
> Kbuild, but I looked it up and found this:
> https://www.kernel.org/doc/html/v6.14-rc7/kbuild/makefiles.html#the-kbuild-files
>
>>
>>> new file mode 100644
>>> index 0000000000000..dc55b403f201d
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/tuxedo/Kbuild
>>> @@ -0,0 +1,8 @@
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +#
>>> +# Copyright (C) 2024-2025 Werner Sembach wse@...edocomputers.com
>>> +#
>>> +# TUXEDO X86 Platform Specific Drivers
>>> +#
>>> +
>>> +obj-y += nbxx/
>>> diff --git a/drivers/platform/x86/tuxedo/Kconfig b/drivers/platform/x86/tuxedo/Kconfig
>>> new file mode 100644
>>> index 0000000000000..1b22a0b29460a
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/tuxedo/Kconfig
>>> @@ -0,0 +1,8 @@
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +#
>>> +# Copyright (C) 2024-2025 Werner Sembach wse@...edocomputers.com
>>> +#
>>> +# TUXEDO X86 Platform Specific Drivers
>>> +#
>>> +
>>> +source "drivers/platform/x86/tuxedo/nbxx/Kconfig"
>>> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kbuild b/drivers/platform/x86/tuxedo/nbxx/Kbuild
>> Same as above.
> Ack
>>
>>> new file mode 100644
>>> index 0000000000000..256b03921c732
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/tuxedo/nbxx/Kbuild
>>> @@ -0,0 +1,9 @@
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +#
>>> +# Copyright (C) 2024-2025 Werner Sembach wse@...edocomputers.com
>>> +#
>>> +# TUXEDO X86 Platform Specific Drivers
>>> +#
>>> +
>>> +tuxedo_nbxx_acpi_tuxi-y := acpi_tuxi.o
>>> +obj-$(CONFIG_TUXEDO_NBXX_ACPI_TUXI) += tuxedo_nbxx_acpi_tuxi.o
>>> diff --git a/drivers/platform/x86/tuxedo/nbxx/Kconfig b/drivers/platform/x86/tuxedo/nbxx/Kconfig
>>> new file mode 100644
>>> index 0000000000000..0d011c0c715a5
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/tuxedo/nbxx/Kconfig
>>> @@ -0,0 +1,15 @@
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +#
>>> +# Copyright (C) 2024-2025 Werner Sembach wse@...edocomputers.com
>>> +#
>>> +# TUXEDO X86 Platform Specific Drivers
>>> +#
>>> +
>>> +config TUXEDO_NBXX_ACPI_TUXI
>>> + tristate "TUXEDO NBxx ACPI TUXI Platform Driver"
>>> + help
>>> + This driver implements the ACPI TUXI device found on some TUXEDO
>>> + Notebooks. Currently this consists only of the TFAN subdevice which is
>>> + implemented via hwmon.
>>> +
>>> + When compiled as a module it will be called tuxedo_nbxx_acpi_tuxi
>>> diff --git a/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
>>> new file mode 100644
>>> index 0000000000000..bb452cc33568a
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/tuxedo/nbxx/acpi_tuxi.c
>>> @@ -0,0 +1,591 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright (C) 2024-2025 Werner Sembach wse@...edocomputers.com
>>> + */
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/cleanup.h>
>>> +#include <linux/device.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/limits.h>
>>> +#include <linux/minmax.h>
>>> +#include <linux/module.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/units.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#define TUXI_SAFEGUARD_PERIOD 1000 // 1s
>>> +#define TUXI_PWM_FAN_ON_MIN_SPEED 0x40 // ~25%
>>> +#define TUXI_TEMP_LEVEL_HYSTERESIS 1500 // 1.5°C
>>> +#define TUXI_FW_TEMP_OFFSET 2730 // Kelvin to Celsius
>>> +#define TUXI_MAX_FAN_COUNT 16 /*
>>> + * If this is increased, new lines must
>>> + * be added to hwmcinfo below.
>>> + */
>> The style of these macros could be better.
>>
>> I suggest using an enum instead. Something like:
>>
>> enum TUXI_SAFEGUARD_LIMITS {
>> ...
>> }
> Since these values are all on very different scales an enum feels somehow wrong
> for me here.
I agree.
>>
>> with it's values aligned and the comment on top.
> Ack
>>
>>> +
>>> +static const struct acpi_device_id acpi_device_ids[] = {
>>> + {"TUXI0000", 0},
>>> + {"", 0}
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, acpi_device_ids);
>>> +
>>> +struct tuxi_driver_data_t {
>>> + acpi_handle tfan_handle;
>>> + struct device *hwmdev;
>>> +};
>>> +
>>> +struct tuxi_hwmon_driver_data_t {
>>> + struct delayed_work work;
>>> + struct device *hwmdev;
>>> + u8 fan_count;
>>> + const char *fan_types[TUXI_MAX_FAN_COUNT];
>>> + u8 temp_level[TUXI_MAX_FAN_COUNT];
>>> + u8 curr_speed[TUXI_MAX_FAN_COUNT];
>>> + u8 want_speed[TUXI_MAX_FAN_COUNT];
>>> + u8 pwm_enabled;
>>> +};
>>> +
>>> +struct tuxi_temp_high_config_t {
>>> + long temp;
>>> + u8 min_speed;
>>> +};
>>> +
>>> +/*
>>> + * Speed values in this table must be >= TUXI_PWM_FAN_ON_MIN_SPEED to avoid
>>> + * undefined behaviour.
>>> + */
>>> +static const struct tuxi_temp_high_config_t temp_levels[] = {
>>> + { 80000, 0x4d }, // ~30%
>>> + { 90000, 0x66 }, // ~40%
>>> + { 100000, 0xff }, // 100%
>>> + { }
>>> +};
>>> +
>>> +/*
>>> + * Set fan speed target
>>> + *
>>> + * Set a specific fan speed (needs manual mode)
>>> + *
>>> + * Arg0: Fan index
>>> + * Arg1: Fan speed as a fraction of maximum speed (0-255)
>>> + */
>>> +#define TUXI_TFAN_METHOD_SET_FAN_SPEED "SSPD"
>>> +
>>> +/*
>>> + * Get fan speed target
>>> + *
>>> + * Arg0: Fan index
>>> + * Returns: Current fan speed target a fraction of maximum speed (0-255)
>>> + */
>>> +#define TUXI_TFAN_METHOD_GET_FAN_SPEED "GSPD"
>>> +
>>> +/*
>>> + * Get fans count
>>> + *
>>> + * Returns: Number of individually controllable fans
>>> + */
>>> +#define TUXI_TFAN_METHOD_GET_FAN_COUNT "GCNT"
>>> +
>>> +/*
>>> + * Set fans mode
>>> + *
>>> + * Arg0: 0 = auto, 1 = manual
>>> + */
>>> +#define TUXI_TFAN_METHOD_SET_FAN_MODE "SMOD"
>>> +
>>> +/*
>>> + * Get fans mode
>>> + *
>>> + * Returns: 0 = auto, 1 = manual
>>> + */
>>> +#define TUXI_TFAN_METHOD_GET_FAN_MODE "GMOD"
>>> +
>>> +#define TUXI_TFAN_FAN_MODE_AUTO 0
>>> +#define TUXI_TFAN_FAN_MODE_MANUAL 1
>>> +
>>> +/*
>>> + * Get fan type/what the fan is pointed at
>>> + *
>>> + * Arg0: Fan index
>>> + * Returns: 0 = general, 1 = CPU, 2 = GPU
>>> + */
>>> +#define TUXI_TFAN_METHOD_GET_FAN_TYPE "GTYP"
>>> +
>>> +static const char * const tuxi_fan_type_labels[] = {
>>> + "general",
>>> + "cpu",
>>> + "gpu"
>>> +};
>>> +
>>> +/*
>>> + * Get fan temperature/temperature of what the fan is pointed at
>>> + *
>>> + * Arg0: Fan index
>>> + * Returns: Temperature sensor value in 10ths of degrees kelvin
>>> + */
>>> +#define TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE "GTMP"
>>> +
>>> +/*
>>> + * Get actual fan speed in RPM
>>> + *
>>> + * Arg0: Fan index
>>> + * Returns: Speed sensor value in revolutions per minute
>>> + */
>>> +#define TUXI_TFAN_METHOD_GET_FAN_RPM "GRPM"
>>> +
>>> +static int tuxi_tfan_method(struct acpi_device *device, acpi_string method,
>>> + unsigned long long *params, u32 pcount,
>>> + unsigned long long *retval)
>>> +{
>>> + struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
>>> + acpi_handle handle = driver_data->tfan_handle;
>>> + union acpi_object *obj __free(kfree) = NULL;
>>> + struct acpi_object_list arguments;
>>> + unsigned long long data;
>>> + acpi_status status;
>>> + unsigned int i;
>>> +
>>> + if (pcount > 0) {
>>> + obj = kcalloc(pcount, sizeof(*arguments.pointer), GFP_KERNEL);
>>> +
>>> + arguments.count = pcount;
>>> + arguments.pointer = obj;
>>> + for (i = 0; i < pcount; ++i) {
>>> + arguments.pointer[i].type = ACPI_TYPE_INTEGER;
>>> + arguments.pointer[i].integer.value = params[i];
>>> + }
>>> + }
>>> + status = acpi_evaluate_integer(handle, method,
>>> + pcount ? &arguments : NULL, &data);
>>> + if (ACPI_FAILURE(status))
>>> + return_ACPI_STATUS(status);
>>> +
>>> + if (retval)
>>> + *retval = data;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static umode_t hwm_is_visible(const void *data, enum hwmon_sensor_types type,
>> All of these callbacks should be prefixed with "tuxi_".
> Ack.
>>
>>> + u32 __always_unused attr, int channel)
>>> +{
>>> + struct tuxi_hwmon_driver_data_t const *driver_data = data;
>>> +
>>> + if (channel >= driver_data->fan_count)
>>> + return 0;
>>> +
>>> + switch (type) {
>>> + case hwmon_fan:
>>> + return 0444;
>>> + case hwmon_pwm:
>>> + return 0644;
>>> + case hwmon_temp:
>>> + return 0444;
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>> + int channel, long *val)
>>> +{
>>> + struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>>> + struct acpi_device *pdev = to_acpi_device(dev->parent);
>>> + unsigned long long params[2], retval;
>>> + int ret;
>>> +
>>> + switch (type) {
>>> + case hwmon_fan:
>>> + params[0] = channel;
>>> + ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_RPM,
>>> + params, 1, &retval);
>>> + *val = retval > S32_MAX ? S32_MAX : retval;
>> Use clamp_val().
> Thanks for the tip
>>
>> Also, is an RPM >S32_MAX a bogus value? Should it be treated as an
>> error instead?
> Yeah sounds good, will do.
>>
>>> + return ret;
>>> + case hwmon_pwm:
>>> + switch (attr) {
>>> + case hwmon_pwm_input:
>>> + if (driver_data->pwm_enabled) {
>> I think this needs locking.
> Since this was only meant as a small performance optimization if wonder if I
> shouldn't just skip the if-case and just ask the wmi every time instead?
IMO, yes. I don't think it's necessary to cache the value either, as you
could just retrieve it every time. That way you avoid having to provide
locking.
>>
>>> + *val = driver_data->curr_speed[channel];
>>> + return 0;
>>> + }
>>> + params[0] = channel;
>>> + ret = tuxi_tfan_method(pdev,
>>> + TUXI_TFAN_METHOD_GET_FAN_SPEED,
>>> + params, 1, &retval);
>>> + *val = retval > S32_MAX ? S32_MAX : retval;
>> HWMON ABI specifies that the `pwm` attribute range is 0-255. Are values
>> above this, bogus values?
> Didn't know, but ofc I can change the check here.
>>
>>> + return ret;
>>> + case hwmon_pwm_enable:
>>> + *val = driver_data->pwm_enabled;
>>> + return ret;
>> `ret` is uninitialized when used here!
> Sorry, will fix.
>>
>>> + }
>>> + break;
>>> + case hwmon_temp:
>>> + params[0] = channel;
>>> + ret = tuxi_tfan_method(pdev,
>>> + TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
>>> + params, 1, &retval);
>>> + *val = retval > S32_MAX / 100 ?
>>> + S32_MAX : (retval - TUXI_FW_TEMP_OFFSET) * 100;
>> Same comment as the clamped values above.
>>
>> Additionally, please use deci_kelvin_to_millicelsius() from
>> <linux/units.h> and IMO this should be an standard if-else block.
>
> Because of firmware weirdness I can't use deci_kelvin_to_millicelsius: While the
> firmware claims to output 10th kelvin it actually outputs only full degrees with
> the last number always being 0, that's why I defined the custom
> TUXI_FW_TEMP_OFFSET as 2730 instead of 2731.
Then you can use deci_kelvin_to_millicelsius_with_offset() and provide
your own offset.
>
> I could use deci_kelvin_to_celsius()*MILLIDEGREE_PER_DEGREE, but this would
deci_kelvin_to_millicelsius() avoids rounding and makes that
multiplication unnecessary.
> introduce an unnecessary rounding step.
>
>>
>>> + return ret;
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int hwm_read_string(struct device *dev,
>>> + enum hwmon_sensor_types __always_unused type,
>>> + u32 __always_unused attr, int channel,
>>> + const char **str)
>>> +{
>>> + struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>>> +
>>> + *str = driver_data->fan_types[channel];
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int write_speed(struct device *dev, int channel, u8 val, bool force)
>>> +{
>>> + struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>>> + struct acpi_device *pdev = to_acpi_device(dev->parent);
>>> + unsigned long long new_speed, params[2];
>>> + u8 temp_level;
>>> + int ret;
>>> +
>>> + params[0] = channel;
>>> +
>>> + /*
>>> + * The heatpipe across the VRMs is shared between both fans and the VRMs
>>> + * are the most likely to go up in smoke. So it's better to apply the
>>> + * minimum fan speed to all fans if either CPU or GPU is working hard.
>>> + */
>>> + temp_level = max_array(driver_data->temp_level, driver_data->fan_count);
>>> + if (temp_level)
>>> + new_speed = max(val, temp_levels[temp_level - 1].min_speed);
>>> + else if (val < TUXI_PWM_FAN_ON_MIN_SPEED / 2)
>>> + new_speed = 0;
>>> + else if (val < TUXI_PWM_FAN_ON_MIN_SPEED)
>>> + new_speed = TUXI_PWM_FAN_ON_MIN_SPEED;
>>> + else
>>> + new_speed = val;
>>> +
>>> + if (force || new_speed != driver_data->curr_speed[channel]) {
>>> + params[0] = channel;
>>> + params[1] = new_speed;
>>> + ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_SPEED,
>>> + params, 2, NULL);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + driver_data->curr_speed[channel] = new_speed;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int hwm_write(struct device *dev,
>>> + enum hwmon_sensor_types __always_unused type, u32 attr,
>>> + int channel, long val)
>>> +{
>>> + struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(dev);
>>> + struct acpi_device *pdev = to_acpi_device(dev->parent);
>>> + unsigned long long params[2];
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + switch (attr) {
>>> + case hwmon_pwm_input:
>>> + if (val > U8_MAX || val < 0)
>>> + return -EINVAL;
>> Please, clamp_val() instead.
> ack
>>
>>> +
>>> + if (driver_data->pwm_enabled) {
>> I believe this needs locking to ensure pwm_enabled hasn't changed.
> ok
>>
>>> + driver_data->want_speed[channel] = val;
>>> + return write_speed(dev, channel, val, false);
>>> + }
>>> +
>>> + return 0;
>>> + case hwmon_pwm_enable:
>>> + params[0] = val ? TUXI_TFAN_FAN_MODE_MANUAL :
>>> + TUXI_TFAN_FAN_MODE_AUTO;
>> Per ABI specification, a value of 0 corresponds to fans at full speed,
>> not "auto". A value of 2+ corresponds to auto.
>>
>> Please, refer to:
>>
>> https://docs.kernel.org/admin-guide/abi-testing.html#abi-sys-class-hwmon-hwmonx-pwmy-enable
>>
>> for more details.
> Thanks for the link, will fix
>>
>>> + ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE,
>>> + params, 1, NULL);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + driver_data->pwm_enabled = val;
>>> +
>>> + /*
>>> + * Activating PWM sets speed to 0. Alternative design decision
>>> + * could be to keep the current value. This would require proper
>>> + * setting of driver_data->curr_speed for example.
>>> + */
>>> + if (val)
>>> + for (i = 0; i < driver_data->fan_count; ++i) {
>>> + ret = write_speed(dev, i, 0, true);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> + }
>>> +
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static const struct hwmon_ops hwmops = {
>> Prefix this struct with "tuxi_"
> ok
>>
>>> + .is_visible = hwm_is_visible,
>>> + .read = hwm_read,
>>> + .read_string = hwm_read_string,
>>> + .write = hwm_write,
>>> +};
>>> +
>>> +static const struct hwmon_channel_info * const hwmcinfo[] = {
>> Same as above.
> ok
>>
>>> + HWMON_CHANNEL_INFO(fan,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL,
>>> + HWMON_F_INPUT | HWMON_F_LABEL),
>>> + HWMON_CHANNEL_INFO(pwm,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
>>> + HWMON_CHANNEL_INFO(temp,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL),
>>> + NULL
>>> +};
>>> +
>>> +static const struct hwmon_chip_info hwminfo = {
>> Same as above.
> ok
>>
>>> + .ops = &hwmops,
>>> + .info = hwmcinfo
>>> +};
>>> +
>>> +static u8 tuxi_get_temp_level(struct tuxi_hwmon_driver_data_t *driver_data,
>>> + u8 fan_id, long temp)
>>> +{
>>> + long temp_low, temp_high;
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + ret = driver_data->temp_level[fan_id];
>>> +
>>> + for (i = 0; temp_levels[i].temp; ++i) {
>>> + temp_low = i == 0 ? S32_MIN : temp_levels[i - 1].temp;
>>> + temp_high = temp_levels[i].temp;
>>> + if (ret > i)
>>> + temp_high -= TUXI_TEMP_LEVEL_HYSTERESIS;
>>> +
>>> + if (temp >= temp_low && temp < temp_high)
>>> + return i;
>>> + }
>>> + if (temp >= temp_high)
>>> + ret = i;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void tuxi_periodic_hw_safeguard(struct work_struct *work)
>> I wonder if all of this convoluted logic is really necessary. Why can't
>> you just force TUXI_TFAN_FAN_MODE_AUTO if a certain limit is surpassed?
> The auto mode, aka the built in fan curve is way more aggressive than this, and
> reaching 80°C is very easy on this device.
If it's more aggresive, isn't it safer? IMO we should trust FW instead
of hardcoded values.
>>
>> That would mean fewer CPU instructions that are constantly running.
>>
>>> +{
>>> + struct tuxi_hwmon_driver_data_t *driver_data = container_of(work,
>>> + struct tuxi_hwmon_driver_data_t,
>>> + work.work);
>>> + struct device *dev = driver_data->hwmdev;
>>> + struct acpi_device *pdev = to_acpi_device(dev->parent);
>>> + unsigned long long params[2], retval;
>>> + unsigned int i;
>>> + long temp;
>>> + int ret;
>>> +
>>> + for (i = 0; i < driver_data->fan_count; ++i) {
>>> + params[0] = i;
>>> + ret = tuxi_tfan_method(pdev,
>>> + TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
>>> + params, 1, &retval);
>>> + /*
>>> + * If reading the temperature fails, default to a high value to
>>> + * be on the safe side in the worst case.
>>> + */
>>> + if (ret)
>>> + retval = TUXI_FW_TEMP_OFFSET + 1200;
>>> +
>>> + temp = retval > S32_MAX / 100 ?
>>> + S32_MAX : (retval - TUXI_FW_TEMP_OFFSET) * 100;
>>> +
>>> + driver_data->temp_level[i] = tuxi_get_temp_level(driver_data, i,
>>> + temp);
>>> + }
>>> +
>>> + // Reapply want_speeds to respect eventual new temp_levels
>>> + for (i = 0; i < driver_data->fan_count; ++i)
>>> + write_speed(dev, i, driver_data->want_speed[i], false);
>>> +
>>> + schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
>>> +}
>>> +
>>> +static int tuxi_hwmon_add_devices(struct acpi_device *pdev, struct device **hwmdev)
>>> +{
>>> + struct tuxi_hwmon_driver_data_t *driver_data;
>>> + unsigned long long params[2], retval;
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + /*
>>> + * The first version of TUXI TFAN didn't have the Get Fan Temperature
>>> + * method which is integral to this driver. So probe for existence and
>>> + * abort otherwise.
>>> + *
>>> + * The Get Fan Speed method is also missing in that version, but was
>>> + * added in the same version so it doesn't have to be probe separately.
>>> + */
>>> + params[0] = 0;
>>> + ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TEMPERATURE,
>>> + params, 1, &retval);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data), GFP_KERNEL);
>>> + if (!driver_data)
>>> + return -ENOMEM;
>>> +
>>> + /*
>>> + * Loading this module sets the fan mode to auto. Alternative design
>>> + * decision could be to keep the current value. This would require
>>> + * proper initialization of driver_data->curr_speed for example.
>>> + */
>>> + params[0] = TUXI_TFAN_FAN_MODE_AUTO;
>>> + ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1,
>>> + NULL);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_COUNT, NULL, 0,
>>> + &retval);
>>> + if (ret)
>>> + return ret;
>>> + if (retval > TUXI_MAX_FAN_COUNT)
>>> + return -EINVAL;
>>> + driver_data->fan_count = retval;
>>> +
>>> + for (i = 0; i < driver_data->fan_count; ++i) {
>>> + params[0] = i;
>>> + ret = tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_GET_FAN_TYPE,
>>> + params, 1, &retval);
>>> + if (ret)
>>> + return ret;
>>> + if (retval >= ARRAY_SIZE(tuxi_fan_type_labels))
>>> + return -EOPNOTSUPP;
>>> + driver_data->fan_types[i] = tuxi_fan_type_labels[retval];
>>> + }
>>> +
>>> + *hwmdev = devm_hwmon_device_register_with_info(&pdev->dev,
>>> + "tuxedo_nbxx_acpi_tuxi",
>>> + driver_data, &hwminfo,
>>> + NULL);
>>> + if (IS_ERR(*hwmdev))
>>> + return PTR_ERR(*hwmdev);
>>> +
>>> + driver_data->hwmdev = *hwmdev;
>>> +
>>> + INIT_DELAYED_WORK(&driver_data->work, tuxi_periodic_hw_safeguard);
>>> + schedule_delayed_work(&driver_data->work, TUXI_SAFEGUARD_PERIOD);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void tuxi_hwmon_remove_devices(struct device *hwmdev)
>>> +{
>>> + struct tuxi_hwmon_driver_data_t *driver_data = dev_get_drvdata(hwmdev);
>>> + struct acpi_device *pdev = to_acpi_device(hwmdev->parent);
>>> + unsigned long long params[2];
>>> +
>>> + cancel_delayed_work_sync(&driver_data->work);
>>> +
>>> + params[0] = TUXI_TFAN_FAN_MODE_AUTO;
>>> + tuxi_tfan_method(pdev, TUXI_TFAN_METHOD_SET_FAN_MODE, params, 1, NULL);
>>> +}
>>> +
>>> +static int tuxi_add(struct acpi_device *device)
>>> +{
>>> + struct tuxi_driver_data_t *driver_data;
>>> + acpi_status status;
>>> +
>>> + driver_data = devm_kzalloc(&device->dev, sizeof(*driver_data),
>>> + GFP_KERNEL);
>>> + if (!driver_data)
>>> + return -ENOMEM;
>>> +
>>> + // Find subdevices
>>> + status = acpi_get_handle(device->handle, "TFAN",
>>> + &driver_data->tfan_handle);
>>> + if (ACPI_FAILURE(status))
>>> + return_ACPI_STATUS(status);
>>> +
>>> + dev_set_drvdata(&device->dev, driver_data);
>>> +
>>> + return tuxi_hwmon_add_devices(device, &driver_data->hwmdev);
>>> +}
>>> +
>>> +static void tuxi_remove(struct acpi_device *device)
>>> +{
>>> + struct tuxi_driver_data_t *driver_data = dev_get_drvdata(&device->dev);
>>> +
>>> + tuxi_hwmon_remove_devices(driver_data->hwmdev);
>>> +}
>>> +
>>> +static struct acpi_driver acpi_driver = {
>> AFAIK platform drivers are *strongly* preferred over acpi drivers. You
>> should use that instead.
>
> Sorry, first time reading this. Can change it ofc, but is there a specific reason?
I'm not really sure. But while samsung-galaxybook was in review, I
believe this was requested by Hans and Armin.
Hopefully they can elaborate on this.
--
~ Kurt
>
> Best regards,
>
> Werner
>
>>
>> For an example check the samsung-galaxybook driver in the for-next
>> branch.
>>
Powered by blists - more mailing lists