| 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: <0053b236-79cc-496d-936b-5f8b12b39f10@gmx.de>
Date: Fri, 14 Feb 2025 17:46:50 +0100
From: Armin Wolf <W_Armin@....de>
To: Joshua Grisham <josh@...huagrisham.com>
Cc: rafael@...nel.org, lenb@...nel.org, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] ACPI: fan: Add fan speed reporting for fans with only
_FST
Am 06.02.25 um 08:37 schrieb Joshua Grisham:
> Den tors 6 feb. 2025 kl 06:05 skrev Armin Wolf <W_Armin@....de>:
>> Am 25.01.25 um 11:07 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].
>> Any updates on this patch? For me it seems ready for mainline.
>>
>> Thanks,
>> Armin Wolf
>>
> Hi Armin, thanks for checking in on this!
>
> For me I have no further updates that I planned or intended to send.
> If it looks good to Rafael or anyone else who wants or needs to review
> then I would be glad to see it applied.
>
> If needed then I can re-send with Armin's Reviewed-by tag inline in
> the commit message but otherwise everything is as I would have wished
> it to be, for what that is worth :)
>
> Thanks again!
>
> Best regards,
> Joshua
I was hoping to get Rafaels attention so that he can give your patch a closer look.
Thanks,
Armin Wolf
>>> [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>
>>> ---
>>>
>>> v1->v2:
>>> - Still allow acpi4_only_fst fans to update power state on
>>> suspend/resume
>>> - Fix if / else if logic error
>>> - Also hide hwmon_power_input for acpi4_only_fst fans
>>>
>>> v2->v3:
>>> - Still allow acpi4_only_fst fans to set initial power state on probe
>>> ---
>>> drivers/acpi/fan.h | 1 +
>>> drivers/acpi/fan_attr.c | 37 ++++++++++++++++++++++---------------
>>> drivers/acpi/fan_core.c | 22 +++++++++++++++++-----
>>> drivers/acpi/fan_hwmon.c | 12 ++++++++++++
>>> 4 files changed, 52 insertions(+), 20 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..66aa1be64 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,9 @@ static int acpi_fan_probe(struct platform_device *pdev)
>>> result = acpi_fan_get_fps(device);
>>> if (result)
>>> return result;
>>> + }
>>>
>>> + if (fan->acpi4 || fan->acpi4_only_fst) {
>>> result = devm_acpi_fan_create_hwmon(device);
>>> if (result)
>>> return result;
>>> @@ -343,9 +355,9 @@ static int acpi_fan_probe(struct platform_device *pdev)
>>> result = acpi_fan_create_attributes(device);
>>> if (result)
>>> return result;
>>> + }
>>>
>>> - fan->acpi4 = true;
>>> - } else {
>>> + if (!fan->acpi4) {
>>> result = acpi_device_update_power(device, NULL);
>>> if (result) {
>>> dev_err(&device->dev, "Failed to set initial power state\n");
>>> @@ -391,7 +403,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 +413,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);
>>> diff --git a/drivers/acpi/fan_hwmon.c b/drivers/acpi/fan_hwmon.c
>>> index bd0d31a39..d0668ecc2 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;
>>> +
>>> /*
>>> * When in fine grain control mode, not every fan control value
>>> * has an associated fan performance state.
>>> @@ -57,6 +63,12 @@ static umode_t acpi_fan_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_
>>> case hwmon_power:
>>> switch (attr) {
>>> case hwmon_power_input:
>>> + /*
>>> + * Fans with only _FST do not support fan control.
>>> + */
>>> + if (fan->acpi4_only_fst)
>>> + return 0;
>>> +
>>> /*
>>> * When in fine grain control mode, not every fan control value
>>> * has an associated fan performance state.
Powered by blists - more mailing lists