All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/2] Sensor readings fixes
@ 2021-12-20 17:41 ` Cristian Marussi
  0 siblings, 0 replies; 17+ 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] 17+ messages in thread
* [RFC PATCH 0/2] Sensor readings fixes
@ 2021-12-20 17:41 ` Cristian Marussi
  0 siblings, 0 replies; 17+ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get
  2021-12-20 17:41 ` Cristian Marussi
@ 2021-12-20 17:41   ` Cristian Marussi
  -1 siblings, 0 replies; 17+ 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] 17+ messages in thread
* [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get
@ 2021-12-20 17:41   ` Cristian Marussi
  0 siblings, 0 replies; 17+ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives
  2021-12-20 17:41 ` Cristian Marussi
@ 2021-12-20 17:41   ` Cristian Marussi
  -1 siblings, 0 replies; 17+ 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] 17+ messages in thread
* [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives
@ 2021-12-20 17:41   ` Cristian Marussi
  0 siblings, 0 replies; 17+ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get
  2021-12-20 17:41   ` Cristian Marussi
@ 2021-12-20 22:28     ` kernel test robot
  -1 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-12-20 22:28 UTC (permalink / raw)
  To: Cristian Marussi; +Cc: llvm, kbuild-all

Hi Cristian,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linux/master]
[also build test WARNING on groeck-staging/hwmon-next soc/for-next linus/master v5.16-rc6 next-20211220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Sensor-readings-fixes/20211221-014235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
config: hexagon-randconfig-r041-20211220 (https://download.01.org/0day-ci/archive/20211221/202112210649.czZr6hoM-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 555eacf75f21cd1dfc6363d73ad187b730349543)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6f0ec7d066a305bdb889a75e772ef0be72f365f9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cristian-Marussi/Sensor-readings-fixes/20211221-014235
        git checkout 6f0ec7d066a305bdb889a75e772ef0be72f365f9
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/firmware/arm_scmi/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/firmware/arm_scmi/sensors.c:733:63: warning: shift count >= width of type [-Wshift-count-overflow]
           if (!ret && si->version > SCMIv2_SENSOR_PROTOCOL && *value & BIT(63)) {
                                                                        ^~~~~~~
   include/vdso/bits.h:7:26: note: expanded from macro 'BIT'
   #define BIT(nr)                 (UL(1) << (nr))
                                          ^  ~~~~
   1 warning generated.


vim +733 drivers/firmware/arm_scmi/sensors.c

   681	
   682	/**
   683	 * scmi_sensor_reading_get  - Read scalar sensor value
   684	 * @ph: Protocol handle
   685	 * @sensor_id: Sensor ID
   686	 * @value: The 64bit value sensor reading
   687	 *
   688	 * This function returns a single 64 bit reading value representing the sensor
   689	 * value; if the platform SCMI Protocol implementation and the sensor support
   690	 * multiple axis and timestamped-reads, this just returns the first axis while
   691	 * dropping the timestamp value.
   692	 * Use instead the @scmi_sensor_reading_get_timestamped to retrieve the array of
   693	 * timestamped multi-axis values.
   694	 *
   695	 * Return: 0 on Success
   696	 */
   697	static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph,
   698					   u32 sensor_id, u64 *value)
   699	{
   700		int ret;
   701		struct scmi_xfer *t;
   702		struct scmi_msg_sensor_reading_get *sensor;
   703		struct sensors_info *si = ph->get_priv(ph);
   704		struct scmi_sensor_info *s = si->sensors + sensor_id;
   705	
   706		ret = ph->xops->xfer_get_init(ph, SENSOR_READING_GET,
   707					      sizeof(*sensor), 0, &t);
   708		if (ret)
   709			return ret;
   710	
   711		sensor = t->tx.buf;
   712		sensor->id = cpu_to_le32(sensor_id);
   713		if (s->async) {
   714			sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
   715			ret = ph->xops->do_xfer_with_response(ph, t);
   716			if (!ret) {
   717				struct scmi_resp_sensor_reading_complete *resp;
   718	
   719				resp = t->rx.buf;
   720				if (le32_to_cpu(resp->id) == sensor_id)
   721					*value =
   722						get_unaligned_le64(&resp->readings_low);
   723				else
   724					ret = -EPROTO;
   725			}
   726		} else {
   727			sensor->flags = cpu_to_le32(0);
   728			ret = ph->xops->do_xfer(ph, t);
   729			if (!ret)
   730				*value = get_unaligned_le64(t->rx.buf);
   731		}
   732	
 > 733		if (!ret && si->version > SCMIv2_SENSOR_PROTOCOL && *value & BIT(63)) {
   734			dev_warn_once(ph->dev,
   735				      "SCMI FW Sensor version:0x%X reported negative value %ld\n",
   736				      si->version, (long)*value);
   737			*value = 0;
   738			ret = -EIO;
   739		}
   740	
   741		ph->xops->xfer_put(ph, t);
   742		return ret;
   743	}
   744	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get
@ 2021-12-20 22:28     ` kernel test robot
  0 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-12-20 22:28 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 4688 bytes --]

Hi Cristian,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linux/master]
[also build test WARNING on groeck-staging/hwmon-next soc/for-next linus/master v5.16-rc6 next-20211220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Sensor-readings-fixes/20211221-014235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
config: hexagon-randconfig-r041-20211220 (https://download.01.org/0day-ci/archive/20211221/202112210649.czZr6hoM-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 555eacf75f21cd1dfc6363d73ad187b730349543)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6f0ec7d066a305bdb889a75e772ef0be72f365f9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cristian-Marussi/Sensor-readings-fixes/20211221-014235
        git checkout 6f0ec7d066a305bdb889a75e772ef0be72f365f9
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/firmware/arm_scmi/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/firmware/arm_scmi/sensors.c:733:63: warning: shift count >= width of type [-Wshift-count-overflow]
           if (!ret && si->version > SCMIv2_SENSOR_PROTOCOL && *value & BIT(63)) {
                                                                        ^~~~~~~
   include/vdso/bits.h:7:26: note: expanded from macro 'BIT'
   #define BIT(nr)                 (UL(1) << (nr))
                                          ^  ~~~~
   1 warning generated.


vim +733 drivers/firmware/arm_scmi/sensors.c

   681	
   682	/**
   683	 * scmi_sensor_reading_get  - Read scalar sensor value
   684	 * @ph: Protocol handle
   685	 * @sensor_id: Sensor ID
   686	 * @value: The 64bit value sensor reading
   687	 *
   688	 * This function returns a single 64 bit reading value representing the sensor
   689	 * value; if the platform SCMI Protocol implementation and the sensor support
   690	 * multiple axis and timestamped-reads, this just returns the first axis while
   691	 * dropping the timestamp value.
   692	 * Use instead the @scmi_sensor_reading_get_timestamped to retrieve the array of
   693	 * timestamped multi-axis values.
   694	 *
   695	 * Return: 0 on Success
   696	 */
   697	static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph,
   698					   u32 sensor_id, u64 *value)
   699	{
   700		int ret;
   701		struct scmi_xfer *t;
   702		struct scmi_msg_sensor_reading_get *sensor;
   703		struct sensors_info *si = ph->get_priv(ph);
   704		struct scmi_sensor_info *s = si->sensors + sensor_id;
   705	
   706		ret = ph->xops->xfer_get_init(ph, SENSOR_READING_GET,
   707					      sizeof(*sensor), 0, &t);
   708		if (ret)
   709			return ret;
   710	
   711		sensor = t->tx.buf;
   712		sensor->id = cpu_to_le32(sensor_id);
   713		if (s->async) {
   714			sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
   715			ret = ph->xops->do_xfer_with_response(ph, t);
   716			if (!ret) {
   717				struct scmi_resp_sensor_reading_complete *resp;
   718	
   719				resp = t->rx.buf;
   720				if (le32_to_cpu(resp->id) == sensor_id)
   721					*value =
   722						get_unaligned_le64(&resp->readings_low);
   723				else
   724					ret = -EPROTO;
   725			}
   726		} else {
   727			sensor->flags = cpu_to_le32(0);
   728			ret = ph->xops->do_xfer(ph, t);
   729			if (!ret)
   730				*value = get_unaligned_le64(t->rx.buf);
   731		}
   732	
 > 733		if (!ret && si->version > SCMIv2_SENSOR_PROTOCOL && *value & BIT(63)) {
   734			dev_warn_once(ph->dev,
   735				      "SCMI FW Sensor version:0x%X reported negative value %ld\n",
   736				      si->version, (long)*value);
   737			*value = 0;
   738			ret = -EIO;
   739		}
   740	
   741		ph->xops->xfer_put(ph, t);
   742		return ret;
   743	}
   744	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives
  2021-12-20 17:41   ` Cristian Marussi
@ 2021-12-21  3:54     ` kernel test robot
  -1 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-12-21  3:54 UTC (permalink / raw)
  To: Cristian Marussi; +Cc: llvm, kbuild-all

Hi Cristian,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linux/master]
[also build test WARNING on groeck-staging/hwmon-next soc/for-next linus/master v5.16-rc6 next-20211220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Sensor-readings-fixes/20211221-014235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
config: arm-randconfig-r031-20211220 (https://download.01.org/0day-ci/archive/20211221/202112211141.DiW591Y0-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 555eacf75f21cd1dfc6363d73ad187b730349543)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/6a3ccc22734d65dfe13f598a7469c4ca3094eeea
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cristian-Marussi/Sensor-readings-fixes/20211221-014235
        git checkout 6a3ccc22734d65dfe13f598a7469c4ca3094eeea
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/hwmon/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/hwmon/scmi-hwmon.c:88:14: warning: shift count >= width of type [-Wshift-count-overflow]
           if (value & BIT(63)) {
                       ^~~~~~~
   include/vdso/bits.h:7:26: note: expanded from macro 'BIT'
   #define BIT(nr)                 (UL(1) << (nr))
                                          ^  ~~~~
   1 warning generated.


vim +88 drivers/hwmon/scmi-hwmon.c

    66	
    67	static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
    68				   u32 attr, int channel, long *val)
    69	{
    70		int ret;
    71		u64 value;
    72		const struct scmi_sensor_info *sensor;
    73		struct scmi_sensors *scmi_sensors = dev_get_drvdata(dev);
    74	
    75		sensor = *(scmi_sensors->info[type] + channel);
    76		/*
    77		 * Can fail with EIO if the backing SCMI Sensor FW version tried to
    78		 * report a negative value.
    79		 */
    80		ret = sensor_ops->reading_get(scmi_sensors->ph, sensor->id, &value);
    81		if (ret)
    82			return ret;
    83	
    84		/*
    85		 * Cannot accept either valid positive values so big that would be
    86		 * interpreted as negative by HWMON signed long *val return value.
    87		 */
  > 88		if (value & BIT(63)) {
    89			dev_warn_once(dev,
    90				      "Reported unsigned value too big.\n");
    91			return -EIO;
    92		}
    93	
    94		ret = scmi_hwmon_scale(sensor, &value);
    95		if (!ret)
    96			*val = value;
    97	
    98		return ret;
    99	}
   100	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives
@ 2021-12-21  3:54     ` kernel test robot
  0 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-12-21  3:54 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3483 bytes --]

Hi Cristian,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linux/master]
[also build test WARNING on groeck-staging/hwmon-next soc/for-next linus/master v5.16-rc6 next-20211220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Sensor-readings-fixes/20211221-014235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
config: arm-randconfig-r031-20211220 (https://download.01.org/0day-ci/archive/20211221/202112211141.DiW591Y0-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 555eacf75f21cd1dfc6363d73ad187b730349543)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/6a3ccc22734d65dfe13f598a7469c4ca3094eeea
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cristian-Marussi/Sensor-readings-fixes/20211221-014235
        git checkout 6a3ccc22734d65dfe13f598a7469c4ca3094eeea
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/hwmon/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/hwmon/scmi-hwmon.c:88:14: warning: shift count >= width of type [-Wshift-count-overflow]
           if (value & BIT(63)) {
                       ^~~~~~~
   include/vdso/bits.h:7:26: note: expanded from macro 'BIT'
   #define BIT(nr)                 (UL(1) << (nr))
                                          ^  ~~~~
   1 warning generated.


vim +88 drivers/hwmon/scmi-hwmon.c

    66	
    67	static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
    68				   u32 attr, int channel, long *val)
    69	{
    70		int ret;
    71		u64 value;
    72		const struct scmi_sensor_info *sensor;
    73		struct scmi_sensors *scmi_sensors = dev_get_drvdata(dev);
    74	
    75		sensor = *(scmi_sensors->info[type] + channel);
    76		/*
    77		 * Can fail with EIO if the backing SCMI Sensor FW version tried to
    78		 * report a negative value.
    79		 */
    80		ret = sensor_ops->reading_get(scmi_sensors->ph, sensor->id, &value);
    81		if (ret)
    82			return ret;
    83	
    84		/*
    85		 * Cannot accept either valid positive values so big that would be
    86		 * interpreted as negative by HWMON signed long *val return value.
    87		 */
  > 88		if (value & BIT(63)) {
    89			dev_warn_once(dev,
    90				      "Reported unsigned value too big.\n");
    91			return -EIO;
    92		}
    93	
    94		ret = scmi_hwmon_scale(sensor, &value);
    95		if (!ret)
    96			*val = value;
    97	
    98		return ret;
    99	}
   100	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes
  2021-12-20 17:41 ` Cristian Marussi
@ 2022-03-25 11:41   ` Cristian Marussi
  -1 siblings, 0 replies; 17+ 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] 17+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes
@ 2022-03-25 11:41   ` Cristian Marussi
  0 siblings, 0 replies; 17+ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes
  2022-03-25 11:41   ` Cristian Marussi
@ 2022-03-25 21:38     ` Florian Fainelli
  -1 siblings, 0 replies; 17+ 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] 17+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes
@ 2022-03-25 21:38     ` Florian Fainelli
  0 siblings, 0 replies; 17+ 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ 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
  -1 siblings, 0 replies; 17+ 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] 17+ messages in thread
