Re: [PATCH] platform/x86: lg-laptop: Add operation region support

[Armin Wolf <W_Armin@gmx.de>]

Am 12.08.24 um 14:19 schrieb Ilpo Järvinen:

> On Fri, 9 Aug 2024, 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 attempts to
> attempt
>
>> 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@gmail.com>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/lg-laptop.c | 128 +++++++++++++++++++++++++++++++
>>   1 file changed, 128 insertions(+)
>>
>> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
>> index 9c7857842caf..6310a23f808c 100644
>> --- a/drivers/platform/x86/lg-laptop.c
>> +++ b/drivers/platform/x86/lg-laptop.c
>> @@ -8,6 +8,8 @@
>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>>   #include <linux/acpi.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 +33,25 @@ 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_DEBUG_MSG_START_ADR	0x3E8
>> +#define LG_ADDRESS_SPACE_DEBUG_MSG_END_ADR	0x5E8
>> +
>> +#define LG_ADDRESS_SPACE_DTTM_FLAG_ADR		0x20
>> +#define LG_ADDRESS_SPACE_FAN_MODE_ADR		0x03
> Any particular reason why there are not in numerical order?

Hi,

i decided to group all fields connected to the DTTM flag together, but if you wish i can
sort them in numerical order.

>> +#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 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 +667,101 @@ 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)
> Why is this write function taking u64 _pointer_?

Good point, i will change that.

>
>> +{
>> +	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:
>> +		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 % 8)
>> +		return AE_BAD_PARAMETER;
>> +
>> +	size = bits / 8;
> BITS_PER_BYTE x2
>