| Message ID | 20220330150551.2573938-16-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCMIv3.1 Miscellaneous changes | expand |
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. Best regards, Peter > __le32 attributes_high; > - u8 name[SCMI_MAX_STR_SIZE]; > + u8 name[SCMI_SHORT_NAME_MAX_SIZE]; > __le32 resolution; > struct scmi_msg_resp_attrs attrs; > } desc[]; > }; > > +struct scmi_msg_resp_sensor_axis_names_description { > + __le32 num_axis_flags; > + struct scmi_sensor_axis_name_descriptor { > + __le32 axis_id; > + u8 name[SCMI_MAX_STR_SIZE]; > + } desc[]; > +}; > + > /* Base scmi_axis_descriptor size excluding extended attrs after name */ > #define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ 28 > > @@ -393,7 +403,6 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph, > a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl); > > attrh = le32_to_cpu(adesc->attributes_high); > - > a->scale = S32_EXT(SENSOR_SCALE(attrh)); > a->type = SENSOR_TYPE(attrh); > strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE); The strscpy() call should probably change the size parameter to SCMI_SHORT_NAME_MAX_SIZE. > @@ -408,15 +417,69 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph, > scmi_parse_range_attrs(&a->attrs, &adesc->attrs); > dsize += sizeof(adesc->attrs); > } > - > st->priv = ((u8 *)adesc + dsize); > > return 0; > } > > +static int > +iter_axes_extended_name_update_state(struct scmi_iterator_state *st, > + const void *response, void *priv) > +{ > + u32 flags; > + const struct scmi_msg_resp_sensor_axis_names_description *r = response; > + > + flags = le32_to_cpu(r->num_axis_flags); > + st->num_returned = NUM_AXIS_RETURNED(flags); > + st->num_remaining = NUM_AXIS_REMAINING(flags); > + st->priv = (void *)&r->desc[0]; > + > + return 0; > +} > + > +static int > +iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph, > + const void *response, > + struct scmi_iterator_state *st, > + void *priv) > +{ > + struct scmi_sensor_axis_info *a; > + const struct scmi_sensor_info *s = priv; > + struct scmi_sensor_axis_name_descriptor *adesc = st->priv; > + > + a = &s->axis[st->desc_index + st->loop_idx]; > + strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE); > + st->priv = ++adesc; > + > + return 0; > +} > + > +static int > +scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph, > + struct scmi_sensor_info *s) > +{ > + void *iter; > + struct scmi_msg_sensor_axis_description_get *msg; > + 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, > + }; > + > + iter = ph->hops->iter_response_init(ph, &ops, s->num_axis, > + SENSOR_AXIS_NAME_GET, > + sizeof(*msg), s); > + if (IS_ERR(iter)) > + return PTR_ERR(iter); > + > + return ph->hops->iter_response_run(iter); > +} > + > 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? > + > + return ret; > } > > static void iter_sens_descr_prepare_message(void *message, > @@ -559,7 +629,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph, > } > > if (s->num_axis > 0) > - ret = scmi_sensor_axis_description(ph, s); > + ret = scmi_sensor_axis_description(ph, s, si->version); > > st->priv = ((u8 *)sdesc + dsize); >
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. > Best regards, > > Peter > > > __le32 attributes_high; > > - u8 name[SCMI_MAX_STR_SIZE]; > > + u8 name[SCMI_SHORT_NAME_MAX_SIZE]; > > __le32 resolution; > > struct scmi_msg_resp_attrs attrs; > > } desc[]; > > }; > > > > +struct scmi_msg_resp_sensor_axis_names_description { > > + __le32 num_axis_flags; > > + struct scmi_sensor_axis_name_descriptor { > > + __le32 axis_id; > > + u8 name[SCMI_MAX_STR_SIZE]; > > + } desc[]; > > +}; > > + > > /* Base scmi_axis_descriptor size excluding extended attrs after name */ > > #define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ 28 > > > > @@ -393,7 +403,6 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph, > > a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl); > > > > attrh = le32_to_cpu(adesc->attributes_high); > > - > > a->scale = S32_EXT(SENSOR_SCALE(attrh)); > > a->type = SENSOR_TYPE(attrh); > > strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE); > > The strscpy() call should probably change the size parameter to > SCMI_SHORT_NAME_MAX_SIZE. > I disagree on this, the strscpy size param is meant to represent the max size of the dest buffer and here the destination buffer 'a->name' is the string buffer publicly exposed by include/linux/scmi_protocol.h:scmi_sensor_axis_info whose size is SCMI_MAX_STR_SIZE. > > @@ -408,15 +417,69 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph, > > scmi_parse_range_attrs(&a->attrs, &adesc->attrs); > > dsize += sizeof(adesc->attrs); > > } > > - > > st->priv = ((u8 *)adesc + dsize); > > > > return 0; > > } > > > > +static int > > +iter_axes_extended_name_update_state(struct scmi_iterator_state *st, > > + const void *response, void *priv) > > +{ > > + u32 flags; > > + const struct scmi_msg_resp_sensor_axis_names_description *r = response; > > + > > + flags = le32_to_cpu(r->num_axis_flags); > > + st->num_returned = NUM_AXIS_RETURNED(flags); > > + st->num_remaining = NUM_AXIS_REMAINING(flags); > > + st->priv = (void *)&r->desc[0]; > > + > > + return 0; > > +} > > + > > +static int > > +iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph, > > + const void *response, > > + struct scmi_iterator_state *st, > > + void *priv) > > +{ > > + struct scmi_sensor_axis_info *a; > > + const struct scmi_sensor_info *s = priv; > > + struct scmi_sensor_axis_name_descriptor *adesc = st->priv; > > + > > + a = &s->axis[st->desc_index + st->loop_idx]; > > + strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE); > > + st->priv = ++adesc; > > + > > + return 0; > > +} > > + > > +static int > > +scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph, > > + struct scmi_sensor_info *s) > > +{ > > + void *iter; > > + struct scmi_msg_sensor_axis_description_get *msg; > > + 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, > > + }; > > + > > + iter = ph->hops->iter_response_init(ph, &ops, s->num_axis, > > + SENSOR_AXIS_NAME_GET, > > + sizeof(*msg), s); > > + if (IS_ERR(iter)) > > + return PTR_ERR(iter); > > + > > + return ph->hops->iter_response_run(iter); > > +} > > + > > 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
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
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
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)) __le32 attributes_high; - u8 name[SCMI_MAX_STR_SIZE]; + u8 name[SCMI_SHORT_NAME_MAX_SIZE]; __le32 resolution; struct scmi_msg_resp_attrs attrs; } desc[]; }; +struct scmi_msg_resp_sensor_axis_names_description { + __le32 num_axis_flags; + struct scmi_sensor_axis_name_descriptor { + __le32 axis_id; + u8 name[SCMI_MAX_STR_SIZE]; + } desc[]; +}; + /* Base scmi_axis_descriptor size excluding extended attrs after name */ #define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ 28 @@ -393,7 +403,6 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph, a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl); attrh = le32_to_cpu(adesc->attributes_high); - a->scale = S32_EXT(SENSOR_SCALE(attrh)); a->type = SENSOR_TYPE(attrh); strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE); @@ -408,15 +417,69 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph, scmi_parse_range_attrs(&a->attrs, &adesc->attrs); dsize += sizeof(adesc->attrs); } - st->priv = ((u8 *)adesc + dsize); return 0; } +static int +iter_axes_extended_name_update_state(struct scmi_iterator_state *st, + const void *response, void *priv) +{ + u32 flags; + const struct scmi_msg_resp_sensor_axis_names_description *r = response; + + flags = le32_to_cpu(r->num_axis_flags); + st->num_returned = NUM_AXIS_RETURNED(flags); + st->num_remaining = NUM_AXIS_REMAINING(flags); + st->priv = (void *)&r->desc[0]; + + return 0; +} + +static int +iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph, + const void *response, + struct scmi_iterator_state *st, + void *priv) +{ + struct scmi_sensor_axis_info *a; + const struct scmi_sensor_info *s = priv; + struct scmi_sensor_axis_name_descriptor *adesc = st->priv; + + a = &s->axis[st->desc_index + st->loop_idx]; + strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE); + st->priv = ++adesc; + + return 0; +} + +static int +scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph, + struct scmi_sensor_info *s) +{ + void *iter; + struct scmi_msg_sensor_axis_description_get *msg; + 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, + }; + + iter = ph->hops->iter_response_init(ph, &ops, s->num_axis, + SENSOR_AXIS_NAME_GET, + sizeof(*msg), s); + if (IS_ERR(iter)) + return PTR_ERR(iter); + + return ph->hops->iter_response_run(iter); +} + 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); + + return ret; } static void iter_sens_descr_prepare_message(void *message, @@ -559,7 +629,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph, } if (s->num_axis > 0) - ret = scmi_sensor_axis_description(ph, s); + ret = scmi_sensor_axis_description(ph, s, si->version); st->priv = ((u8 *)sdesc + dsize);
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(-)