Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <faf91200-4790-1210-7ba5-7892c98fcb5e@gmail.com>
Date:   Fri, 25 Mar 2022 14:38:06 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC PATCH 0/2] Sensor readings fixes
Content-Language: en-US
To:     Cristian Marussi <cristian.marussi@arm.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Cc:     sudeep.holla@arm.com, souvik.chakravarty@arm.com,
        nicola.mazzucato@arm.com
References: <20211220174155.40239-1-cristian.marussi@arm.com>
 <Yj2qTMcW9sfMyvAc@e120937-lin>
From:   Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <Yj2qTMcW9sfMyvAc@e120937-lin>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/25/22 04:41, Cristian Marussi wrote:
> On Mon, Dec 20, 2021 at 05:41:53PM +0000, Cristian Marussi wrote:
>> Hi,
>>
>> this was supposed to be an easy fix on how sensor readings are handled
>> across different FW versions while maintaining backward compatibility,
>> but the solution raised for me more questions than the issue itself...
>> ...so I posted as an RFC.
>>
>> In a nutshell, since SCMI FWv3.0 spec, sensors SCMI_READING_GET command
>> can report axis and timestamps too, beside readings, so a brand new
>> scmi_reading_get_timestamped protocol operation was exposed (used by IIO)
>> while the old scmi_reading_get was kept as it was, already used by HWMON
>> subsystem for other classes of sensors.
>>
>> Unfortunately, also the flavour of reported values changed from unsigned
>> to signed with v3.0, so if you end-up on a system running against an SCMI
>> v3.0 FW platform you could end up reading a negative value and interpreting
>> it as a big positive since scmi_reading_get reports only u64.
>>
>> 01/02 simply takes care, when a FW >= 3.0 is detected, to return an error
>> to any scmi_reading_get request if that would result in tryinh to carry
>> a negative value into an u64.
>>
>> So this should rectify the API exposed by SCMI sensor and make it
>> consistent in general, in such a way that a user calling it won't risk to
>> receive a false big-positive which was indeed a 2-complement negative from
>> the perpective of the SCMI fw.
>> 	
>> So far so good...sort of...since, to make things more dire, the HWMON
>> interface, which is the only current upstream user of scmi_reading_get
>> DOES allow indeed to report to the HWMON core negative values, so it was
>> just that we were silently interpreting u64 as s64 :P ...
>>
>> ...as a consequence the fix above to the SCMI API will potentially break
>> this undocumented behaviour of our only scmi_reading_get user.
>>
>> Additionally, while looking at this, I realized that for similar reasons
>> even on systems running the current SCMI stack API and an old FW <=2.0
>> the current HWMON read is potentially broken, since when the FW reports
>> a very big and real positive number we'll report it as a signed long to
>> the HWMON core, so turning it wrongly into a negative report: for this
>> reason 02/02 adds a check inside scmi-hwmon to filter out, reporting
>> errors, any result reported by scmi_reading_get so big as to be considered
>> a negative in 2-complement...
>>
>> ...and this will probably break even more the undocumented behaviours...
>>
>> Any feedback welcome !
> 
> Hi,
> 
> any feedback on this ? (...before I forgot again :D)

Sorry for the lag, I threw these into a build and the first thing that 
popped is the following warning on a 32-bit ARM build:

In file included from ./include/linux/bits.h:6,
                  from ./include/linux/bitops.h:6,
                  from ./include/linux/hwmon.h:15,
                  from drivers/hwmon/scmi-hwmon.c:9:
drivers/hwmon/scmi-hwmon.c: In function 'scmi_hwmon_read':
./include/vdso/bits.h:7:26: warning: left shift count >= width of type 
[-Wshift-count-overflow]
  #define BIT(nr)   (UL(1) << (nr))
                           ^~
drivers/hwmon/scmi-hwmon.c:88:14: note: in expansion of macro 'BIT'
   if (value & BIT(63)) {
               ^~~

Now, in terms of functional testing it did seems to work as intended for 
32-bit kernels not for 64-bit kernels where I got:

# sensors
scmi_sensors-virtual-0
Adapter: Virtual device
[   16.413590] hwmon hwmon0: Reported unsigned value too big.
ERROR: Can't get value of subfeature temp1_input: I/O error
avs_pvt_temp:         N/A
pmic_die_temp:    +53.4 C

whereas 32-bit would return the following:

# sensors
scmi_sensors-virtual-0
Adapter: Virtual device
avs_pvt_temp:      -6.7 C
pmic_die_temp:    +52.3 C

The firmware is version 1:

[    0.044969] arm-scmi brcm_scmi@0: SCMI Protocol v1.0 'brcm-scmi:' 
Firmware version 0x1

I will need to revisit the specification to make sure I implemented it 
correctly the first time in our version of TF-A.
-- 
Florian