Am 10.03.24 um 03:39 schrieb Kuppuswamy Sathyanarayanan:
On Sat, Mar 9, 2024 at 11:17 AM Armin Wolf <W_Armin@xxxxxx> wrote:
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
/s/prefectly/perfectly
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@xxxxxx>
---
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.
From the implementation, I don't see any length restriction. If it is
a functional restriction, then fine.
int ec_transaction(u8 command,
const u8 *wdata, unsigned wdata_len,
u8 *rdata, unsigned rdata_len)
{
struct transaction t = {.command = command,
.wdata = wdata, .rdata = rdata,
.wlen = wdata_len, .rlen = rdata_len};
if (!first_ec)
return -ENODEV;
return acpi_ec_transaction(first_ec, &t);
}
EXPORT_SYMBOL(ec_transaction);
Since we are using the ACPI_EC_COMMAND_READ/_WRITE, we can only read/write a single byte, as specified
in ACPI (12.3.1 and 12.3.2).
Thanks,
Armin Wolf
+
+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.
Got it.
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
