Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support

[Peter Hilber]

On 06.06.22 10:18, Cristian Marussi wrote:
> On Thu, Jun 02, 2022 at 04:25:45PM +0200, Peter Hilber wrote:
>> 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@xxxxxxx>
>>> ---
>>>  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.
>>
> 
> Hi Peter,
> 
> thanks for having a look, your feedback is always appreciated.
> 
> Plese see my answers inline.
> 
[snip]
>>>  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?
>>
> 
> Yes, indeed this behaviour was deliberate so as to keep this code
> simpler while addressing some tricky definitions in the spec.
> (not so short explanation follows :P)
> 
> SENSOR_AXIS_DESCRIPTION_GET is a command that, issued against a specific
> sensor, return a list of axes descriptors for that sensor and such
> descriptors in turn also include the flag you're mentioning that states
> if a specific ax does support an extended name or not that will have to
> be fetched with SENSOR_AXIS_GET_NAME.
> 
> BUT the SENSOR_AXIS_GET_NAME command is a multi-part command issued
> against a specific sensor to retrieve the list of all the axes extended
> names for that sensor, NOT to retrieve a single ax extended name, so I
> cannot really check each ax extended name support before issuing the
> commmand and, even though weird, the axes could have different support
> with some of them supporting the extended name while some other don't:
> as a consequence my take about this was that the platform would reply
> anyway but only with the list of axes having an extended name (possibly
> a subset of all the axes).
> 
> What could be missing in this context it's the handling of the case in
> which all axes does NOT support extended names where probably the platform
> won't even answer my request. (unsupported even if PROTO > 3.0)
> 
> Moreover even tracking this per-ax support while iterating the replies
> would have made more complex some of the logic with anyway at the same
> time hitting all the limitations explained above.
> 
> In this context, it seemed to me simpler (and a good trade-off) to issue
> anyway the command while checking only for the protocol version and
> accepting thatSENSOR_AXIS_GET_NAME could fail because unsupported
> by all the axes, with the result of leaving the ax->name string content
> filled with the short name previously retrieved.
> 
> Assuming that my blabbing above is acceptable, what IS indeed wrong
> (reviewig this patch) is that the any 'acceptable' failure as depicted
> above is not properly ignored in fact. I'll post a fix on top like:
> 
> --->8----
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 50502c530b2f..788b566f634b 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -472,7 +472,9 @@ scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
>         if (IS_ERR(iter))
>                 return PTR_ERR(iter);
>  
> -       return ph->hops->iter_response_run(iter);
> +       ph->hops->iter_response_run(iter);
> +
> +       return 0;
>  }
>  
>  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> ----
> 
> Moreover even the parsing logic for the SENSOR_AXIS_GET_NAME command has to
> be sligthly reviewed to address the fact that the list of returned axes
> extended names is incomplete so the returned axes won't necessarily be
> returned in order (i.e. I'll have to check 'axis_d' in the SENSOR_AXIS_NAME_GET
> replies to look up the proper ax descriptor.).
> I'll post this as a distinct fix.
> 
> Does all of this make sense/seems reasonable ?
> 
> Thanks for the review again,
> Cristian
> 

Hi Cristian,

thanks for your quick reply, this does all make sense to me.

Best regards,

Peter