[PATCH] SCPI (pre-v1.0): fix reading sensor value

[PATCH] SCPI (pre-v1.0): fix reading sensor value

[Martin Blumenstingl]

I observed the following "strange" value when trying to read the SCPI
temperature sensor on my Amlogic GXM S912 device:
$ cat /sys/class/hwmon/hwmon0/temp1_input
6875990994467160116

The value reported by the original kernel (Amlogic vendor kernel, after
a reboot obviously) was 53C.
The Amlogic SCPI driver only uses a single 32bit value to read the
sensor value, instead of two. After stripping the upper 32bits from
above value gives "52" as result, which is basically identical to
what the vendor kernel reports.
I also compared this with the value shown by u-boot (since there's
less delay between "reboot to u-boot" compared to "reboot from mainline
kernel to vendor kernel") and the temperature reported by u-boot always
matches the lower 32bits of the value from scpi-hwmon temp1_input.

This patch introduces a separate function for reading the sensor value
on pre-v1.0 SCPI firmwares. I also tried initializing the "buf"
variable in scpi_sensor_get_value() but this didn't work (because we
request a 64bit payload from the SCPI firmware, but the firmware only
replies with a 32bit payload).

Martin Blumenstingl (1):
  firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI
    firmwares

 drivers/firmware/arm_scpi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

-- 
2.10.2

[PATCH] firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI firmwares

[Martin Blumenstingl]

The pre-1.0 SCPI firmwares are using one __le32 as sensor value, while
the 1.0 SCPI protocol uses two __le32 as sensor values (a total of
64bit, split into 32bit upper and 32bit lower value).
Using an "struct sensor_value" to read the sensor value on a pre-1.0
SCPI firmware gives garbage in the "hi_val" field. Introducing a
separate function which handles scpi_ops.sensor_get_value for pre-1.0
SCPI firmware implementations ensures that we do not read memory which
was not written by the SCPI firmware (which fixes for example the
temperature reported by scpi-hwmon).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/firmware/arm_scpi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 70e1323..19f750d 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -728,6 +728,20 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
 	return ret;
 }
 
+static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
+{
+	__le16 id = cpu_to_le16(sensor);
+	__le32 value;
+	int ret;
+
+	ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
+				&value, sizeof(value));
+	if (!ret)
+		*val = le32_to_cpu(value);
+
+	return ret;
+}
+
 static int scpi_device_get_power_state(u16 dev_id)
 {
 	int ret;
@@ -960,6 +974,7 @@ static int scpi_probe(struct platform_device *pdev)
 	if (scpi_info->is_legacy) {
 		/* Replace with legacy variants */
 		scpi_ops.clk_set_val = legacy_scpi_clk_set_val;
+		scpi_ops.sensor_get_value = legacy_scpi_sensor_get_value;
 		scpi_info->commands = scpi_legacy_commands;
 
 		/* Fill priority bitmap */
-- 
2.10.2

[PATCH] firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI firmwares

[Sudeep Holla]

On 24/11/16 00:18, Martin Blumenstingl wrote:
> The pre-1.0 SCPI firmwares are using one __le32 as sensor value, while
> the 1.0 SCPI protocol uses two __le32 as sensor values (a total of
> 64bit, split into 32bit upper and 32bit lower value).
> Using an "struct sensor_value" to read the sensor value on a pre-1.0
> SCPI firmware gives garbage in the "hi_val" field. Introducing a
> separate function which handles scpi_ops.sensor_get_value for pre-1.0
> SCPI firmware implementations ensures that we do not read memory which
> was not written by the SCPI firmware (which fixes for example the
> temperature reported by scpi-hwmon).
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/firmware/arm_scpi.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 70e1323..19f750d 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -728,6 +728,20 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>  	return ret;
>  }
>  
> +static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
> +{
> +	__le16 id = cpu_to_le16(sensor);
> +	__le32 value;
> +	int ret;
> +
> +	ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
> +				&value, sizeof(value));
> +	if (!ret)
> +		*val = le32_to_cpu(value);
> +
> +	return ret;
> +}

Instead of duplicating the code so much, can we just manage something
like this. If more commands need Rx len, then we can think of adding
that. Even then reseting shared buffers is not good, we can clear the
buffers on the stack.

diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index 70e13230d8db..04aa873205e9 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/drivers/firmware/arm_scpi.c
@@ -721,11 +721,15 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)

        ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
                                &buf, sizeof(buf));
-       if (!ret)
+       if (ret)
+               return ret;
+
+       if (scpi_info->is_legacy) /* 32-bits supported, hi_val can be
junk */
+               *val = le32_to_cpu(buf.lo_val);
+       else
                *val = (u64)le32_to_cpu(buf.hi_val) << 32 |
                        le32_to_cpu(buf.lo_val);
