| 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: <d523569d-2470-3e01-c407-d6e723c7d0c1@opensynergy.com>
Date: Thu, 2 Jun 2022 16:25:45 +0200
From: Peter Hilber <peter.hilber@...nsynergy.com>
To: Cristian Marussi <cristian.marussi@....com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc: sudeep.holla@....com, james.quinlan@...adcom.com,
Jonathan.Cameron@...wei.com, f.fainelli@...il.com,
etienne.carriere@...aro.org, vincent.guittot@...aro.org,
souvik.chakravarty@....com
Subject: Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1
SENSOR_AXIS_NAME_GET support
On 30.03.22 17:05, Cristian Marussi wrote:
> Add support for SCMIv3.1 SENSOR_AXIS_NAME_GET multi-part command using the
> common iterator protocol helpers.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> ---
> drivers/firmware/arm_scmi/sensors.c | 82 ++++++++++++++++++++++++++---
> 1 file changed, 76 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index e1a94463d7d8..21e0ce89b153 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -28,6 +28,7 @@ enum scmi_sensor_protocol_cmd {
> SENSOR_CONFIG_SET = 0xA,
> SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
> SENSOR_NAME_GET = 0xC,
> + SENSOR_AXIS_NAME_GET = 0xD,
> };
>
> struct scmi_msg_resp_sensor_attributes {
> @@ -117,13 +118,22 @@ struct scmi_msg_resp_sensor_axis_description {
> struct scmi_axis_descriptor {
> __le32 id;
> __le32 attributes_low;
> +#define SUPPORTS_EXTENDED_AXIS_NAMES(x) FIELD_GET(BIT(9), (x))
Hi Cristian,
I saw this patch is probably going into v5.19 already, so I'm a bit late, but I
wanted to point out a compatibility issue, and a small error handling issue.
Please see below.
Best regards,
Peter
> __le32 attributes_high;
> - u8 name[SCMI_MAX_STR_SIZE];
> + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> __le32 resolution;
> struct scmi_msg_resp_attrs attrs;
> } desc[];
> };
>
> +struct scmi_msg_resp_sensor_axis_names_description {
> + __le32 num_axis_flags;
> + struct scmi_sensor_axis_name_descriptor {
> + __le32 axis_id;
> + u8 name[SCMI_MAX_STR_SIZE];
> + } desc[];
> +};
> +
> /* Base scmi_axis_descriptor size excluding extended attrs after name */
> #define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ 28
>
> @@ -393,7 +403,6 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
> a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
>
> attrh = le32_to_cpu(adesc->attributes_high);
> -
> a->scale = S32_EXT(SENSOR_SCALE(attrh));
> a->type = SENSOR_TYPE(attrh);
> strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
The strscpy() call should probably change the size parameter to
SCMI_SHORT_NAME_MAX_SIZE.
> @@ -408,15 +417,69 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
> scmi_parse_range_attrs(&a->attrs, &adesc->attrs);
> dsize += sizeof(adesc->attrs);
> }
> -
> st->priv = ((u8 *)adesc + dsize);
>
> return 0;
> }
>
> +static int
> +iter_axes_extended_name_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + u32 flags;
> + const struct scmi_msg_resp_sensor_axis_names_description *r = response;
> +
> + flags = le32_to_cpu(r->num_axis_flags);
> + st->num_returned = NUM_AXIS_RETURNED(flags);
> + st->num_remaining = NUM_AXIS_REMAINING(flags);
> + st->priv = (void *)&r->desc[0];
> +
> + return 0;
> +}
> +
> +static int
> +iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st,
> + void *priv)
> +{
> + struct scmi_sensor_axis_info *a;
> + const struct scmi_sensor_info *s = priv;
> + struct scmi_sensor_axis_name_descriptor *adesc = st->priv;
> +
> + a = &s->axis[st->desc_index + st->loop_idx];
> + strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
> + st->priv = ++adesc;
> +
> + return 0;
> +}
> +
> +static int
> +scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
> + struct scmi_sensor_info *s)
> +{
> + void *iter;
> + struct scmi_msg_sensor_axis_description_get *msg;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_axes_desc_prepare_message,
> + .update_state = iter_axes_extended_name_update_state,
> + .process_response = iter_axes_extended_name_process_response,
> + };
> +
> + iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
> + SENSOR_AXIS_NAME_GET,
> + sizeof(*msg), s);
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> - struct scmi_sensor_info *s)
> + struct scmi_sensor_info *s,
> + u32 version)
> {
> + int ret;
> void *iter;
> struct scmi_msg_sensor_axis_description_get *msg;
> struct scmi_iterator_ops ops = {
> @@ -436,7 +499,14 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> if (IS_ERR(iter))
> return PTR_ERR(iter);
>
> - return ph->hops->iter_response_run(iter);
> + ret = ph->hops->iter_response_run(iter);
> + if (ret)
> + return ret;
> +
> + if (PROTOCOL_REV_MAJOR(version) >= 0x3)
> + ret = scmi_sensor_axis_extended_names_get(ph, s);
>From the SCMI v3.1 spec, I understood that the reading of the extended axis
name should be conditional on the bit checked by SUPPORTS_EXTENDED_AXIS_NAMES()
(the `Extended axis name' bit). Yet, the implementation doesn't use the macro,
and instead decides whether to issue SENSOR_AXIS_NAME_GET depending on the
(sensor management) protocol version being at least v3.0. But, per the spec, it
would be permissible for a v3.0 protocol to not support SENSOR_AXIS_NAME_GET at
all. Is my understanding correct?
> +
> + return ret;
> }
>
> static void iter_sens_descr_prepare_message(void *message,
> @@ -559,7 +629,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
> }
>
> if (s->num_axis > 0)
> - ret = scmi_sensor_axis_description(ph, s);
> + ret = scmi_sensor_axis_description(ph, s, si->version);
>
> st->priv = ((u8 *)sdesc + dsize);
>
Powered by blists - more mailing lists