| 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: <154d5d69-09c3-460a-945b-3f4f3b452d9d@redhat.com>
Date: Tue, 27 Aug 2024 10:33:17 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Armin Wolf <W_Armin@....de>, matan@...alib.org, ghostwind@...il.com
Cc: ilpo.jarvinen@...ux.intel.com, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] platform/x86: lg-laptop: Add operation region support
Hi Armin,
On 8/26/24 10:09 PM, Armin Wolf wrote:
> Am 19.08.24 um 13:37 schrieb Hans de Goede:
>
>> Hi,
>>
>> On 8/13/24 4:29 AM, Armin Wolf wrote:
>>> The LEGX0820 ACPI device is expected to provide a custom operation
>>> region:
>>>
>>> OperationRegion (XIN1, 0x8F, Zero, 0x04B0)
>>> Field (XIN1, AnyAcc, Lock, Preserve)
>>> {
>>> DMSG, 8,
>>> HDAP, 8,
>>> Offset (0x03),
>>> AFNM, 8,
>>> Offset (0x10),
>>> P80B, 8,
>>> P81B, 8,
>>> P82B, 8,
>>> P83B, 8,
>>> P84B, 8,
>>> P85B, 8,
>>> P86B, 8,
>>> P87B, 8,
>>> Offset (0x20),
>>> DTTM, 8,
>>> TMP1, 8,
>>> LTP1, 8,
>>> HTP1, 8,
>>> TMP2, 8,
>>> LTP2, 8,
>>> HTP2, 8,
>>> Offset (0x3E8),
>>> PMSG, 1600
>>> }
>>>
>>> The PMSG field is used by AML code to log debug messages when DMSG is
>>> true. Since those debug messages are already logged using the standard
>>> ACPI Debug object, we set DMSG unconditionally to 0x00 and ignore any
>>> writes to PMSG.
>>>
>>> The TMPx, LTPx, HTPx and AFNM fields are used to inform the driver when
>>> the temperature/(presumably) trip points/fan mode changes. This only
>>> happens when the DTTM flag is set.
>>>
>>> Unfortunately we have to implement support for this operation region
>>> because the AML codes uses code constructs like this one:
>>>
>>> If (((\_SB.XINI.PLAV != Zero) && (\_SB.XINI.DTTM != Zero)))
>>>
>>> The PLAV field gets set to 1 when the driver registers its address space
>>> handler, so by default XIN1 should not be accessed.
>>>
>>> However ACPI does not use short-circuit evaluation when evaluating
>>> logical conditions. This causes the DTTM field to be accessed even
>>> when PLAV is 0, which results in an ACPI error.
>>> Since this check happens inside various thermal-related ACPI control
>>> methods, various thermal zone become unusable since any attempt to
>>> read their temperature results in an ACPI error.
>>>
>>> Fix this by providing support for this operation region. I suspect
>>> that the problem does not happen under Windows (which seemingly does
>>> not use short-circuit evaluation either) because the necessary driver
>>> comes preinstalled with the machine.
>>>
>>> Tested-by: Chris <ghostwind@...il.com>
>>> Signed-off-by: Armin Wolf <W_Armin@....de>
>> Thanks, patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@...hat.com>
>>
>> Regards,
>>
>> Hans
>>
> Any updates on this? I would prefer to have this merged for the upcoming 6.12 kernel since
> without this patch many LG notebooks have unusable thermal zones.
This patch has already been merged and already is in linux-next,
but I see that I forgot to send my usual email confirming that
I merged it, sorry.
Regards,
Hans
>>> ---
>>> Changes since v1:
>>> - attempts -> attempt
>>> - sort defines in numerical order
>>> - make lg_laptop_address_space_write() take a plain u64
>>> - use BITS_PER_BYTE
>>> - manually check fw_debug when handling fan mode updates
>>> ---
>>> drivers/platform/x86/lg-laptop.c | 136 +++++++++++++++++++++++++++++++
>>> 1 file changed, 136 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
>>> index 9c7857842caf..55d31d4fefd6 100644
>>> --- a/drivers/platform/x86/lg-laptop.c
>>> +++ b/drivers/platform/x86/lg-laptop.c
>>> @@ -8,6 +8,9 @@
>>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>
>>> #include <linux/acpi.h>
>>> +#include <linux/bits.h>
>>> +#include <linux/device.h>
>>> +#include <linux/dev_printk.h>
>>> #include <linux/dmi.h>
>>> #include <linux/input.h>
>>> #include <linux/input/sparse-keymap.h>
>>> @@ -31,6 +34,26 @@ MODULE_AUTHOR("Matan Ziv-Av");
>>> MODULE_DESCRIPTION("LG WMI Hotkey Driver");
>>> MODULE_LICENSE("GPL");
>>>
>>> +static bool fw_debug;
>>> +module_param(fw_debug, bool, 0);
>>> +MODULE_PARM_DESC(fw_debug, "Enable printing of firmware debug messages");
>>> +
>>> +#define LG_ADDRESS_SPACE_ID 0x8F
>>> +
>>> +#define LG_ADDRESS_SPACE_DEBUG_FLAG_ADR 0x00
>>> +#define LG_ADDRESS_SPACE_FAN_MODE_ADR 0x03
>>> +
>>> +#define LG_ADDRESS_SPACE_DTTM_FLAG_ADR 0x20
>>> +#define LG_ADDRESS_SPACE_CPU_TEMP_ADR 0x21
>>> +#define LG_ADDRESS_SPACE_CPU_TRIP_LOW_ADR 0x22
>>> +#define LG_ADDRESS_SPACE_CPU_TRIP_HIGH_ADR 0x23
>>> +#define LG_ADDRESS_SPACE_MB_TEMP_ADR 0x24
>>> +#define LG_ADDRESS_SPACE_MB_TRIP_LOW_ADR 0x25
>>> +#define LG_ADDRESS_SPACE_MB_TRIP_HIGH_ADR 0x26
>>> +
>>> +#define LG_ADDRESS_SPACE_DEBUG_MSG_START_ADR 0x3E8
>>> +#define LG_ADDRESS_SPACE_DEBUG_MSG_END_ADR 0x5E8
>>> +
>>> #define WMI_EVENT_GUID0 "E4FB94F9-7F2B-4173-AD1A-CD1D95086248"
>>> #define WMI_EVENT_GUID1 "023B133E-49D1-4E10-B313-698220140DC2"
>>> #define WMI_EVENT_GUID2 "37BE1AC0-C3F2-4B1F-BFBE-8FDEAF2814D6"
>>> @@ -646,6 +669,107 @@ static struct platform_driver pf_driver = {
>>> }
>>> };
>>>
>>> +static acpi_status lg_laptop_address_space_write(struct device *dev, acpi_physical_address address,
>>> + size_t size, u64 value)
>>> +{
>>> + u8 byte;
>>> +
>>> + /* Ignore any debug messages */
>>> + if (address >= LG_ADDRESS_SPACE_DEBUG_MSG_START_ADR &&
>>> + address <= LG_ADDRESS_SPACE_DEBUG_MSG_END_ADR)
>>> + return AE_OK;
>>> +
>>> + if (size != sizeof(byte))
>>> + return AE_BAD_PARAMETER;
>>> +
>>> + byte = value & 0xFF;
>>> +
>>> + switch (address) {
>>> + case LG_ADDRESS_SPACE_FAN_MODE_ADR:
>>> + /*
>>> + * The fan mode field is not affected by the DTTM flag, so we
>>> + * have to manually check fw_debug.
>>> + */
>>> + if (fw_debug)
>>> + dev_dbg(dev, "Fan mode set to mode %u\n", byte);
>>> +
>>> + return AE_OK;
>>> + case LG_ADDRESS_SPACE_CPU_TEMP_ADR:
>>> + dev_dbg(dev, "CPU temperature is %u °C\n", byte);
>>> + return AE_OK;
>>> + case LG_ADDRESS_SPACE_CPU_TRIP_LOW_ADR:
>>> + dev_dbg(dev, "CPU lower trip point set to %u °C\n", byte);
>>> + return AE_OK;
>>> + case LG_ADDRESS_SPACE_CPU_TRIP_HIGH_ADR:
>>> + dev_dbg(dev, "CPU higher trip point set to %u °C\n", byte);
>>> + return AE_OK;
>>> + case LG_ADDRESS_SPACE_MB_TEMP_ADR:
>>> + dev_dbg(dev, "Motherboard temperature is %u °C\n", byte);
>>> + return AE_OK;
>>> + case LG_ADDRESS_SPACE_MB_TRIP_LOW_ADR:
>>> + dev_dbg(dev, "Motherboard lower trip point set to %u °C\n", byte);
>>> + return AE_OK;
>>> + case LG_ADDRESS_SPACE_MB_TRIP_HIGH_ADR:
>>> + dev_dbg(dev, "Motherboard higher trip point set to %u °C\n", byte);
>>> + return AE_OK;
>>> + default:
>>> + dev_notice_ratelimited(dev, "Ignoring write to unknown opregion address %llu\n",
>>> + address);
>>> + return AE_OK;
>>> + }
>>> +}
>>> +
>>> +static acpi_status lg_laptop_address_space_read(struct device *dev, acpi_physical_address address,
>>> + size_t size, u64 *value)
>>> +{
>>> + if (size != 1)
>>> + return AE_BAD_PARAMETER;
>>> +
>>> + switch (address) {
>>> + case LG_ADDRESS_SPACE_DEBUG_FLAG_ADR:
>>> + /* Debug messages are already printed using the standard ACPI Debug object */
>>> + *value = 0x00;
>>> + return AE_OK;
>>> + case LG_ADDRESS_SPACE_DTTM_FLAG_ADR:
>>> + *value = fw_debug;
>>> + return AE_OK;
>>> + default:
>>> + dev_notice_ratelimited(dev, "Attempt to read unknown opregion address %llu\n",
>>> + address);
>>> + return AE_BAD_PARAMETER;
>>> + }
>>> +}
>>> +
>>> +static acpi_status lg_laptop_address_space_handler(u32 function, acpi_physical_address address,
>>> + u32 bits, u64 *value, void *handler_context,
>>> + void *region_context)
>>> +{
>>> + struct device *dev = handler_context;
>>> + size_t size;
>>> +
>>> + if (bits % BITS_PER_BYTE)
>>> + return AE_BAD_PARAMETER;
>>> +
>>> + size = bits / BITS_PER_BYTE;
>>> +
>>> + switch (function) {
>>> + case ACPI_READ:
>>> + return lg_laptop_address_space_read(dev, address, size, value);
>>> + case ACPI_WRITE:
>>> + return lg_laptop_address_space_write(dev, address, size, *value);
>>> + default:
>>> + return AE_BAD_PARAMETER;
>>> + }
>>> +}
>>> +
>>> +static void lg_laptop_remove_address_space_handler(void *data)
>>> +{
>>> + struct acpi_device *device = data;
>>> +
>>> + acpi_remove_address_space_handler(device->handle, LG_ADDRESS_SPACE_ID,
>>> + &lg_laptop_address_space_handler);
>>> +}
>>> +
>>> static int acpi_add(struct acpi_device *device)
>>> {
>>> struct platform_device_info pdev_info = {
>>> @@ -653,6 +777,7 @@ static int acpi_add(struct acpi_device *device)
>>> .name = PLATFORM_NAME,
>>> .id = PLATFORM_DEVID_NONE,
>>> };
>>> + acpi_status status;
>>> int ret;
>>> const char *product;
>>> int year = 2017;
>>> @@ -660,6 +785,17 @@ static int acpi_add(struct acpi_device *device)
>>> if (pf_device)
>>> return 0;
>>>
>>> + status = acpi_install_address_space_handler(device->handle, LG_ADDRESS_SPACE_ID,
>>> + &lg_laptop_address_space_handler,
>>> + NULL, &device->dev);
>>> + if (ACPI_FAILURE(status))
>>> + return -ENODEV;
>>> +
>>> + ret = devm_add_action_or_reset(&device->dev, lg_laptop_remove_address_space_handler,
>>> + device);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> ret = platform_driver_register(&pf_driver);
>>> if (ret)
>>> return ret;
>>> --
>>> 2.39.2
>>>
>>
>
Powered by blists - more mailing lists