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

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

[Armin Wolf]

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>
---
 drivers/platform/x86/wmi.c | 44 +++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 1920e115da89..900e0e52a5fa 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1153,6 +1153,32 @@ 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)
+{
+	int i, ret;
+
+	for (i = 0; i < bytes; i++) {
+		ret = ec_read(address + i, &buffer[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ec_write_multiple(u8 address, u8 *buffer, size_t bytes)
+{
+	int i, ret;
+
+	for (i = 0; i < bytes; i++) {
+		ret = ec_write(address + i, buffer[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 /*
  * WMI can have EmbeddedControl access regions. In which case, we just want to
  * hand these off to the EC driver.
@@ -1162,27 +1188,25 @@ 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 / 8;
+	int ret;

-	if ((address > 0xFF) || !value)
+	if (address > 0xFF || !value)
 		return AE_BAD_PARAMETER;

-	if (function != ACPI_READ && function != ACPI_WRITE)
+	if (bytes > sizeof(*value))
 		return AE_BAD_PARAMETER;

-	if (bits != 8)
+	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

[PATCH 2/2] platform/x86: wmi: Avoid returning AE_OK upon unknown error

[Armin Wolf]

If an error code other than EINVAL, ENODEV or ETIME is returned
by ec_read()/ec_write(), then AE_OK is wrongly returned.

Fix this by only returning AE_OK if the return code is 0, and
return AE_ERROR otherwise.

Tested on a Dell Inspiron 3505 and a Asus Prime B650-Plus.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 900e0e52a5fa..be0e772a87c8 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1213,8 +1213,10 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
 		return AE_NOT_FOUND;
 	case -ETIME:
 		return AE_TIME;
-	default:
+	case 0:
 		return AE_OK;
+	default:
+		return AE_ERROR;
 	}
 }

--
2.39.2

Re: [PATCH 2/2] platform/x86: wmi: Avoid returning AE_OK upon unknown error

[Hans de Goede]

Hi,

On 3/4/24 23:17, Armin Wolf wrote:
> If an error code other than EINVAL, ENODEV or ETIME is returned
> by ec_read()/ec_write(), then AE_OK is wrongly returned.
> 
> Fix this by only returning AE_OK if the return code is 0, and
> return AE_ERROR otherwise.
> 
> Tested on a Dell Inspiron 3505 and a Asus Prime B650-Plus.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/platform/x86/wmi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 900e0e52a5fa..be0e772a87c8 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1213,8 +1213,10 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
>  		return AE_NOT_FOUND;
>  	case -ETIME:
>  		return AE_TIME;
> -	default:
> +	case 0:
>  		return AE_OK;
> +	default:
> +		return AE_ERROR;
>  	}
>  }
> 
> --
> 2.39.2
> 

Re: [PATCH 2/2] platform/x86: wmi: Avoid returning AE_OK upon unknown error

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

On Mon, 4 Mar 2024, Armin Wolf wrote:

> If an error code other than EINVAL, ENODEV or ETIME is returned
> by ec_read()/ec_write(), then AE_OK is wrongly returned.
> 
> Fix this by only returning AE_OK if the return code is 0, and
> return AE_ERROR otherwise.
> 
> Tested on a Dell Inspiron 3505 and a Asus Prime B650-Plus.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/wmi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 900e0e52a5fa..be0e772a87c8 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1213,8 +1213,10 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
>  		return AE_NOT_FOUND;
>  	case -ETIME:
>  		return AE_TIME;
> -	default:
> +	case 0:
>  		return AE_OK;
> +	default:
> +		return AE_ERROR;
>  	}
>  }

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

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

[Hans de Goede]

Hi,

On 3/4/24 23:17, 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>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/platform/x86/wmi.c | 44 +++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 1920e115da89..900e0e52a5fa 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1153,6 +1153,32 @@ 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)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < bytes; i++) {
> +		ret = ec_read(address + i, &buffer[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ec_write_multiple(u8 address, u8 *buffer, size_t bytes)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < bytes; i++) {
> +		ret = ec_write(address + i, buffer[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * WMI can have EmbeddedControl access regions. In which case, we just want to
>   * hand these off to the EC driver.
> @@ -1162,27 +1188,25 @@ 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 / 8;
> +	int ret;
> 
> -	if ((address > 0xFF) || !value)
> +	if (address > 0xFF || !value)
>  		return AE_BAD_PARAMETER;
> 
> -	if (function != ACPI_READ && function != ACPI_WRITE)
> +	if (bytes > sizeof(*value))
>  		return AE_BAD_PARAMETER;
> 
> -	if (bits != 8)
> +	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
> 

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

[Ilpo Järvinen]

On Mon, 4 Mar 2024, 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>
> ---
>  drivers/platform/x86/wmi.c | 44 +++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 1920e115da89..900e0e52a5fa 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1153,6 +1153,32 @@ 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)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < bytes; i++) {
> +		ret = ec_read(address + i, &buffer[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ec_write_multiple(u8 address, u8 *buffer, size_t bytes)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < bytes; i++) {
> +		ret = ec_write(address + i, buffer[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * WMI can have EmbeddedControl access regions. In which case, we just want to
>   * hand these off to the EC driver.
> @@ -1162,27 +1188,25 @@ 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 / 8;

I'm a quite hesitant about this. IMO, it should do DIV_ROUND_UP(bits, 
BITS_PER_BYTE) or return AE_BAD_PARAMETER when bits is not divisable by 8. 
And if you choose the round up approach, I'm not sure what the write 
should do with the excess bits.

In any case, 8 -> BITS_PER_BYTE.

> +	int ret;
> 
> -	if ((address > 0xFF) || !value)
> +	if (address > 0xFF || !value)

This should takes bytes into account to not overflow the u8 address?

-- 
 i.

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

[Armin Wolf]

Am 06.03.24 um 11:19 schrieb Ilpo Järvinen:

> On Mon, 4 Mar 2024, 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>
>> ---
>>   drivers/platform/x86/wmi.c | 44 +++++++++++++++++++++++++++++---------
>>   1 file changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index 1920e115da89..900e0e52a5fa 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -1153,6 +1153,32 @@ 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)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i < bytes; i++) {
>> +		ret = ec_read(address + i, &buffer[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ec_write_multiple(u8 address, u8 *buffer, size_t bytes)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i < bytes; i++) {
>> +		ret = ec_write(address + i, buffer[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   /*
>>    * WMI can have EmbeddedControl access regions. In which case, we just want to
>>    * hand these off to the EC driver.
>> @@ -1162,27 +1188,25 @@ 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 / 8;
> I'm a quite hesitant about this. IMO, it should do DIV_ROUND_UP(bits,
> BITS_PER_BYTE) or return AE_BAD_PARAMETER when bits is not divisable by 8.
> And if you choose the round up approach, I'm not sure what the write
> should do with the excess bits.
>
> In any case, 8 -> BITS_PER_BYTE.

After taking a look at acpi_ex_access_region(), which invokes the address space handler,
i think the number of bits are always divisible by 8.

I CCed the maintainers of the ACPI EC driver, so that we can clarify if this is indeed
always the case.

Thanks,
Armin Wolf

>> +	int ret;
>>
>> -	if ((address > 0xFF) || !value)
>> +	if (address > 0xFF || !value)
> This should takes bytes into account to not overflow the u8 address?
>