[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 at googlemail.com> wrote:
>> On Thu, Nov 24, 2016 at 12:15 PM, Martin Blumenstingl
>> <martin.blumenstingl at googlemail.com> wrote:
>>> 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).
>> 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