| Message ID | 20240308210519.2986-1-W_Armin@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] platform/x86: wmi: Support reading/writing 16 bit EC values | expand |
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? > + > +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 > + 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 > >
On 3/8/24 1:05 PM, 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. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > 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 d9bf6d452b3a..84d1ccf6bc14 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -1218,8 +1218,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; > } After checking the callers of acpi_wmi_ec_space_handler() it looks like there is no benefit in returning different ACPI status per error values. It is not being used. why no just return for result < 0 AE_ERROR and return for other cases? > } > > -- > 2.39.2 > >
Am 09.03.24 um 18:41 schrieb Kuppuswamy Sathyanarayanan: > On 3/8/24 1:05 PM, 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. >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> 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 d9bf6d452b3a..84d1ccf6bc14 100644 >> --- a/drivers/platform/x86/wmi.c >> +++ b/drivers/platform/x86/wmi.c >> @@ -1218,8 +1218,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; >> } > After checking the callers of acpi_wmi_ec_space_handler() it looks like there is no benefit in returning different ACPI status per error values. It is not being used. why no just return for result < 0 AE_ERROR and return for other cases? Hi, those handler functions are being called in acpi_ev_address_space_dispatch(), which uses the return value to print error messages. So it makes sense to return different ACPI error values here. Thanks, Armin Wolf >> } >> >> -- >> 2.39.2 >> >>
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 >> >>
On Sat, Mar 9, 2024 at 11:17 AM Armin Wolf <W_Armin@gmx.de> 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@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.
On Sat, Mar 9, 2024 at 11:10 AM Armin Wolf <W_Armin@gmx.de> wrote: > > Am 09.03.24 um 18:41 schrieb Kuppuswamy Sathyanarayanan: > > > On 3/8/24 1:05 PM, 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. > >> > >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > >> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> > >> --- Got it. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >> 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 d9bf6d452b3a..84d1ccf6bc14 100644 > >> --- a/drivers/platform/x86/wmi.c > >> +++ b/drivers/platform/x86/wmi.c > >> @@ -1218,8 +1218,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; > >> } > > After checking the callers of acpi_wmi_ec_space_handler() it looks like there is no benefit in returning different ACPI status per error values. It is not being used. why no just return for result < 0 AE_ERROR and return for other cases? > > Hi, > > those handler functions are being called in acpi_ev_address_space_dispatch(), which uses the return value to print error messages. > So it makes sense to return different ACPI error values here. > > Thanks, > Armin Wolf > > >> } > >> > >> -- > >> 2.39.2 > >> > >>
Am 10.03.24 um 03:39 schrieb Kuppuswamy Sathyanarayanan: > On Sat, Mar 9, 2024 at 11:17 AM Armin Wolf <W_Armin@gmx.de> 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@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. > 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 >>>> >>>>
On 3/8/24 1:05 PM, 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. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- Looks good to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > 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 d9bf6d452b3a..84d1ccf6bc14 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -1218,8 +1218,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 > >
On Fri, 8 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> > --- > 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; > +} > + > +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; > +} > + > /* > * 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) > + 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: Seems okay now, thanks. Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
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; +} + +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; +} + /* * 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) + 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:
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(-) -- 2.39.2