| 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: <b0899d74-79d7-459c-8f2d-17a17a0f58d7@gmx.de>
Date: Fri, 12 Apr 2024 01:38:11 +0200
From: Armin Wolf <W_Armin@....de>
To: Mikael Lund Jepsen <mlj@...elec.com>,
"rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
"lenb@...nel.org" <lenb@...nel.org>
Cc: "jdelvare@...e.com" <jdelvare@...e.com>,
"linux@...ck-us.net" <linux@...ck-us.net>,
"linux@...ssschuh.net" <linux@...ssschuh.net>,
"ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>
Subject: Re: [PATCH v2] ACPI: fan: Add hwmon support
Am 11.04.24 um 22:30 schrieb Armin Wolf:
> Am 10.04.24 um 16:29 schrieb Mikael Lund Jepsen:
>
>> On 9. april 2024 Armin Wolf wrote:
>>> To: Mikael Lund Jepsen <mlj@...elec.com>;
>>> rafael.j.wysocki@...el.com; lenb@...nel.org
>>> Cc: jdelvare@...e.com; linux@...ck-us.net; linux@...ssschuh.net;
>>> ilpo.jarvinen@...ux.intel.com; linux-acpi@...r.kernel.org;
>>> linux-hwmon@...r.kernel.org; linux-kernel@...r.kernel.org;
>>> platform-driver-x86@...r.kernel.org
>>> Subject: [PATCH v2] ACPI: fan: Add hwmon support
>>>
>>> Caution: External email. Do not click links or open attachments
>>> unless you recognize the sender and know the content is safe.
>>>
>>>
>>> Currently, the driver does only support a custom sysfs to allow
>>> userspace to read the fan speed.
>>> Add support for the standard hwmon interface so users can read the
>>> fan speed with standard tools like "sensors".
>>>
>>> Compile-tested only.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@....de>
>>> ---
>>> Changes since v1:
>>> - fix undefined reference error
>>> - fix fan speed validation
>>> - coding style fixes
>>> - clarify that the changes are compile-tested only
>>> - add hwmon maintainers to cc list
>>>
>>> The changes will be tested by Mikael Lund Jepsen from Danelec and
>>> should be merged only after those tests.
>>> ---
>>> drivers/acpi/Makefile | 1 +
>>> drivers/acpi/fan.h | 9 +++++
>>> drivers/acpi/fan_core.c | 4 ++
>>> drivers/acpi/fan_hwmon.c | 83 ++++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 97 insertions(+)
>>> create mode 100644 drivers/acpi/fan_hwmon.c
>>>
>>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
>>> d69d5444acdb..c272ab2c93b9 100644
>>> --- a/drivers/acpi/Makefile
>>> +++ b/drivers/acpi/Makefile
>>> @@ -83,6 +83,7 @@ obj-$(CONFIG_ACPI_TINY_POWER_BUTTON) +=
>>> tiny-power-button.o
>>> obj-$(CONFIG_ACPI_FAN) += fan.o
>>> fan-objs := fan_core.o
>>> fan-objs += fan_attr.o
>>> +fan-$(CONFIG_HWMON) += fan_hwmon.o
>>>
>>> obj-$(CONFIG_ACPI_VIDEO) += video.o
>>> obj-$(CONFIG_ACPI_TAD) += acpi_tad.o
>>> diff --git a/drivers/acpi/fan.h b/drivers/acpi/fan.h index
>>> e7b4b4e4a55e..97863bdb6303 100644
>>> --- a/drivers/acpi/fan.h
>>> +++ b/drivers/acpi/fan.h
>>> @@ -10,6 +10,8 @@
>>> #ifndef _ACPI_FAN_H_
>>> #define _ACPI_FAN_H_
>>>
>>> +#include <linux/kconfig.h>
>>> +
>>> #define ACPI_FAN_DEVICE_IDS \
>>> {"INT3404", }, /* Fan */ \
>>> {"INTC1044", }, /* Fan for Tiger Lake generation */ \ @@
>>> -56,4 +58,11 @@ struct acpi_fan { int acpi_fan_get_fst(struct
>>> acpi_device *device, struct acpi_fan_fst *fst); int
>>> acpi_fan_create_attributes(struct acpi_device *device); void
>>> acpi_fan_delete_attributes(struct acpi_device *device);
>>> +
>>> +#if IS_REACHABLE(CONFIG_HWMON)
>>> +int devm_acpi_fan_create_hwmon(struct acpi_device *device); #else
>>> +static inline int devm_acpi_fan_create_hwmon(struct acpi_device
>>> +*device) { return 0; }; #endif
>>> +
>>> #endif
>>> diff --git a/drivers/acpi/fan_core.c b/drivers/acpi/fan_core.c index
>>> ff72e4ef8738..7cea4495f19b 100644
>>> --- a/drivers/acpi/fan_core.c
>>> +++ b/drivers/acpi/fan_core.c
>>> @@ -336,6 +336,10 @@ static int acpi_fan_probe(struct
>>> platform_device *pdev)
>>> if (result)
>>> return result;
>>>
>>> + result = devm_acpi_fan_create_hwmon(device);
>>> + if (result)
>>> + return result;
>>> +
>>> result = acpi_fan_create_attributes(device);
>>> if (result)
>>> return result;
>>> diff --git a/drivers/acpi/fan_hwmon.c b/drivers/acpi/fan_hwmon.c new
>>> file mode 100644 index 000000000000..b01055432ded
>>> --- /dev/null
>>> +++ b/drivers/acpi/fan_hwmon.c
>>> @@ -0,0 +1,83 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * fan_hwmon.c - hwmon interface for the ACPI Fan driver
>>> + *
>>> + * Copyright (C) 2024 Armin Wolf <W_Armin@....de> */
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/limits.h>
>>> +
>>> +#include "fan.h"
>>> +
>>> +/* Returned when the ACPI fan does not support speed reporting */
>>> +#define FAN_SPEED_UNAVAILABLE 0xffffffff
>>> +
>>> +static umode_t acpi_fan_is_visible(const void *drvdata, enum
>>> hwmon_sensor_types type, u32 attr,
>>> + int channel) {
>>> + return 0444;
>>> +}
>>> +
>>> +static int acpi_fan_read(struct device *dev, enum
>>> hwmon_sensor_types type, u32 attr, int channel,
>>> + long *val)
>>> +{
>>> + struct acpi_device *adev = dev_get_drvdata(dev);
>>> + struct acpi_fan_fst fst;
>>> + int ret;
>>> +
>>> + switch (type) {
>>> + case hwmon_fan:
>>> + ret = acpi_fan_get_fst(adev, &fst);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + switch (attr) {
>>> + case hwmon_fan_input:
>>> + if (fst.speed == FAN_SPEED_UNAVAILABLE)
>>> + return -ENODATA;
>>> +
>>> + if (fst.speed > LONG_MAX)
>>> + return -EOVERFLOW;
>>> +
>>> + *val = fst.speed;
>>> + return 0;
>>> + case hwmon_fan_fault:
>>> + *val = (fst.speed == FAN_SPEED_UNAVAILABLE);
>>> + return 0;
>>> + default:
>>> + break;
>>> + }
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static const struct hwmon_ops acpi_fan_ops = {
>>> + .is_visible = acpi_fan_is_visible,
>>> + .read = acpi_fan_read,
>>> +};
>>> +
>>> +static const struct hwmon_channel_info * const acpi_fan_info[] = {
>>> + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT),
>>> + NULL
>>> +};
>>> +
>>> +static const struct hwmon_chip_info acpi_fan_chip_info = {
>>> + .ops = &acpi_fan_ops,
>>> + .info = acpi_fan_info,
>>> +};
>>> +
>>> +int devm_acpi_fan_create_hwmon(struct acpi_device *device) {
>>> + struct device *hdev;
>>> +
>>> + hdev = devm_hwmon_device_register_with_info(&device->dev,
>>> "acpi_fan", device,
>>> + &acpi_fan_chip_info,
>>> + NULL);
>>> +
>>> + return PTR_ERR_OR_ZERO(hdev);
>>> +}
>>> --
>>> 2.39.2
>>>
>> Sorry about the delay, new to ACPI and needed to create the missing
>> tables to define the fan as an ACPIv4 fan and make it talk to the
>> pse/eclite firmware.
>>
>> I've tested patch v2 on kernel 6.6.15 on my Intel ElkhartLake CRB
>> board, and it works fine for me:
>> Fan tacho value is shown correctly and FAULT is reported if
>> ishtp_eclite driver is not loaded.
>>
>> For reference, my test setup:
>> - Intel ElkhartLake CRB board w/ Intel Atom x6425RE
>> - Slimbootloader: Intel MR7 release, with custom ACPI definitions for
>> the fan, and If I remember correctly, I also needed to correct some
>> TGPIO softstraps compared to release defaults
>> - PseFW: Intel MR7 release, with fixes to enable TGPIO/Tacho reading
>> (which was not enabled in release defaults) and to make sure tacho
>> value is read even when fan is turning off (default PSE FW skipped
>> reading tacho if control value was 0, so last tacho value read while
>> fan was turned on would persist).
>>
>> One thing that occurred to me: The fan control value is reported in
>> _FST, but not used in acpi-fan, so how can we flag an error in hwmon
>> if the fan is broken, but shall not always be running?
>> If the control value was exported, then we can alert if tacho is zero
>> when control > 0. I think I'm missing something here?
>>
>> Thanks,
>> Mikael
>
> You are right, i can expose the target RPM as fan1_target when the fan
> is not in fine-grained control mode (otherwise there is no guarantee
> that each
> control value has an associated fan state entry).
>
> This way, userspace can detect when the fan is stuck. I will send a v3
> soon.
>
> Armin Wolf
>
I just noticed that the drvdata argument of the is_visible callback is marked as const, so i cannot use dev_get_drvdata() on the resulting ACPI device.
Guenter, would it be ok to make drvdata non-const in a separate patch series?
Thanks,
Armin Wolf
Powered by blists - more mailing lists