| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201110170429.GD42652@e120937-lin>
Date: Tue, 10 Nov 2020 17:04:29 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Peter Hilber <peter.hilber@...nsynergy.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
sudeep.holla@....com, lukasz.luba@....com,
james.quinlan@...adcom.com, Jonathan.Cameron@...wei.com,
egranata@...gle.com, jbhayana@...gle.com,
mikhail.golubev@...nsynergy.com, Igor.Skalkin@...nsynergy.com
Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors
timestamped reads
Hi Peter
thanks for the review.
On Tue, Nov 10, 2020 at 05:01:26PM +0100, Peter Hilber wrote:
> On 26.10.20 21:10, Cristian Marussi wrote:
> > Add new .reading_get_timestamped() method to sensor_ops to support SCMIv3.0
> > timestamped reads.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> > ---
> > drivers/firmware/arm_scmi/sensors.c | 134 ++++++++++++++++++++++++++--
> > include/linux/scmi_protocol.h | 22 +++++
> > 2 files changed, 151 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> > index 5a18f8c84bef..bdb0ed0df683 100644
> > --- a/drivers/firmware/arm_scmi/sensors.c
> > +++ b/drivers/firmware/arm_scmi/sensors.c
> > @@ -156,6 +156,27 @@ struct scmi_msg_sensor_reading_get {
> > #define SENSOR_READ_ASYNC BIT(0)
> > };
> >
> > +struct scmi_resp_sensor_reading_get {
> > + __le64 readings;
> > +};
> > +
> > +struct scmi_resp_sensor_reading_complete {
> > + __le32 id;
> > + __le64 readings;
> > +};
>
> In my understanding the id field is not present in the spec. The
> implementation seems to have introduced it already before this patch.
>
Well, it is indeed defined in 4.7.3.1 "SENSOR_READING_COMPLETE" both in
SCMI V3.0 and in V2.0: it is the async delayed response and this 'id'
represents the sensor_id: in fact it is used only the in the async path
in the reading funcs; the sync version uses directly sensor_reading_le.
(which has no id n it)
> > +
> > +struct scmi_sensor_reading_le {
> > + __le32 sensor_value_low;
> > + __le32 sensor_value_high;
> > + __le32 timestamp_low;
> > + __le32 timestamp_high;
> > +};
> > +
> > +struct scmi_resp_sensor_reading_complete_v3 {
> > + __le32 id;
> > + struct scmi_sensor_reading_le readings[];
> > +};
>
> As above, IMHO the id field is not present in the spec.
>
As said above it is for the delayed_responses, in this case for V3
messages.
> > +
> > struct scmi_sensor_trip_notify_payld {
> > __le32 agent_id;
> > __le32 sensor_id;
> > @@ -576,6 +597,21 @@ scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id,
> > return ret;
> > }
> >
> > +/**
> > + * scmi_sensor_reading_get - Read scalar sensor value
> > + * @handle: Platform handle
> > + * @sensor_id: Sensor ID
> > + * @value: The 64bit value sensor reading
> > + *
> > + * This function returns a single 64 bit reading value representing the sensor
> > + * value; if the platform SCMI Protocol implementation and the sensor support
> > + * multiple axis and timestamped-reads, this just returns the first axis while
> > + * dropping the timestamp value.
> > + * Use instead the @scmi_sensor_reading_get_timestamped to retrieve the array of
> > + * timestamped multi-axis values.
> > + *
> > + * Return: 0 on Success
> > + */
> > static int scmi_sensor_reading_get(const struct scmi_handle *handle,
> > u32 sensor_id, u64 *value)
> > {
> > @@ -593,18 +629,105 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
>
> How about changing the scmi_xfer_get_init() rx_size to 0 (in the
> immediately preceding, not shown lines)? An SCMI platform might not
> expect to just have room for the first reading, excluding the timestamp.
>
Ah right, because this is the old v2.0 interface which I kept unchanged but
now internally the same v3.0 SENSOR_READING_GET message on a v3.0 platform
could return multiple per-axis timestamped values even if I just return
the first u64 without timestamp. Is this that you mean ?
I'll fix to 0.
> >
> > sensor = t->tx.buf;
> > sensor->id = cpu_to_le32(sensor_id);
> > + if (s->async) {
> > + sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
> > + ret = scmi_do_xfer_with_response(handle, t);
> > + if (!ret) {
> > + struct scmi_resp_sensor_reading_complete *resp;
> > +
> > + resp = t->rx.buf;
> > + if (le32_to_cpu(resp->id) == sensor_id)
> > + *value = le64_to_cpu(resp->readings);
>
> Maybe this le64_to_cpu() and the one a few lines below should be
> replaced by get_unaligned_le64()?
I'll check.
>
> > + else
> > + ret = -EPROTO;
> > + }
> > + } else {
> > + sensor->flags = cpu_to_le32(0);
> > + ret = scmi_do_xfer(handle, t);
> > + if (!ret) {
> > + struct scmi_resp_sensor_reading_get *resp;
> > +
> > + resp = t->rx.buf;
> > + *value = le64_to_cpu(resp->readings);
> > + }
> > + }
> >
> > + scmi_xfer_put(handle, t);
> > + return ret;
> > +}
> > +
>
> [...]
> > +
> > /**
> > * scmi_range_attrs - specifies a sensor or axis values' range
> > * @min_range: The minimum value which can be represented by the sensor/axis.
> > @@ -387,6 +401,11 @@ enum scmi_sensor_class {
> > * @info_get: get the information of the specified sensor
> > * @trip_point_config: selects and configures a trip-point of interest
> > * @reading_get: gets the current value of the sensor
> > + * @reading_get_timestamped: gets the current value and timestamp, when
> > + * available, of the sensor. (as of v2.1 spec)
>
> Nitpicking: v2.1 -> v3.0
>
Ok.
> > + * Supports multi-axis sensors for sensors which
> > + * supports it and if the @reading array size of
> > + * @count entry equals the sensor num_axis
> > */
> > struct scmi_sensor_ops {
> > int (*count_get)(const struct scmi_handle *handle);
> > @@ -396,6 +415,9 @@ struct scmi_sensor_ops {
> > u32 sensor_id, u8 trip_id, u64 trip_value);
> > int (*reading_get)(const struct scmi_handle *handle, u32 sensor_id,
> > u64 *value);
> > + int (*reading_get_timestamped)(const struct scmi_handle *handle,
> > + u32 sensor_id, u8 count,
> > + struct scmi_sensor_reading *readings);
> > };
> >
> > /**
> >
>
> Best regards,
>
> Peter
Thanks
Cristian
Powered by blists - more mailing lists