-
-       return ret;
+       return 0;
 }

 static int scpi_device_get_power_state(u16 dev_id)


-- 
Regards,
Sudeep

[PATCH] firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI firmwares

[Martin Blumenstingl]

On Wed, Dec 7, 2016 at 7:44 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 24/11/16 00:18, Martin Blumenstingl wrote:
>> The pre-1.0 SCPI firmwares are using one __le32 as sensor value, while
>> the 1.0 SCPI protocol uses two __le32 as sensor values (a total of
>> 64bit, split into 32bit upper and 32bit lower value).
>> Using an "struct sensor_value" to read the sensor value on a pre-1.0
>> SCPI firmware gives garbage in the "hi_val" field. Introducing a
>> separate function which handles scpi_ops.sensor_get_value for pre-1.0
>> SCPI firmware implementations ensures that we do not read memory which
>> was not written by the SCPI firmware (which fixes for example the
>> temperature reported by scpi-hwmon).
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 70e1323..19f750d 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -728,6 +728,20 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>>       return ret;
>>  }
>>
>> +static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
>> +{
>> +     __le16 id = cpu_to_le16(sensor);
>> +     __le32 value;
>> +     int ret;
>> +
>> +     ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
>> +                             &value, sizeof(value));
>> +     if (!ret)
>> +             *val = le32_to_cpu(value);
>> +
>> +     return ret;
>> +}
>
> Instead of duplicating the code so much, can we just manage something
> like this. If more commands need Rx len, then we can think of adding
> that. Even then reseting shared buffers is not good, we can clear the
> buffers on the stack.
I tested this approach and it's working fine! I just went ahead and
took your patch, moved the comment to a separate line, added a
description and send the patch as v3 (feel free to add yourself as
author and simply replace my Signed-off-by with a Tested-by).

> diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
> index 70e13230d8db..04aa873205e9 100644
> --- i/drivers/firmware/arm_scpi.c
> +++ w/drivers/firmware/arm_scpi.c
> @@ -721,11 +721,15 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>
>         ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
>                                 &buf, sizeof(buf));
> -       if (!ret)
> +       if (ret)
> +               return ret;
> +
> +       if (scpi_info->is_legacy) /* 32-bits supported, hi_val can be
> junk */
> +               *val = le32_to_cpu(buf.lo_val);
> +       else
>                 *val = (u64)le32_to_cpu(buf.hi_val) << 32 |
>                         le32_to_cpu(buf.lo_val);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static int scpi_device_get_power_state(u16 dev_id)
>
>
> --
> Regards,
> Sudeep

[PATCH] SCPI (pre-v1.0): fix reading sensor value

[Sudeep Holla]

On 24/11/16 00:18, Martin Blumenstingl wrote:
> I observed the following "strange" value when trying to read the SCPI
> temperature sensor on my Amlogic GXM S912 device:
> $ cat /sys/class/hwmon/hwmon0/temp1_input
> 6875990994467160116
>
> The value reported by the original kernel (Amlogic vendor kernel, after
> a reboot obviously) was 53C.
> The Amlogic SCPI driver only uses a single 32bit value to read the
> sensor value, instead of two. After stripping the upper 32bits from
> above value gives "52" as result, which is basically identical to
> what the vendor kernel reports.

Can you check why the upper 32-bit is not set to 0 ?

In scpi_process_cmd, we memset extra rx_buf length by 0 and that should
take care. Neil had mentioned that works but now I doubt if firmware
returns 8 instead of 4 in the size which is wrong as it supports only
32-bit.

-- 
Regards,
Sudeep

[PATCH] SCPI (pre-v1.0): fix reading sensor value

[Martin Blumenstingl]

On Thu, Nov 24, 2016 at 11:47 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 24/11/16 00:18, Martin Blumenstingl wrote:
>>
>> I observed the following "strange" value when trying to read the SCPI
>> temperature sensor on my Amlogic GXM S912 device:
>> $ cat /sys/class/hwmon/hwmon0/temp1_input
>> 6875990994467160116
>>
>> The value reported by the original kernel (Amlogic vendor kernel, after
>> a reboot obviously) was 53C.
>> The Amlogic SCPI driver only uses a single 32bit value to read the
>> sensor value, instead of two. After stripping the upper 32bits from
>> above value gives "52" as result, which is basically identical to
>> what the vendor kernel reports.
>
>
> Can you check why the upper 32-bit is not set to 0 ?
>
> In scpi_process_cmd, we memset extra rx_buf length by 0 and that should
> take care. Neil had mentioned that works but now I doubt if firmware
> returns 8 instead of 4 in the size which is wrong as it supports only
> 32-bit.
according to the code "RX Length is not replied by the legacy
Firmware", so for legacy firmwares the "if (match->rx_len > len)"
condition will never be true (because both values are always equal).
in the sensor case we then go and copy 8 byte from mem->payload to
match->rx_buf, but SCPI firmware only wrote 4 bytes to mem->payload.
This means we are simply reading 4 byte (hi_val) of uninitialized
memory - which may be all zeroes if we're lucky - but in my case I got
"garbage" (I guess it's the second byte from the *previous* command
which are leaking here).

