| 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: <CAMF+Kea0g1W=P92mNUcimGHXfMbBq=dmpoQ50cheaSHFaZrirg@mail.gmail.com>
Date: Sat, 22 Feb 2025 09:54:17 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: lenb@...nel.org, W_Armin@....de, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI: fan: Add fan speed reporting for fans with only _FST
Hi Rafael, thanks for taking a look! Comments/questions inline below... :)
Den tis 18 feb. 2025 kl 17:49 skrev Rafael J. Wysocki <rafael@...nel.org>:
>
> On Tue, Jan 14, 2025 at 2:22 AM Joshua Grisham <josh@...huagrisham.com> wrote:
> >
> > 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;
>
> I would use has_fst instead of this, that is
>
> + bool has_fst;
>
> which would be true when acpi4 is true, but not necessarily the other
> way around.
>
No problem at all with this, I actually struggled a lot with what to
call this (or to change the flag to use different bits instead even
but did not want to rock the boat tooooo much ;) ) .. I still think
naming is probably one of the hardest things in software!
> > 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;
>
> So the above may become
>
> if (!fan->acpi4)
> return 0;
>
Yes I think this is also better and feels more "self-documenting."
> > +
> > + 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;
>
> And here one could do
>
> if (acpi_fan_has_fst(device)) {
> fan->has_fst = true;
> fan->acpi4 = acpi_fan_is_acpi4(device);
> }
>
> and if I'm not mistaken, the check for _FST presence could be dropped
> from acpi_fan_is_acpi4().
>
Yes if you only call acpi_fan_is_acpi4() after you have already gotten
positive from acpi_fan_has_fst() then "has _FST" is already baked in.
Maybe a bit less obvious if you only look at the code within the
function acpi_fan_is_acpi4() but more efficient and less code will be
executed overall so if you are ok with this then I will change it like
this :)
> > +
> > + 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) {
>
> Then, all of the checks like the above could be replaced with
> fan->has_fst checks.
>
Yes and looks nicer!
> > 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;
> >
> > 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.
> > + */
>
> Nit: One-line comment here, please.
>
Yes this one is the easiest by far ;) I think it was more that I
copy-pasted from other comments within the switch and kept it the same
"style" but agree this new one is short enough to be one line. I will
probably anyway change the comment a bit to fit better with the change
to `(!fan->acpi4)` per your comment below but will see what I can do!
I will get these tweaks in as a v4 to this patch shortly.
Thanks again!
Regards,
Joshua
> > + if (fan->acpi4_only_fst)
>
> And this would become
>
> if (!fan->acpi4)
>
> > + 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