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

[Martin Blumenstingl]

On Thu, Nov 24, 2016 at 11:47 AM, Sudeep Holla <sudeep.holla at 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