Re: [PATCH v4 1/2] platform/x86: wmi: Support reading/writing 16 bit EC values

[Armin Wolf <W_Armin@gmx.de>]

Am 09.03.24 um 18:07 schrieb Kuppuswamy Sathyanarayanan:

> On 3/8/24 1:05 PM, Armin Wolf wrote:
>> The ACPI EC address space handler currently only supports
>> reading/writing 8 bit values. Some firmware implementations however
>> want to access for example 16 bit values, which is prefectly legal
>> according to the ACPI spec.
>>
>> Add support for reading/writing such values.
>>
>> Tested on a Dell Inspiron 3505 and a Asus Prime B650-Plus.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> Changes since v3:
>> - change type of variable i to size_t
>>
>> Changes since v2:
>> - fix address overflow check
>>
>> Changes since v1:
>> - use BITS_PER_BYTE
>> - validate that number of bytes to read/write does not overflow the
>>    address
>> ---
>>   drivers/platform/x86/wmi.c | 49 ++++++++++++++++++++++++++++++--------
>>   1 file changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index 1920e115da89..d9bf6d452b3a 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -1153,6 +1153,34 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>>   	return 0;
>>   }
>>
>> +static int ec_read_multiple(u8 address, u8 *buffer, size_t bytes)
>> +{
>> +	size_t i;
>> +	int ret;
>> +
>> +	for (i = 0; i < bytes; i++) {
>> +		ret = ec_read(address + i, &buffer[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> Why not use ec_transaction?

Hi,

because ec_transaction() is meant to send raw commands to the EC. And AFAIK read/write transactions can only transfer a
single byte at once, so using ec_transaction() would yield no benefit here.

>
>> +
>> +static int ec_write_multiple(u8 address, u8 *buffer, size_t bytes)
>> +{
>> +	size_t i;
>> +	int ret;
>> +
>> +	for (i = 0; i < bytes; i++) {
>> +		ret = ec_write(address + i, buffer[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> Same as above.
>> +
>>   /*
>>    * WMI can have EmbeddedControl access regions. In which case, we just want to
>>    * hand these off to the EC driver.
>> @@ -1162,27 +1190,28 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
>>   			  u32 bits, u64 *value,
>>   			  void *handler_context, void *region_context)
>>   {
>> -	int result = 0;
>> -	u8 temp = 0;
>> +	int bytes = bits / BITS_PER_BYTE;
>> +	int ret;
>> +
>> +	if (!value)
>> +		return AE_NULL_ENTRY;
>>
>> -	if ((address > 0xFF) || !value)
>> +	if (!bytes || bytes > sizeof(*value))
>>   		return AE_BAD_PARAMETER;
>>
>> -	if (function != ACPI_READ && function != ACPI_WRITE)
>> +	if (address > U8_MAX || address + bytes - 1 > U8_MAX)
>>   		return AE_BAD_PARAMETER;
>>
>> -	if (bits != 8)
> Since you want to support only 16 bit reads/writes, can you check for >16

The 16 bit reads/writes where meant as an example, ACPI code can request much larger values.
The WMI EC handler should be able to handle those, just like the regular ACPI EC handler.

Thanks,
Armin Wolf

>> +	if (function != ACPI_READ && function != ACPI_WRITE)
>>   		return AE_BAD_PARAMETER;
>>
>>   	if (function == ACPI_READ) {
>> -		result = ec_read(address, &temp);
>> -		*value = temp;
>> +		ret = ec_read_multiple(address, (u8 *)value, bytes);
>>   	} else {
>> -		temp = 0xff & *value;
>> -		result = ec_write(address, temp);
>> +		ret = ec_write_multiple(address, (u8 *)value, bytes);
>>   	}
>>
>> -	switch (result) {
>> +	switch (ret) {
>>   	case -EINVAL:
>>   		return AE_BAD_PARAMETER;
>>   	case -ENODEV:
>> --
>> 2.39.2
>>
>>