diff mbox series

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

Message ID 20240308210519.2986-1-W_Armin@gmx.de (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [v4,1/2] platform/x86: wmi: Support reading/writing 16 bit EC values | expand

Commit Message

Armin Wolf March 8, 2024, 9:05 p.m. UTC 
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

Comments

Kuppuswamy Sathyanarayanan March 9, 2024, 5:07 p.m. UTC | #1 
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
>
>
Armin Wolf March 9, 2024, 7:17 p.m. UTC | #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
>>
>>
Kuppuswamy Sathyanarayanan March 10, 2024, 2:39 a.m. UTC | #3 
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);


>
> >
> >> +
> >> +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
> >>
> >>
Armin Wolf March 10, 2024, 6:27 a.m. UTC | #4 
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
>>>>
>>>>
Ilpo Järvinen March 11, 2024, 2:03 p.m. UTC | #5 
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 mbox series

Patch

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: