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

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 <[email protected]>

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

drivers/firmware/arm_scpi.c +3 -0

index 70e1323..19f750d 100644
@@ -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);

Quoted 33 lines · 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 <[email protected]>

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

drivers/firmware/arm_scpi.c +10 -0

index 70e1323..19f750d 100644
@@ -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);

drivers/firmware/arm_scpi.c static int scpi_sensor_get_value(u16 sensor, u64 *val) · line 743

Quoted 3 lines

drivers/firmware/arm_scpi.c +3 -0

+
+     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.

drivers/firmware/arm_scpi.c +4 -1

index 70e13230d8db..04aa873205e9 100644
@@ -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 */

drivers/firmware/arm_scpi.c +3 -3

+               *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

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.

Quoted 6 lines · 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

Quoted 6 lines

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.

Quoted 17 lines · Sudeep Holla

Quoted 6 lines · 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

Quoted 6 lines

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.

Quoted 6 lines

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).

Quoted 34 lines · Martin Blumenstingl

Quoted 17 lines · Sudeep Holla

Quoted 6 lines · 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

Quoted 6 lines

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.

Quoted 6 lines

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).

Quoted 9 lines

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).

Hello Sudeep,

Quoted 45 lines · Martin Blumenstingl

Quoted 43 lines · Martin Blumenstingl

Quoted 17 lines · Sudeep Holla

Quoted 6 lines · 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

Quoted 6 lines

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.

Quoted 6 lines

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).

Quoted 5 lines

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?

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 <[email protected]>

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

drivers/firmware/arm_scpi.c +5 -0

index 70e1323..8c183d8 100644
@@ -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

Quoted 42 lines · 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 <[email protected]>

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

drivers/firmware/arm_scpi.c +11 -0

index 70e1323..8c183d8 100644
@@ -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

drivers/firmware/arm_scpi.c static void scpi_tx_prepare(struct mbox_client *c, void *msg) · line 487

Quoted 4 lines

drivers/firmware/arm_scpi.c +4 -0

+              * 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.

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 <[email protected]>

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

drivers/firmware/arm_scpi.c +0 -0

index 70e1323..8c183d8 100644

drivers/firmware/arm_scpi.c +4 -0

 	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

drivers/firmware/arm_scpi.c +3 -1

 		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;

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 <[email protected]>

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

drivers/firmware/arm_scpi.c +0 -0

index 8c183d8..78ea8c7 100644

drivers/firmware/arm_scpi.c +1 -0

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

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 <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>