lkml.org 
[lkml]   [2022]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support
    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

    \
     
     \ /
      Last update: 2022-06-08 11:23    [W:4.107 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site