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

Peter Hilber
2022-06-08T08:54:58Z

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@arm.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.
>>
>
> 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