| 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: <6c1dbd29-71fb-4714-a661-7539408874d3@gmx.de>
Date: Thu, 23 Jan 2025 01:49:49 +0100
From: Armin Wolf <W_Armin@....de>
To: Joshua Grisham <josh@...huagrisham.com>, rafael@...nel.org,
lenb@...nel.org, linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI: fan: Add fan speed reporting for fans with only
_FST
Am 14.01.25 um 02:21 schrieb Joshua Grisham:
> Add support for ACPI fans with _FST to report their speed even if they do
> not support fan control.
>
> As suggested by Armin Wolf [1] and per the Windows Thermal Management
> Design Guide [2], Samsung Galaxy Book series devices (and possibly many
> more devices where the Windows guide was strictly followed) only implement
> the _FST method and do not support ACPI-based fan control.
>
> Currently, these fans are not supported by the kernel driver but this patch
> will make some very small adjustments to allow them to be supported.
>
> This patch is tested and working for me on a Samsung Galaxy Book2 Pro whose
> DSDT (and several other Samsung Galaxy Book series notebooks which
> currently have the same issue) can be found at [3].
>
> [1]: https://lore.kernel.org/platform-driver-x86/53c5075b-1967-45d0-937f-463912dd966d@gmx.de
> [2]: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/design-guide
> [3]: https://github.com/joshuagrisham/samsung-galaxybook-extras/tree/8e3087a06b8bdcdfdd081367af4b744a56cc4ee9/dsdt
>
> Signed-off-by: Joshua Grisham <josh@...huagrisham.com>
> ---
> drivers/acpi/fan.h | 1 +
> drivers/acpi/fan_attr.c | 37 ++++++++++++++++++++++---------------
> drivers/acpi/fan_core.c | 24 ++++++++++++++++--------
> drivers/acpi/fan_hwmon.c | 6 ++++++
> 4 files changed, 45 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/fan.h b/drivers/acpi/fan.h
> index 488b51e2c..d0aad88a7 100644
> --- a/drivers/acpi/fan.h
> +++ b/drivers/acpi/fan.h
> @@ -49,6 +49,7 @@ struct acpi_fan_fst {
>
> struct acpi_fan {
> bool acpi4;
> + bool acpi4_only_fst;
> struct acpi_fan_fif fif;
> struct acpi_fan_fps *fps;
> int fps_count;
> diff --git a/drivers/acpi/fan_attr.c b/drivers/acpi/fan_attr.c
> index f4f6e2381..d83f88429 100644
> --- a/drivers/acpi/fan_attr.c
> +++ b/drivers/acpi/fan_attr.c
> @@ -75,15 +75,6 @@ int acpi_fan_create_attributes(struct acpi_device *device)
> struct acpi_fan *fan = acpi_driver_data(device);
> int i, status;
>
> - sysfs_attr_init(&fan->fine_grain_control.attr);
> - fan->fine_grain_control.show = show_fine_grain_control;
> - fan->fine_grain_control.store = NULL;
> - fan->fine_grain_control.attr.name = "fine_grain_control";
> - fan->fine_grain_control.attr.mode = 0444;
> - status = sysfs_create_file(&device->dev.kobj, &fan->fine_grain_control.attr);
> - if (status)
> - return status;
> -
> /* _FST is present if we are here */
> sysfs_attr_init(&fan->fst_speed.attr);
> fan->fst_speed.show = show_fan_speed;
> @@ -92,7 +83,19 @@ int acpi_fan_create_attributes(struct acpi_device *device)
> fan->fst_speed.attr.mode = 0444;
> status = sysfs_create_file(&device->dev.kobj, &fan->fst_speed.attr);
> if (status)
> - goto rem_fine_grain_attr;
> + return status;
> +
> + if (fan->acpi4_only_fst)
> + return 0;
> +
> + sysfs_attr_init(&fan->fine_grain_control.attr);
> + fan->fine_grain_control.show = show_fine_grain_control;
> + fan->fine_grain_control.store = NULL;
> + fan->fine_grain_control.attr.name = "fine_grain_control";
> + fan->fine_grain_control.attr.mode = 0444;
> + status = sysfs_create_file(&device->dev.kobj, &fan->fine_grain_control.attr);
> + if (status)
> + goto rem_fst_attr;
>
> for (i = 0; i < fan->fps_count; ++i) {
> struct acpi_fan_fps *fps = &fan->fps[i];
> @@ -109,18 +112,18 @@ int acpi_fan_create_attributes(struct acpi_device *device)
>
> for (j = 0; j < i; ++j)
> sysfs_remove_file(&device->dev.kobj, &fan->fps[j].dev_attr.attr);
> - goto rem_fst_attr;
> + goto rem_fine_grain_attr;
> }
> }
>
> return 0;
>
> -rem_fst_attr:
> - sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
> -
> rem_fine_grain_attr:
> sysfs_remove_file(&device->dev.kobj, &fan->fine_grain_control.attr);
>
> +rem_fst_attr:
> + sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
> +
> return status;
> }
>
> @@ -129,9 +132,13 @@ void acpi_fan_delete_attributes(struct acpi_device *device)
> struct acpi_fan *fan = acpi_driver_data(device);
> int i;
>
> + sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
> +
> + if (fan->acpi4_only_fst)
> + return;
> +
> for (i = 0; i < fan->fps_count; ++i)
> sysfs_remove_file(&device->dev.kobj, &fan->fps[i].dev_attr.attr);
>
> - sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
> sysfs_remove_file(&device->dev.kobj, &fan->fine_grain_control.attr);
> }
> diff --git a/drivers/acpi/fan_core.c b/drivers/acpi/fan_core.c
> index 10016f52f..b51b1481c 100644
> --- a/drivers/acpi/fan_core.c
> +++ b/drivers/acpi/fan_core.c
> @@ -211,6 +211,11 @@ static bool acpi_fan_is_acpi4(struct acpi_device *device)
> acpi_has_method(device->handle, "_FST");
> }
>
> +static bool acpi_fan_has_fst(struct acpi_device *device)
> +{
> + return acpi_has_method(device->handle, "_FST");
> +}
> +
> static int acpi_fan_get_fif(struct acpi_device *device)
> {
> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -327,7 +332,12 @@ static int acpi_fan_probe(struct platform_device *pdev)
> device->driver_data = fan;
> platform_set_drvdata(pdev, fan);
>
> - if (acpi_fan_is_acpi4(device)) {
> + if (acpi_fan_is_acpi4(device))
> + fan->acpi4 = true;
> + else if (acpi_fan_has_fst(device))
> + fan->acpi4_only_fst = true;
> +
> + if (fan->acpi4) {
> result = acpi_fan_get_fif(device);
> if (result)
> return result;
> @@ -335,7 +345,7 @@ static int acpi_fan_probe(struct platform_device *pdev)
> result = acpi_fan_get_fps(device);
> if (result)
> return result;
> -
> + } else if (fan->acpi4 || fan->acpi4_only_fst) {
Hi,
this will not register any attributes or the hwmon interface if "fan->acpi4" is true.
I think the "else" is misplaced here.
> result = devm_acpi_fan_create_hwmon(device);
> if (result)
> return result;
> @@ -343,8 +353,6 @@ static int acpi_fan_probe(struct platform_device *pdev)
> result = acpi_fan_create_attributes(device);
> if (result)
> return result;
> -
> - fan->acpi4 = true;
> } else {
> result = acpi_device_update_power(device, NULL);
> if (result) {
> @@ -391,7 +399,7 @@ static int acpi_fan_probe(struct platform_device *pdev)
> err_unregister:
> thermal_cooling_device_unregister(cdev);
> err_end:
> - if (fan->acpi4)
> + if (fan->acpi4 || fan->acpi4_only_fst)
> acpi_fan_delete_attributes(device);
>
> return result;
> @@ -401,7 +409,7 @@ static void acpi_fan_remove(struct platform_device *pdev)
> {
> struct acpi_fan *fan = platform_get_drvdata(pdev);
>
> - if (fan->acpi4) {
> + if (fan->acpi4 || fan->acpi4_only_fst) {
> struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>
> acpi_fan_delete_attributes(device);
> @@ -415,7 +423,7 @@ static void acpi_fan_remove(struct platform_device *pdev)
> static int acpi_fan_suspend(struct device *dev)
> {
> struct acpi_fan *fan = dev_get_drvdata(dev);
> - if (fan->acpi4)
> + if (fan->acpi4 || fan->acpi4_only_fst)
> return 0;
>
> acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
> @@ -428,7 +436,7 @@ static int acpi_fan_resume(struct device *dev)
> int result;
> struct acpi_fan *fan = dev_get_drvdata(dev);
>
> - if (fan->acpi4)
> + if (fan->acpi4 || fan->acpi4_only_fst)
> return 0;
The Windows design guide says:
"A fan that implements the _FST object is not required to be in a thermal
zone's _ALx device list, but it can, as an option, be in this list.
This option enables a hybrid solution in which a fan is typically
controlled by a third-party driver, but can be controlled by the OS
thermal zone if the third-party driver is not installed. If a fan is in
an _ALx device list and is engaged by the thermal zone (placed in D0),
the _FST object is required to indicate a nonzero Control value."
Because this i think the suspend/resume path should still change the power state of the fan device.
>
> result = acpi_device_update_power(ACPI_COMPANION(dev), NULL);
> diff --git a/drivers/acpi/fan_hwmon.c b/drivers/acpi/fan_hwmon.c
> index bd0d31a39..87bee018c 100644
> --- a/drivers/acpi/fan_hwmon.c
> +++ b/drivers/acpi/fan_hwmon.c
> @@ -43,6 +43,12 @@ static umode_t acpi_fan_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_
> case hwmon_fan_input:
> return 0444;
> case hwmon_fan_target:
> + /*
> + * Fans with only _FST do not support fan control.
> + */
> + if (fan->acpi4_only_fst)
> + return 0;
The same needs to be done for hwmon_power_input, since currently the code assumes that the _FIF
ACPI control method is always present.
Other than that the patch looks promising.
Thanks,
Armin Wolf
> +
> /*
> * When in fine grain control mode, not every fan control value
> * has an associated fan performance state.
Powered by blists - more mailing lists