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

Cristian Marussi
2022-06-08T09:04:24Z

On Wed, Jun 08, 2022 at 10:40:30AM +0200, Peter Hilber wrote:
> 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,
> >>

Hi Peter,

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

...after a quick chat with Sudeep I'll also post a fix to avoid issuing a
SENSOR_AXIS_GET_NAME if NONE of the axes has been advertised as supporting extended
names

Thanks,
Cristian