* [RFC PATCH 0/2] Sensor readings fixes @ 2021-12-20 17:41 Cristian Marussi 2021-12-20 17:41 ` [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get Cristian Marussi ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Cristian Marussi @ 2021-12-20 17:41 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: sudeep.holla, f.fainelli, souvik.chakravarty, nicola.mazzucato, Cristian Marussi 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 ! Thanks, Cristian Cristian Marussi (2): firmware: arm_scmi: Filter out negative results on scmi_reading_get hwmon: (scmi) Filter out results wrongly interpreted as negatives drivers/firmware/arm_scmi/sensors.c | 8 ++++++++ drivers/hwmon/scmi-hwmon.c | 14 ++++++++++++++ 2 files changed, 22 insertions(+) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get 2021-12-20 17:41 [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi @ 2021-12-20 17:41 ` Cristian Marussi 2021-12-20 17:41 ` [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives Cristian Marussi 2022-03-25 11:41 ` [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi 2 siblings, 0 replies; 6+ messages in thread From: Cristian Marussi @ 2021-12-20 17:41 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: sudeep.holla, f.fainelli, souvik.chakravarty, nicola.mazzucato, Cristian Marussi When the backing SCMI Platform supports a Sensor protocol whose version is greater than SCMI v2.0 (0x10000), the underlying SCMI SENSOR_READING_GET command will report sensors readings no more as unsigned but as signed values. A new scmi_reading_get_timestamped operation was added to properly handle this and other changes like timestamps and multi-axis sensors; on the other side the Sensor scmi_reading_get protocol operation was kepts as it was for backward compatibility and so it stil reports values as unsigned, but no check was added to detect the situation in which a newer FW could try to report negative values over an unsigned quantity. Filter out negative values in scmi_reading_get, reporting an error, when the SCMI FW version is greater than V2.0 and a negative value has been received. Reported-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- Note that, till now, HWMON SCMI driver was silently interpreting u64 as s64, so after this change whenever a FW >2.0 is used it won't be possible anymore to report negative values in HWMON. --- drivers/firmware/arm_scmi/sensors.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c index cdbb287bd8bc..f3952f1d9f61 100644 --- a/drivers/firmware/arm_scmi/sensors.c +++ b/drivers/firmware/arm_scmi/sensors.c @@ -730,6 +730,14 @@ static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph, *value = get_unaligned_le64(t->rx.buf); } + if (!ret && si->version > SCMIv2_SENSOR_PROTOCOL && *value & BIT(63)) { + dev_warn_once(ph->dev, + "SCMI FW Sensor version:0x%X reported negative value %ld\n", + si->version, (long)*value); + *value = 0; + ret = -EIO; + } + ph->xops->xfer_put(ph, t); return ret; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives 2021-12-20 17:41 [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi 2021-12-20 17:41 ` [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get Cristian Marussi @ 2021-12-20 17:41 ` Cristian Marussi 2022-03-25 11:41 ` [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi 2 siblings, 0 replies; 6+ messages in thread From: Cristian Marussi @ 2021-12-20 17:41 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: sudeep.holla, f.fainelli, souvik.chakravarty, nicola.mazzucato, Cristian Marussi SCMI Sensor scmi_reading_get interface can report only unsigned values while hwmon_ops.read allows for signed negative values to be passed on: this has the undesired side effect of silently interpreting as negative any big-enough positive reading reported by the SCMI interface. Filter out such big positives reporting an error. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/hwmon/scmi-hwmon.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c index b1329a58ce40..0924d1b8a9ce 100644 --- a/drivers/hwmon/scmi-hwmon.c +++ b/drivers/hwmon/scmi-hwmon.c @@ -73,10 +73,24 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, struct scmi_sensors *scmi_sensors = dev_get_drvdata(dev); sensor = *(scmi_sensors->info[type] + channel); + /* + * Can fail with EIO if the backing SCMI Sensor FW version tried to + * report a negative value. + */ ret = sensor_ops->reading_get(scmi_sensors->ph, sensor->id, &value); if (ret) return ret; + /* + * Cannot accept either valid positive values so big that would be + * interpreted as negative by HWMON signed long *val return value. + */ + if (value & BIT(63)) { + dev_warn_once(dev, + "Reported unsigned value too big.\n"); + return -EIO; + } + ret = scmi_hwmon_scale(sensor, &value); if (!ret) *val = value; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes 2021-12-20 17:41 [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi 2021-12-20 17:41 ` [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get Cristian Marussi 2021-12-20 17:41 ` [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives Cristian Marussi @ 2022-03-25 11:41 ` Cristian Marussi 2022-03-25 21:38 ` Florian Fainelli 2 siblings, 1 reply; 6+ messages in thread From: Cristian Marussi @ 2022-03-25 11:41 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: sudeep.holla, f.fainelli, souvik.chakravarty, nicola.mazzucato, cristian.marussi 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) Thanks, Cristian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes 2022-03-25 11:41 ` [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi @ 2022-03-25 21:38 ` Florian Fainelli 2022-03-30 14:43 ` Cristian Marussi 0 siblings, 1 reply; 6+ messages in thread From: Florian Fainelli @ 2022-03-25 21:38 UTC (permalink / raw) To: Cristian Marussi, linux-kernel, linux-arm-kernel Cc: sudeep.holla, souvik.chakravarty, nicola.mazzucato 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes 2022-03-25 21:38 ` Florian Fainelli @ 2022-03-30 14:43 ` Cristian Marussi 0 siblings, 0 replies; 6+ messages in thread From: Cristian Marussi @ 2022-03-30 14:43 UTC (permalink / raw) To: Florian Fainelli Cc: linux-kernel, linux-arm-kernel, sudeep.holla, souvik.chakravarty, nicola.mazzucato On Fri, Mar 25, 2022 at 02:38:06PM -0700, Florian Fainelli wrote: > 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: > Hi Florian, thanks for the feedback first of all... > 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)) { > ^~~ > ..and sorry that the series does not seem in good shape... > 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 > So this is my patch apparently breaking things....which was what I wanted to verify really :P ... the thing is that up till SCMI v2.0 (Sensor Vers <= 0x10000) the SENSOR_READING_GET command returned a single u32 reading-value by the spec, after that, starting with SCMIv3.0, the SENSOR_READING_GET returned also a timestamp and per-axis reading-values BUT these readings are now signed s32 ! So I kept the old SCMI interface as it was (used by HWMON): int (*reading_get)(const struct scmi_protocol_handle *ph, u32 sensor_id, u64 *value); and introduced a new one for timestamped/per-axis values provided by newer FW (used by SCMI IIO driver): int (*reading_get_timestamped)(const struct scmi_protocol_handle *ph, u32 sensor_id, u8 count, struct scmi_sensor_reading *readings); (which conveys timestamps and s32 values inside the *readings) The old interface pass back unsigned values only, in theory, BUT its only user HWMON hanldes also negatives, so, it sort of makes sense if the FW conveyed signed values inside an unsigned variable in the context of HWMON, (breaking the spec) since it cannot convey negatives in any other way... The whole point of this (broken) series was to try to see if I could sort of sanitizing the results depending on the backend FW version detected while maintaining backward compatibility...but the current approach of this series is deadly broken (as you had seen :<) since makes impossible really at the end for the FW to convey negative values as a whole... I'll have a thought about it but I think I'll drop this series as it is... > 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 think this works because my patch is flaky :P ... but as said I'll drop this as it is now. Thanks and sorry for the noise, Cristian ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-30 14:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-20 17:41 [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi 2021-12-20 17:41 ` [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get Cristian Marussi 2021-12-20 17:41 ` [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives Cristian Marussi 2022-03-25 11:41 ` [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi 2022-03-25 21:38 ` Florian Fainelli 2022-03-30 14:43 ` Cristian Marussi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).