while writing this I see a second (more generic) approach which might
work as well:
scpi_chan does not hold any information about rx_payload/tx_payload
sizes (these are calculated in scpi_probe but not stored anywhere).
(for now, let's assume we had the rx_payload_size available)
we could then go ahead and memset(rx_payload, 0, rx_payload_size) in
scpi_tx_prepare or scpi_send_message.
However, I am not sure if that would have any side-effects (for
example on newer SCPI implementations).


Regards,
Martin

[PATCH] SCPI (pre-v1.0): fix reading sensor value

[Martin Blumenstingl]

On Thu, Nov 24, 2016 at 12:15 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Thu, Nov 24, 2016 at 11:47 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 24/11/16 00:18, Martin Blumenstingl wrote:
>>>
>>> I observed the following "strange" value when trying to read the SCPI
>>> temperature sensor on my Amlogic GXM S912 device:
>>> $ cat /sys/class/hwmon/hwmon0/temp1_input
>>> 6875990994467160116
>>>
>>> The value reported by the original kernel (Amlogic vendor kernel, after
>>> a reboot obviously) was 53C.
>>> The Amlogic SCPI driver only uses a single 32bit value to read the
>>> sensor value, instead of two. After stripping the upper 32bits from
>>> above value gives "52" as result, which is basically identical to
>>> what the vendor kernel reports.
>>
>>
>> Can you check why the upper 32-bit is not set to 0 ?
>>
>> In scpi_process_cmd, we memset extra rx_buf length by 0 and that should
>> take care. Neil had mentioned that works but now I doubt if firmware
>> returns 8 instead of 4 in the size which is wrong as it supports only
>> 32-bit.
> according to the code "RX Length is not replied by the legacy
> Firmware", so for legacy firmwares the "if (match->rx_len > len)"
> condition will never be true (because both values are always equal).
> in the sensor case we then go and copy 8 byte from mem->payload to
> match->rx_buf, but SCPI firmware only wrote 4 bytes to mem->payload.
> This means we are simply reading 4 byte (hi_val) of uninitialized
> memory - which may be all zeroes if we're lucky - but in my case I got
> "garbage" (I guess it's the second byte from the *previous* command
> which are leaking here).
>
> while writing this I see a second (more generic) approach which might
> work as well:
> scpi_chan does not hold any information about rx_payload/tx_payload
> sizes (these are calculated in scpi_probe but not stored anywhere).
> (for now, let's assume we had the rx_payload_size available)
> we could then go ahead and memset(rx_payload, 0, rx_payload_size) in
> scpi_tx_prepare or scpi_send_message.
> However, I am not sure if that would have any side-effects (for
> example on newer SCPI implementations).
I simply tried implementing this solution and I find it better than
the old one. However, I am still not sure if there are any
side-effects. maybe you can simply review v2 of this series which
implements the described approach (the result is the same as with v1:
temp1_input contains the correct value).


Regards,
Martin

[PATCH] SCPI (pre-v1.0): fix reading sensor value

[Martin Blumenstingl]

Hello Sudeep,

On Fri, Nov 25, 2016 at 1:56 AM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Thu, Nov 24, 2016 at 12:15 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> On Thu, Nov 24, 2016 at 11:47 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 24/11/16 00:18, Martin Blumenstingl wrote:
>>>>
>>>> I observed the following "strange" value when trying to read the SCPI
>>>> temperature sensor on my Amlogic GXM S912 device:
>>>> $ cat /sys/class/hwmon/hwmon0/temp1_input
>>>> 6875990994467160116
>>>>
>>>> The value reported by the original kernel (Amlogic vendor kernel, after
>>>> a reboot obviously) was 53C.
>>>> The Amlogic SCPI driver only uses a single 32bit value to read the
>>>> sensor value, instead of two. After stripping the upper 32bits from
>>>> above value gives "52" as result, which is basically identical to
>>>> what the vendor kernel reports.
>>>
>>>
>>> Can you check why the upper 32-bit is not set to 0 ?
>>>
>>> In scpi_process_cmd, we memset extra rx_buf length by 0 and that should
>>> take care. Neil had mentioned that works but now I doubt if firmware
>>> returns 8 instead of 4 in the size which is wrong as it supports only
>>> 32-bit.
>> according to the code "RX Length is not replied by the legacy
>> Firmware", so for legacy firmwares the "if (match->rx_len > len)"
>> condition will never be true (because both values are always equal).
>> in the sensor case we then go and copy 8 byte from mem->payload to
>> match->rx_buf, but SCPI firmware only wrote 4 bytes to mem->payload.
>> This means we are simply reading 4 byte (hi_val) of uninitialized
>> memory - which may be all zeroes if we're lucky - but in my case I got
>> "garbage" (I guess it's the second byte from the *previous* command
>> which are leaking here).
>>
>> while writing this I see a second (more generic) approach which might
>> work as well:
>> scpi_chan does not hold any information about rx_payload/tx_payload
>> sizes (these are calculated in scpi_probe but not stored anywhere).
>> (for now, let's assume we had the rx_payload_size available)
>> we could then go ahead and memset(rx_payload, 0, rx_payload_size) in
>> scpi_tx_prepare or scpi_send_message.
>> However, I am not sure if that would have any side-effects (for
>> example on newer SCPI implementations).
> I simply tried implementing this solution and I find it better than
> the old one. However, I am still not sure if there are any
> side-effects. maybe you can simply review v2 of this series which
> implements the described approach (the result is the same as with v1:
> temp1_input contains the correct value).
did you have time to review this yet?


Regards,
Martin

[PATCH] SCPI (pre-v1.0): fix reading sensor value

[Sudeep Holla]

On 02/12/16 22:54, Martin Blumenstingl wrote:
> Hello Sudeep,
> 
> On Fri, Nov 25, 2016 at 1:56 AM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> On Thu, Nov 24, 2016 at 12:15 PM, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>> On Thu, Nov 24, 2016 at 11:47 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>
>>>>
>>>> On 24/11/16 00:18, Martin Blumenstingl wrote:
>>>>>
>>>>> I observed the following "strange" value when trying to read the SCPI
>>>>> temperature sensor on my Amlogic GXM S912 device:
>>>>> $ cat /sys/class/hwmon/hwmon0/temp1_input
>>>>> 6875990994467160116
>>>>>
>>>>> The value reported by the original kernel (Amlogic vendor kernel, after
>>>>> a reboot obviously) was 53C.
>>>>> The Amlogic SCPI driver only uses a single 32bit value to read the
>>>>> sensor value, instead of two. After stripping the upper 32bits from
>>>>> above value gives "52" as result, which is basically identical to
>>>>> what the vendor kernel reports.
>>>>
>>>>
>>>> Can you check why the upper 32-bit is not set to 0 ?
>>>>
>>>> In scpi_process_cmd, we memset extra rx_buf length by 0 and that should
>>>> take care. Neil had mentioned that works but now I doubt if firmware
>>>> returns 8 instead of 4 in the size which is wrong as it supports only
>>>> 32-bit.
>>> according to the code "RX Length is not replied by the legacy
>>> Firmware", so for legacy firmwares the "if (match->rx_len > len)"
>>> condition will never be true (because both values are always equal).
>>> in the sensor case we then go and copy 8 byte from mem->payload to
>>> match->rx_buf, but SCPI firmware only wrote 4 bytes to mem->payload.
>>> This means we are simply reading 4 byte (hi_val) of uninitialized
>>> memory - which may be all zeroes if we're lucky - but in my case I got
>>> "garbage" (I guess it's the second byte from the *previous* command
>>> which are leaking here).
>>>
>>> while writing this I see a second (more generic) approach which might
>>> work as well:
>>> scpi_chan does not hold any information about rx_payload/tx_payload
>>> sizes (these are calculated in scpi_probe but not stored anywhere).
>>> (for now, let's assume we had the rx_payload_size available)
>>> we could then go ahead and memset(rx_payload, 0, rx_payload_size) in
>>> scpi_tx_prepare or scpi_send_message.
>>> However, I am not sure if that would have any side-effects (for
>>> example on newer SCPI implementations).
>> I simply tried implementing this solution and I find it better than
>> the old one. However, I am still not sure if there are any
>> side-effects. maybe you can simply review v2 of this series which
>> implements the described approach (the result is the same as with v1:
>> temp1_input contains the correct value).
>
> did you have time to review this yet?
> 

I was away, I will look into this today or tomorrow.

-- 
Regards,
Sudeep

[PATCH v2 0/2] SCPI (pre-v1.0): fix reading sensor value

[Martin Blumenstingl]

I observed the following "strange" value when trying to read the SCPI
temperature sensor on my Amlogic GXM S912 device:
$ cat /sys/class/hwmon/hwmon0/temp1_input
6875990994467160116

The value reported by the original kernel (Amlogic vendor kernel, after
a reboot obviously) was 53C.
The Amlogic SCPI driver only uses a single 32bit value to read the
sensor value, instead of two. After stripping the upper 32bits from
above value gives "52" as result, which is basically identical to
what the vendor kernel reports.
I also compared this with the value shown by u-boot (since there's
less delay between "reboot to u-boot" compared to "reboot from mainline
kernel to vendor kernel") and the temperature reported by u-boot always
matches the lower 32bits of the value from scpi-hwmon temp1_input.

Version 1 of this series introduced a separate function for reading the
sensor value on pre-v1.0 SCPI firmwares. I also tried initializing the
"buf" variable in scpi_sensor_get_value() but this didn't work (because
we request a 64bit payload from the SCPI firmware, but the firmware only
replies with a 32bit payload).

Version 2 is different as it does not require a "legacy implementation"
for scpi_sensor_get_value(). Instead the rx_buf is zeroed out before
reading the buffer from the mailbox. This works fine as well, since the
sensor_value.hi_val field was added after sensor_value.lo_val (meaning
it is backwards compatible, as long as hi_val is all zeroes on
pre-v1.0 SCPI firmwares).
A small benefit we get from this: we are now able to handle all
commands where new fields are introduced at the end of receive buffer
(which might be relevant also for future SCPI implementations - but
this is pure speculation).
I did not remove the memset(buf, 0, len) in scpi_process_cmd() because
I am not sure if there are v1.0 SCPI firmwares out there which respond
that X bytes are available in the rx_buf while it actually writes more
data (X + n where n > 0 bytes) than indicated.


Changes since v1:
- zero out the rx_buf before reading the mbox buffer (see long
  description above) instead of introducing a separate legacy command
  for reading the sensor value
- added patch 2/2 which validates the payload lengths (so nobody can
  read or write data beyond rx_buf or tx_buf). This optional and patch
  1/2 can be applied without it


Martin Blumenstingl (2):
  firmware: arm_scpi: zero RX buffer before requesting data from the
    mbox
  firmware: arm_scpi: check the payload length in scpi_send_message

 drivers/firmware/arm_scpi.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

-- 
2.10.2

[PATCH v2 1/2] firmware: arm_scpi: zero RX buffer before requesting data from the mbox

[Martin Blumenstingl]

The original code was relying on the fact that the SCPI firmware
responds with the same number of bytes (or more, all extra data would be
ignored in that case) as requested.
However, we have some pre-v1.0 SCPI firmwares which are responding with
less data for some commands (sensor_value.hi_val did not exist in the
old implementation). This means that some data from the previous
command's RX buffer was leaked into the current command (as the RX
buffer is re-used for all commands on the same channel). Clearing the
RX buffer before (re-) using it ensures we get a consistent result, even
if the SCPI firmware returns less bytes than requested.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 70e1323..8c183d8 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -259,6 +259,7 @@ struct scpi_chan {
 	struct mbox_chan *chan;
 	void __iomem *tx_payload;
 	void __iomem *rx_payload;
+	resource_size_t max_payload_len;
 	struct list_head rx_pending;
 	struct list_head xfers_list;
 	struct scpi_xfer *xfers;
@@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 	if (t->rx_buf) {
 		if (!(++ch->token))
 			++ch->token;
+
+		/* clear the RX buffer as it is shared across all commands on
+		 * the same channel (to make sure we're not leaking data from
+		 * the previous response into the current command if the SCPI
+		 * firmware writes less data than requested).
+		 * This is especially important for pre-v1.0 SCPI firmwares
+		 * where some fields in the responses do not exist (while they
+		 * exist but are optional in newer versions). One example for
+		 * this problem is sensor_value.hi_val, which would contain
+		 * ("leak") the second 4 bytes of the RX buffer from the
+		 * previous command.
+		 */
+		memset_io(ch->rx_payload, 0, ch->max_payload_len);
+
 		ADD_SCPI_TOKEN(t->cmd, ch->token);
 		spin_lock_irqsave(&ch->rx_lock, flags);
 		list_add_tail(&t->node, &ch->rx_pending);
@@ -921,7 +936,9 @@ static int scpi_probe(struct platform_device *pdev)
 			ret = -EADDRNOTAVAIL;
 			goto err;
 		}
-		pchan->tx_payload = pchan->rx_payload + (size >> 1);
+
+		pchan->max_payload_len = size / 2;
+		pchan->tx_payload = pchan->rx_payload + pchan->max_payload_len;
 
 		cl->dev = dev;
 		cl->rx_callback = scpi_handle_remote_msg;
-- 
2.10.2

[PATCH v2 1/2] firmware: arm_scpi: zero RX buffer before requesting data from the mbox

[Sudeep Holla]

On 25/11/16 00:54, Martin Blumenstingl wrote:
> The original code was relying on the fact that the SCPI firmware
> responds with the same number of bytes (or more, all extra data would be
> ignored in that case) as requested.
> However, we have some pre-v1.0 SCPI firmwares which are responding with
> less data for some commands (sensor_value.hi_val did not exist in the
> old implementation). This means that some data from the previous
> command's RX buffer was leaked into the current command (as the RX
> buffer is re-used for all commands on the same channel). Clearing the
> RX buffer before (re-) using it ensures we get a consistent result, even
> if the SCPI firmware returns less bytes than requested.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 70e1323..8c183d8 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -259,6 +259,7 @@ struct scpi_chan {
>  	struct mbox_chan *chan;
>  	void __iomem *tx_payload;
>  	void __iomem *rx_payload;
> +	resource_size_t max_payload_len;
>  	struct list_head rx_pending;
>  	struct list_head xfers_list;
>  	struct scpi_xfer *xfers;
> @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>  	if (t->rx_buf) {
>  		if (!(++ch->token))
>  			++ch->token;
> +
> +		/* clear the RX buffer as it is shared across all commands on
> +		 * the same channel (to make sure we're not leaking data from
> +		 * the previous response into the current command if the SCPI
> +		 * firmware writes less data than requested).
> +		 * This is especially important for pre-v1.0 SCPI firmwares
> +		 * where some fields in the responses do not exist (while they
> +		 * exist but are optional in newer versions). One example for
> +		 * this problem is sensor_value.hi_val, which would contain
> +		 * ("leak") the second 4 bytes of the RX buffer from the
> +		 * previous command.
> +		 */
> +		memset_io(ch->rx_payload, 0, ch->max_payload_len);
> +

This looks like a overkill to me. I prefer your first approach over
this, if it's only this command that's affected. I am still not sure
why Neil Armstrong mentioned that it worked for him with 64-bit read.

-- 
Regards,
Sudeep

[PATCH v2 1/2] firmware: arm_scpi: zero RX buffer before requesting data from the mbox

[Martin Blumenstingl]

On Wed, Dec 7, 2016 at 7:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 25/11/16 00:54, Martin Blumenstingl wrote:
>> The original code was relying on the fact that the SCPI firmware
>> responds with the same number of bytes (or more, all extra data would be
>> ignored in that case) as requested.
>> However, we have some pre-v1.0 SCPI firmwares which are responding with
>> less data for some commands (sensor_value.hi_val did not exist in the
>> old implementation). This means that some data from the previous
>> command's RX buffer was leaked into the current command (as the RX
>> buffer is re-used for all commands on the same channel). Clearing the
>> RX buffer before (re-) using it ensures we get a consistent result, even
>> if the SCPI firmware returns less bytes than requested.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 70e1323..8c183d8 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -259,6 +259,7 @@ struct scpi_chan {
>>       struct mbox_chan *chan;
>>       void __iomem *tx_payload;
>>       void __iomem *rx_payload;
>> +     resource_size_t max_payload_len;
>>       struct list_head rx_pending;
>>       struct list_head xfers_list;
>>       struct scpi_xfer *xfers;
>> @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>       if (t->rx_buf) {
>>               if (!(++ch->token))
>>                       ++ch->token;
>> +
>> +             /* clear the RX buffer as it is shared across all commands on
>> +              * the same channel (to make sure we're not leaking data from
>> +              * the previous response into the current command if the SCPI
>> +              * firmware writes less data than requested).
>> +              * This is especially important for pre-v1.0 SCPI firmwares
>> +              * where some fields in the responses do not exist (while they
>> +              * exist but are optional in newer versions). One example for
>> +              * this problem is sensor_value.hi_val, which would contain
>> +              * ("leak") the second 4 bytes of the RX buffer from the
>> +              * previous command.
>> +              */
>> +             memset_io(ch->rx_payload, 0, ch->max_payload_len);
>> +
>
> This looks like a overkill to me. I prefer your first approach over
> this, if it's only this command that's affected. I am still not sure
> why Neil Armstrong mentioned that it worked for him with 64-bit read.
OK, I'm fine with that. I also had a look at the patch you posted in
the cover-letter (I did not have time to test it yet though, I'll give
feedback tomorrow) - this looks better than my v1.

I think the explanation why it worked for Neil is pretty simple:
it depends on the SCPI command which was executed before the "read
sensor" command:
if that command returned [4 bytes with any value]0x00000000[any number
of bytes] then he got a valid result

[PATCH v2 1/2] firmware: arm_scpi: zero RX buffer before requesting data from the mbox

[Jon Medhurst (Tixy)]

On Fri, 2016-11-25 at 01:54 +0100, Martin Blumenstingl wrote:
> The original code was relying on the fact that the SCPI firmware
> responds with the same number of bytes (or more, all extra data would be
> ignored in that case) as requested.
> However, we have some pre-v1.0 SCPI firmwares which are responding with
> less data for some commands (sensor_value.hi_val did not exist in the
> old implementation). This means that some data from the previous
> command's RX buffer was leaked into the current command (as the RX
> buffer is re-used for all commands on the same channel). Clearing the
> RX buffer before (re-) using it ensures we get a consistent result, even
> if the SCPI firmware returns less bytes than requested.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 70e1323..8c183d8 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -259,6 +259,7 @@ struct scpi_chan {
>  	struct mbox_chan *chan;
>  	void __iomem *tx_payload;
>  	void __iomem *rx_payload;
> +	resource_size_t max_payload_len;

Ah, here's max_payload_len, sorry, I reviewed the patches in the wrong
order. And reflecting on things, having the runtime

>  	struct list_head rx_pending;
>  	struct list_head xfers_list;
>  	struct scpi_xfer *xfers;
> @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>  	if (t->rx_buf) {
>  		if (!(++ch->token))
>  			++ch->token;
> +
> +		/* clear the RX buffer as it is shared across all commands on
> +		 * the same channel (to make sure we're not leaking data from
> +		 * the previous response into the current command if the SCPI
> +		 * firmware writes less data than requested).
> +		 * This is especially important for pre-v1.0 SCPI firmwares
> +		 * where some fields in the responses do not exist (while they
> +		 * exist but are optional in newer versions). One example for
> +		 * this problem is sensor_value.hi_val, which would contain
> +		 * ("leak") the second 4 bytes of the RX buffer from the
> +		 * previous command.
> +		 */
> +		memset_io(ch->rx_payload, 0, ch->max_payload_len);
> +

Isn't the payload size specified in the header? In which case the bug
you describe is due to the implementation writing 4 bytes and setting
the length to 8. Anyway, this seems almost like a quirk of a specific
implementation and perhaps should be handled as such, rather that doing
this for all commands on all boards using SCPI.

>  		ADD_SCPI_TOKEN(t->cmd, ch->token);
>  		spin_lock_irqsave(&ch->rx_lock, flags);
>  		list_add_tail(&t->node, &ch->rx_pending);
> @@ -921,7 +936,9 @@ static int scpi_probe(struct platform_device *pdev)
>  			ret = -EADDRNOTAVAIL;
>  			goto err;
>  		}
> -		pchan->tx_payload = pchan->rx_payload + (size >> 1);
> +
> +		pchan->max_payload_len = size / 2;
> +		pchan->tx_payload = pchan->rx_payload + pchan->max_payload_len;
>  
>  		cl->dev = dev;
>  		cl->rx_callback = scpi_handle_remote_msg;

-- 
Tixy

[PATCH v2 2/2] firmware: arm_scpi: check the payload length in scpi_send_message

[Martin Blumenstingl]

This adds a sanity check to ensure we're not writing data beyond the
end of our rx_buf and tx_buf. Currently we are still far from reaching
this limit, so this is a non-critical fix.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/firmware/arm_scpi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 8c183d8..78ea8c7 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -538,6 +538,11 @@ static int scpi_send_message(u8 idx, void *tx_buf, unsigned int tx_len,
 			scpi_info->num_chans;
 	scpi_chan = scpi_info->channels + chan;
 
+	if (tx_len > scpi_chan->max_payload_len)
+		return -EINVAL;
+	if (rx_len > scpi_chan->max_payload_len)
+		return -EINVAL;
+
 	msg = get_scpi_xfer(scpi_chan);
 	if (!msg)
 		return -ENOMEM;
-- 
2.10.2

[PATCH v2 2/2] firmware: arm_scpi: check the payload length in scpi_send_message

[Jon Medhurst (Tixy)]

On Fri, 2016-11-25 at 01:54 +0100, Martin Blumenstingl wrote:
> This adds a sanity check to ensure we're not writing data beyond the
> end of our rx_buf and tx_buf. Currently we are still far from reaching
> this limit, so this is a non-critical fix.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/firmware/arm_scpi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 8c183d8..78ea8c7 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -538,6 +538,11 @@ static int scpi_send_message(u8 idx, void *tx_buf, unsigned int tx_len,
>  			scpi_info->num_chans;
>  	scpi_chan = scpi_info->channels + chan;
>  
> +	if (tx_len > scpi_chan->max_payload_len)
> +		return -EINVAL;
> +	if (rx_len > scpi_chan->max_payload_len)
> +		return -EINVAL;

What is max_payload_len? I don't see it in anywhere in the kernel tree.
Also, why is the check needed? Surely having a channel not be able to
support the requirements of the SCPI protocol is a bit of a
design/configuration flaw of the system and shouldn't happen. If a check
is really needed perhaps it also warrants a WARN_ON or similar?

> +
>  	msg = get_scpi_xfer(scpi_chan);
>  	if (!msg)
>  		return -ENOMEM;

-- 
Tixy

[PATCH v3] SCPI (pre-v1.0): fix reading sensor value

[Martin Blumenstingl]

I observed the following "strange" value when trying to read the SCPI
temperature sensor on my Amlogic GXM S912 device:
$ cat /sys/class/hwmon/hwmon0/temp1_input
6875990994467160116

The value reported by the original kernel (Amlogic vendor kernel, after
a reboot obviously) was 53C.
The Amlogic SCPI driver only uses a single 32bit value to read the
sensor value, instead of two. After stripping the upper 32bits from
above value gives "52" as result, which is basically identical to
what the vendor kernel reports.
I also compared this with the value shown by u-boot (since there's
less delay between "reboot to u-boot" compared to "reboot from mainline
kernel to vendor kernel") and the temperature reported by u-boot always
matches the lower 32bits of the value from scpi-hwmon temp1_input.

Changes since v2:
- use simplified approach from Sudeep Holla which is similar to v1
  but avoids duplicate code by adding a simple
  "if (scpi_info->is_legacy)" to scpi_sensor_get_value() instead of
  duplicating the logic

Changes since v1:
- zero out the rx_buf before reading the mbox buffer (see long
  description above) instead of introducing a separate legacy command
  for reading the sensor value
- added patch 2/2 which validates the payload lengths (so nobody can
  read or write data beyond rx_buf or tx_buf). This optional and patch
  1/2 can be applied without it

Martin Blumenstingl (1):
  firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI
    firmwares

 drivers/firmware/arm_scpi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.10.2

[PATCH v3] firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI firmwares

[Martin Blumenstingl]

The pre-1.0 SCPI firmwares are using one __le32 as sensor value, while
the 1.0 SCPI protocol uses two __le32 as sensor values (a total of
64bit, split into 32bit upper and 32bit lower value).
Using an "struct sensor_value" to read the sensor value on a pre-1.0
SCPI firmware gives garbage in the "hi_val" field. Introducing a
separate function which handles scpi_ops.sensor_get_value for pre-1.0
SCPI firmware implementations ensures that we do not read memory which
was not written by the SCPI firmware (which fixes for example the
temperature reported by scpi-hwmon).

Suggested-by: Sudeep Holla <Sudeep.Holla@arm.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/firmware/arm_scpi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 70e1323..9ad0b19 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -721,11 +721,17 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
 
 	ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
 				&buf, sizeof(buf));
-	if (!ret)
+	if (ret)
+		return ret;
+
+	if (scpi_info->is_legacy)
+		/* only 32-bits supported, hi_val can be junk */
+		*val = le32_to_cpu(buf.lo_val);
+	else
 		*val = (u64)le32_to_cpu(buf.hi_val) << 32 |
 			le32_to_cpu(buf.lo_val);
 
-	return ret;
+	return 0;
 }
 
 static int scpi_device_get_power_state(u16 dev_id)
-- 
2.10.2

[PATCH v3] firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI firmwares

[Sudeep Holla]

On 11/12/16 21:14, Martin Blumenstingl wrote:
> The pre-1.0 SCPI firmwares are using one __le32 as sensor value, while
> the 1.0 SCPI protocol uses two __le32 as sensor values (a total of
> 64bit, split into 32bit upper and 32bit lower value).
> Using an "struct sensor_value" to read the sensor value on a pre-1.0
> SCPI firmware gives garbage in the "hi_val" field. Introducing a
> separate function which handles scpi_ops.sensor_get_value for pre-1.0
> SCPI firmware implementations ensures that we do not read memory which
> was not written by the SCPI firmware (which fixes for example the
> temperature reported by scpi-hwmon).
> 
> Suggested-by: Sudeep Holla <Sudeep.Holla@arm.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Will send to ARM SoC once the SCPI changes land in mainline or post -rc1.

-- 
Regards,
Sudeep