Re: [PATCH v2 2/3] firmware: arm_scmi: Fix SENSOR_AXIS_NAME_GET behaviour when unsupported

[Peter Hilber]

On 08.06.22 18:40, Cristian Marussi wrote:
> Avoid to invoke SENSOR_AXIS_NAME_GET on sensors that have not declared at
> least one of their axes as supporting extended names.
> 
> Since the returned list of axes supporting extended names is not
> necessarily comprising all the existing axes of the specified sensor,
> take care also to properly pick the ax descriptor from the id embedded
> in the reply.
> 
> Cc: Peter Hilber <peter.hilber@xxxxxxxxxxxxxxx>
> Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> Fixes: 802b0bed011e ("firmware: arm_scmi: Add SCMI v3.1 SENSOR_AXIS_NAME_GET support")
> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>

Reviewed-by: Peter Hilber <peter.hilber@xxxxxxxxxxxxxxx>

> ---
> V1 --> v2
> - fixed missing endianity conversion
> ---
>  drivers/firmware/arm_scmi/sensors.c | 56 +++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 75b9d716508e..8a93dd944c49 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -358,15 +358,20 @@ static int scmi_sensor_update_intervals(const struct scmi_protocol_handle *ph,
>  	return ph->hops->iter_response_run(iter);
>  }
>  
> +struct scmi_apriv {
> +	bool any_axes_support_extended_names;
> +	struct scmi_sensor_info *s;
> +};
> +
>  static void iter_axes_desc_prepare_message(void *message,
>  					   const unsigned int desc_index,
>  					   const void *priv)
>  {
>  	struct scmi_msg_sensor_axis_description_get *msg = message;
> -	const struct scmi_sensor_info *s = priv;
> +	const struct scmi_apriv *apriv = priv;
>  
>  	/* Set the number of sensors to be skipped/already read */
> -	msg->id = cpu_to_le32(s->id);
> +	msg->id = cpu_to_le32(apriv->s->id);
>  	msg->axis_desc_index = cpu_to_le32(desc_index);
>  }
>  
> @@ -393,12 +398,14 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
>  	u32 attrh, attrl;
>  	struct scmi_sensor_axis_info *a;
>  	size_t dsize = SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ;
> -	struct scmi_sensor_info *s = priv;
> +	struct scmi_apriv *apriv = priv;
>  	const struct scmi_axis_descriptor *adesc = st->priv;
>  
>  	attrl = le32_to_cpu(adesc->attributes_low);
> +	if (SUPPORTS_EXTENDED_AXIS_NAMES(attrl))
> +		apriv->any_axes_support_extended_names = true;
>  
> -	a = &s->axis[st->desc_index + st->loop_idx];
> +	a = &apriv->s->axis[st->desc_index + st->loop_idx];
>  	a->id = le32_to_cpu(adesc->id);
>  	a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
>  
> @@ -444,10 +451,19 @@ iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph,
>  					 void *priv)
>  {
>  	struct scmi_sensor_axis_info *a;
> -	const struct scmi_sensor_info *s = priv;
> +	const struct scmi_apriv *apriv = priv;
>  	struct scmi_sensor_axis_name_descriptor *adesc = st->priv;
> +	u32 axis_id = le32_to_cpu(adesc->axis_id);
> +
> +	if (axis_id >= st->max_resources)
> +		return -EPROTO;
>  
> -	a = &s->axis[st->desc_index + st->loop_idx];
> +	/*
> +	 * Pick the corresponding descriptor based on the axis_id embedded
> +	 * in the reply since the list of axes supporting extended names
> +	 * can be a subset of all the axes.
> +	 */
> +	a = &apriv->s->axis[axis_id];
>  	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
>  	st->priv = ++adesc;
>  
> @@ -458,21 +474,36 @@ static int
>  scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
>  				    struct scmi_sensor_info *s)
>  {
> +	int ret;
>  	void *iter;
>  	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,
>  	};
> +	struct scmi_apriv apriv = {
> +		.any_axes_support_extended_names = false,
> +		.s = s,
> +	};
>  
>  	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
>  					    SENSOR_AXIS_NAME_GET,
>  					    sizeof(struct scmi_msg_sensor_axis_description_get),
> -					    s);
> +					    &apriv);
>  	if (IS_ERR(iter))
>  		return PTR_ERR(iter);
>  
> -	return ph->hops->iter_response_run(iter);
> +	/*
> +	 * Do not cause whole protocol initialization failure when failing to
> +	 * get extended names for axes.
> +	 */
> +	ret = ph->hops->iter_response_run(iter);
> +	if (ret)
> +		dev_warn(ph->dev,
> +			 "Failed to get axes extended names for %s (ret:%d).\n",
> +			 s->name, ret);
> +
> +	return 0;
>  }
>  
>  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> @@ -486,6 +517,10 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
>  		.update_state = iter_axes_desc_update_state,
>  		.process_response = iter_axes_desc_process_response,
>  	};
> +	struct scmi_apriv apriv = {
> +		.any_axes_support_extended_names = false,
> +		.s = s,
> +	};
>  
>  	s->axis = devm_kcalloc(ph->dev, s->num_axis,
>  			       sizeof(*s->axis), GFP_KERNEL);
> @@ -495,7 +530,7 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
>  	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
>  					    SENSOR_AXIS_DESCRIPTION_GET,
>  					    sizeof(struct scmi_msg_sensor_axis_description_get),
> -					    s);
> +					    &apriv);
>  	if (IS_ERR(iter))
>  		return PTR_ERR(iter);
>  
> @@ -503,7 +538,8 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
>  	if (ret)
>  		return ret;
>  
> -	if (PROTOCOL_REV_MAJOR(version) >= 0x3)
> +	if (PROTOCOL_REV_MAJOR(version) >= 0x3 &&
> +	    apriv.any_axes_support_extended_names)
>  		ret = scmi_sensor_axis_extended_names_get(ph, s);
>  
>  	return ret;