[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 at 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 at 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