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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6573673e-7915-46c9-ad26-0364da30f564@gmx.de>
Date: Wed, 28 Aug 2024 00:03:46 +0200
From: Armin Wolf <W_Armin@....de>
To: Hans de Goede <hdegoede@...hat.com>, 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

Am 27.08.24 um 10:33 schrieb Hans de Goede:

> 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

Thank you :)

>
>>>> ---
>>>> 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

Powered by Openwall GNU/*/Linux Powered by OpenVZ