* Re: [RFC PATCH 0/2] Sensor readings fixes
@ 2022-03-30 14:43       ` Cristian Marussi
  0 siblings, 0 replies; 17+ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get
@ 2021-12-22 12:49 kernel test robot
  0 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-12-22 12:49 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 11607 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211220174155.40239-2-cristian.marussi@arm.com>
References: <20211220174155.40239-2-cristian.marussi@arm.com>
TO: Cristian Marussi <cristian.marussi@arm.com>

Hi Cristian,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linux/master]
[also build test WARNING on groeck-staging/hwmon-next soc/for-next linus/master v5.16-rc6 next-20211221]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cristian-Marussi/Sensor-readings-fixes/20211221-014235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: csky-randconfig-s031-20211222 (https://download.01.org/0day-ci/archive/20211222/202112222052.WENrQg3R-lkp(a)intel.com/config)
compiler: csky-linux-gcc (GCC) 11.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/6f0ec7d066a305bdb889a75e772ef0be72f365f9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cristian-Marussi/Sensor-readings-fixes/20211221-014235
        git checkout 6f0ec7d066a305bdb889a75e772ef0be72f365f9
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=csky SHELL=/bin/bash drivers/firmware/arm_scmi/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   drivers/firmware/arm_scmi/sensors.c:640:28: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] val @@     got restricted __le32 [usertype] @@
   drivers/firmware/arm_scmi/sensors.c:640:28: sparse:     expected unsigned int [usertype] val
   drivers/firmware/arm_scmi/sensors.c:640:28: sparse:     got restricted __le32 [usertype]
>> drivers/firmware/arm_scmi/sensors.c:733:70: sparse: sparse: shift too big (63) for type unsigned long

vim +733 drivers/firmware/arm_scmi/sensors.c

5179c523c1eae4 Sudeep Holla     2017-06-06  628  
9694a7f6235934 Cristian Marussi 2021-03-16  629  static int scmi_sensor_config_get(const struct scmi_protocol_handle *ph,
7b83c5f4108898 Cristian Marussi 2020-11-19  630  				  u32 sensor_id, u32 *sensor_config)
7b83c5f4108898 Cristian Marussi 2020-11-19  631  {
7b83c5f4108898 Cristian Marussi 2020-11-19  632  	int ret;
7b83c5f4108898 Cristian Marussi 2020-11-19  633  	struct scmi_xfer *t;
7b83c5f4108898 Cristian Marussi 2020-11-19  634  
9694a7f6235934 Cristian Marussi 2021-03-16  635  	ret = ph->xops->xfer_get_init(ph, SENSOR_CONFIG_GET,
9694a7f6235934 Cristian Marussi 2021-03-16  636  				      sizeof(__le32), sizeof(__le32), &t);
7b83c5f4108898 Cristian Marussi 2020-11-19  637  	if (ret)
7b83c5f4108898 Cristian Marussi 2020-11-19  638  		return ret;
7b83c5f4108898 Cristian Marussi 2020-11-19  639  
7b83c5f4108898 Cristian Marussi 2020-11-19 @640  	put_unaligned_le32(cpu_to_le32(sensor_id), t->tx.buf);
9694a7f6235934 Cristian Marussi 2021-03-16  641  	ret = ph->xops->do_xfer(ph, t);
7b83c5f4108898 Cristian Marussi 2020-11-19  642  	if (!ret) {
9694a7f6235934 Cristian Marussi 2021-03-16  643  		struct sensors_info *si = ph->get_priv(ph);
7b83c5f4108898 Cristian Marussi 2020-11-19  644  		struct scmi_sensor_info *s = si->sensors + sensor_id;
7b83c5f4108898 Cristian Marussi 2020-11-19  645  
7b83c5f4108898 Cristian Marussi 2020-11-19  646  		*sensor_config = get_unaligned_le64(t->rx.buf);
7b83c5f4108898 Cristian Marussi 2020-11-19  647  		s->sensor_config = *sensor_config;
7b83c5f4108898 Cristian Marussi 2020-11-19  648  	}
7b83c5f4108898 Cristian Marussi 2020-11-19  649  
9694a7f6235934 Cristian Marussi 2021-03-16  650  	ph->xops->xfer_put(ph, t);
7b83c5f4108898 Cristian Marussi 2020-11-19  651  	return ret;
7b83c5f4108898 Cristian Marussi 2020-11-19  652  }
7b83c5f4108898 Cristian Marussi 2020-11-19  653  
9694a7f6235934 Cristian Marussi 2021-03-16  654  static int scmi_sensor_config_set(const struct scmi_protocol_handle *ph,
7b83c5f4108898 Cristian Marussi 2020-11-19  655  				  u32 sensor_id, u32 sensor_config)
7b83c5f4108898 Cristian Marussi 2020-11-19  656  {
7b83c5f4108898 Cristian Marussi 2020-11-19  657  	int ret;
7b83c5f4108898 Cristian Marussi 2020-11-19  658  	struct scmi_xfer *t;
7b83c5f4108898 Cristian Marussi 2020-11-19  659  	struct scmi_msg_sensor_config_set *msg;
7b83c5f4108898 Cristian Marussi 2020-11-19  660  
9694a7f6235934 Cristian Marussi 2021-03-16  661  	ret = ph->xops->xfer_get_init(ph, SENSOR_CONFIG_SET,
9694a7f6235934 Cristian Marussi 2021-03-16  662  				      sizeof(*msg), 0, &t);
7b83c5f4108898 Cristian Marussi 2020-11-19  663  	if (ret)
7b83c5f4108898 Cristian Marussi 2020-11-19  664  		return ret;
7b83c5f4108898 Cristian Marussi 2020-11-19  665  
7b83c5f4108898 Cristian Marussi 2020-11-19  666  	msg = t->tx.buf;
7b83c5f4108898 Cristian Marussi 2020-11-19  667  	msg->id = cpu_to_le32(sensor_id);
7b83c5f4108898 Cristian Marussi 2020-11-19  668  	msg->sensor_config = cpu_to_le32(sensor_config);
7b83c5f4108898 Cristian Marussi 2020-11-19  669  
9694a7f6235934 Cristian Marussi 2021-03-16  670  	ret = ph->xops->do_xfer(ph, t);
7b83c5f4108898 Cristian Marussi 2020-11-19  671  	if (!ret) {
9694a7f6235934 Cristian Marussi 2021-03-16  672  		struct sensors_info *si = ph->get_priv(ph);
7b83c5f4108898 Cristian Marussi 2020-11-19  673  		struct scmi_sensor_info *s = si->sensors + sensor_id;
7b83c5f4108898 Cristian Marussi 2020-11-19  674  
7b83c5f4108898 Cristian Marussi 2020-11-19  675  		s->sensor_config = sensor_config;
7b83c5f4108898 Cristian Marussi 2020-11-19  676  	}
7b83c5f4108898 Cristian Marussi 2020-11-19  677  
9694a7f6235934 Cristian Marussi 2021-03-16  678  	ph->xops->xfer_put(ph, t);
7b83c5f4108898 Cristian Marussi 2020-11-19  679  	return ret;
7b83c5f4108898 Cristian Marussi 2020-11-19  680  }
7b83c5f4108898 Cristian Marussi 2020-11-19  681  
e2083d36739168 Cristian Marussi 2020-11-19  682  /**
e2083d36739168 Cristian Marussi 2020-11-19  683   * scmi_sensor_reading_get  - Read scalar sensor value
9694a7f6235934 Cristian Marussi 2021-03-16  684   * @ph: Protocol handle
e2083d36739168 Cristian Marussi 2020-11-19  685   * @sensor_id: Sensor ID
e2083d36739168 Cristian Marussi 2020-11-19  686   * @value: The 64bit value sensor reading
e2083d36739168 Cristian Marussi 2020-11-19  687   *
e2083d36739168 Cristian Marussi 2020-11-19  688   * This function returns a single 64 bit reading value representing the sensor
e2083d36739168 Cristian Marussi 2020-11-19  689   * value; if the platform SCMI Protocol implementation and the sensor support
e2083d36739168 Cristian Marussi 2020-11-19  690   * multiple axis and timestamped-reads, this just returns the first axis while
e2083d36739168 Cristian Marussi 2020-11-19  691   * dropping the timestamp value.
e2083d36739168 Cristian Marussi 2020-11-19  692   * Use instead the @scmi_sensor_reading_get_timestamped to retrieve the array of
e2083d36739168 Cristian Marussi 2020-11-19  693   * timestamped multi-axis values.
e2083d36739168 Cristian Marussi 2020-11-19  694   *
e2083d36739168 Cristian Marussi 2020-11-19  695   * Return: 0 on Success
e2083d36739168 Cristian Marussi 2020-11-19  696   */
9694a7f6235934 Cristian Marussi 2021-03-16  697  static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph,
6a55331c87d86a Sudeep Holla     2019-07-08  698  				   u32 sensor_id, u64 *value)
5179c523c1eae4 Sudeep Holla     2017-06-06  699  {
5179c523c1eae4 Sudeep Holla     2017-06-06  700  	int ret;
5179c523c1eae4 Sudeep Holla     2017-06-06  701  	struct scmi_xfer *t;
5179c523c1eae4 Sudeep Holla     2017-06-06  702  	struct scmi_msg_sensor_reading_get *sensor;
9694a7f6235934 Cristian Marussi 2021-03-16  703  	struct sensors_info *si = ph->get_priv(ph);
d09aac0eb17c6c Sudeep Holla     2019-07-08  704  	struct scmi_sensor_info *s = si->sensors + sensor_id;
5179c523c1eae4 Sudeep Holla     2017-06-06  705  
9694a7f6235934 Cristian Marussi 2021-03-16  706  	ret = ph->xops->xfer_get_init(ph, SENSOR_READING_GET,
9694a7f6235934 Cristian Marussi 2021-03-16  707  				      sizeof(*sensor), 0, &t);
5179c523c1eae4 Sudeep Holla     2017-06-06  708  	if (ret)
5179c523c1eae4 Sudeep Holla     2017-06-06  709  		return ret;
5179c523c1eae4 Sudeep Holla     2017-06-06  710  
5179c523c1eae4 Sudeep Holla     2017-06-06  711  	sensor = t->tx.buf;
5179c523c1eae4 Sudeep Holla     2017-06-06  712  	sensor->id = cpu_to_le32(sensor_id);
d09aac0eb17c6c Sudeep Holla     2019-07-08  713  	if (s->async) {
d09aac0eb17c6c Sudeep Holla     2019-07-08  714  		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
9694a7f6235934 Cristian Marussi 2021-03-16  715  		ret = ph->xops->do_xfer_with_response(ph, t);
e2083d36739168 Cristian Marussi 2020-11-19  716  		if (!ret) {
e2083d36739168 Cristian Marussi 2020-11-19  717  			struct scmi_resp_sensor_reading_complete *resp;
e2083d36739168 Cristian Marussi 2020-11-19  718  
e2083d36739168 Cristian Marussi 2020-11-19  719  			resp = t->rx.buf;
e2083d36739168 Cristian Marussi 2020-11-19  720  			if (le32_to_cpu(resp->id) == sensor_id)
187a002b07e808 Cristian Marussi 2021-06-28  721  				*value =
187a002b07e808 Cristian Marussi 2021-06-28  722  					get_unaligned_le64(&resp->readings_low);
e2083d36739168 Cristian Marussi 2020-11-19  723  			else
e2083d36739168 Cristian Marussi 2020-11-19  724  				ret = -EPROTO;
e2083d36739168 Cristian Marussi 2020-11-19  725  		}
d09aac0eb17c6c Sudeep Holla     2019-07-08  726  	} else {
d09aac0eb17c6c Sudeep Holla     2019-07-08  727  		sensor->flags = cpu_to_le32(0);
9694a7f6235934 Cristian Marussi 2021-03-16  728  		ret = ph->xops->do_xfer(ph, t);
aa90ac45bc88e6 Sudeep Holla     2019-08-07  729  		if (!ret)
aa90ac45bc88e6 Sudeep Holla     2019-08-07  730  			*value = get_unaligned_le64(t->rx.buf);
d09aac0eb17c6c Sudeep Holla     2019-07-08  731  	}
5179c523c1eae4 Sudeep Holla     2017-06-06  732  
6f0ec7d066a305 Cristian Marussi 2021-12-20 @733  	if (!ret && si->version > SCMIv2_SENSOR_PROTOCOL && *value & BIT(63)) {
6f0ec7d066a305 Cristian Marussi 2021-12-20  734  		dev_warn_once(ph->dev,
6f0ec7d066a305 Cristian Marussi 2021-12-20  735  			      "SCMI FW Sensor version:0x%X reported negative value %ld\n",
6f0ec7d066a305 Cristian Marussi 2021-12-20  736  			      si->version, (long)*value);
6f0ec7d066a305 Cristian Marussi 2021-12-20  737  		*value = 0;
6f0ec7d066a305 Cristian Marussi 2021-12-20  738  		ret = -EIO;
6f0ec7d066a305 Cristian Marussi 2021-12-20  739  	}
6f0ec7d066a305 Cristian Marussi 2021-12-20  740  
9694a7f6235934 Cristian Marussi 2021-03-16  741  	ph->xops->xfer_put(ph, t);
5179c523c1eae4 Sudeep Holla     2017-06-06  742  	return ret;
5179c523c1eae4 Sudeep Holla     2017-06-06  743  }
5179c523c1eae4 Sudeep Holla     2017-06-06  744  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-03-30 14:44 UTC | newest]

Thread overview: 17+ 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 ` 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
2021-12-20 22:28   ` kernel test robot
2021-12-20 22:28     ` kernel test robot
2021-12-20 17:41 ` [RFC PATCH 2/2] hwmon: (scmi) Filter out results wrongly interpreted as negatives Cristian Marussi
2021-12-20 17:41   ` Cristian Marussi
2021-12-21  3:54   ` kernel test robot
2021-12-21  3:54     ` kernel test robot
2022-03-25 11:41 ` [RFC PATCH 0/2] Sensor readings fixes Cristian Marussi
2022-03-25 11:41   ` Cristian Marussi
2022-03-25 21:38   ` Florian Fainelli
2022-03-25 21:38     ` Florian Fainelli
2022-03-30 14:43     ` Cristian Marussi
2022-03-30 14:43       ` Cristian Marussi
2021-12-22 12:49 [RFC PATCH 1/2] firmware: arm_scmi: Filter out negative results on scmi_reading_get kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.