Messages in this thread | | | Date | Wed, 8 Jun 2022 09:49:07 +0100 | From | Cristian Marussi <> | Subject | Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support |
| |
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
| |