From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C5B10C433EF for ; Wed, 8 Jun 2022 09:04:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=G/ZZLOBYRJhFdrXWYIUht+T4uGbmV5qNRtMOL4SNcXg=; b=Ecx1Q2nA0Fym3I rg0izkM/khxyUhWas/no4SjLwG/Hf/0neOYGZEOY/jxZPBNIax6ErSNsTk7d5v1+FYzj6YhGilNuD kVDXAxLieXEIJ70/O0OYensxrjhTh7p56F1UW3lwtJuqofUp1zlfC45ACAXW5qg+Ef7hJvanXmuiz 79R4/0i3ZzH4eIP+bYcSdaULbF25HR9pWW77h9f5qgFWUBRUm5s+L8Yw42UXhDbBSwG3sy2ihmeRU U7hYGEkNWIRrMaN7i6K/eUyznym1uOyDLge1tMRprsV6aXv6yw82FR1yg32hZdquLxnmtBOHkga2Y ibVVzhPU4gmRWdMXR0qg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nyran-00CCDz-6i; Wed, 08 Jun 2022 09:03:09 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nyrNM-00C5fY-H7 for linux-arm-kernel@lists.infradead.org; Wed, 08 Jun 2022 08:49:21 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 341E51424; Wed, 8 Jun 2022 01:49:11 -0700 (PDT) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C53B83F66F; Wed, 8 Jun 2022 01:49:09 -0700 (PDT) Date: Wed, 8 Jun 2022 09:49:07 +0100 From: Cristian Marussi To: Peter Hilber Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com, james.quinlan@broadcom.com, Jonathan.Cameron@huawei.com, f.fainelli@gmail.com, etienne.carriere@linaro.org, vincent.guittot@linaro.org, souvik.chakravarty@arm.com Subject: Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support Message-ID: References: <20220330150551.2573938-1-cristian.marussi@arm.com> <20220330150551.2573938-16-cristian.marussi@arm.com> <3c4aa97e-f121-61d3-c1d9-1e5e1f7c0e6c@opensynergy.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <3c4aa97e-f121-61d3-c1d9-1e5e1f7c0e6c@opensynergy.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220608_014916_723845_1597B8CD X-CRM114-Status: GOOD ( 52.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > >>> --- > >>> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel