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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7910BC433F5 for ; Thu, 28 Apr 2022 12:08:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346013AbiD1MLQ (ORCPT ); Thu, 28 Apr 2022 08:11:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46658 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229650AbiD1MLO (ORCPT ); Thu, 28 Apr 2022 08:11:14 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id D9091972BC for ; Thu, 28 Apr 2022 05:07:59 -0700 (PDT) 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 922051474; Thu, 28 Apr 2022 05:07:59 -0700 (PDT) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 52A5C3F73B; Thu, 28 Apr 2022 05:07:58 -0700 (PDT) Date: Thu, 28 Apr 2022 13:07:56 +0100 From: Cristian Marussi To: Sudeep Holla Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 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 02/22] firmware: arm_scmi: Make protocols init fail on basic errors Message-ID: References: <20220330150551.2573938-1-cristian.marussi@arm.com> <20220330150551.2573938-3-cristian.marussi@arm.com> <20220426153528.bhskpchq2huhjtjk@bogus> <20220428102524.s37xg2ytjkcgkq3e@bogus> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220428102524.s37xg2ytjkcgkq3e@bogus> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 28, 2022 at 11:25:24AM +0100, Sudeep Holla wrote: > On Tue, Apr 26, 2022 at 05:25:28PM +0100, Cristian Marussi wrote: > > On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote: > > > On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote: > > > > Bail out of protocol initialization routine early when basic information > > > > about protocol version and attributes could not be retrieved: failing to > > > > act this way can lead to a successfully initialized SCMI protocol which > > > > is in fact not fully functional. > > > > > > > > Signed-off-by: Cristian Marussi > > > > --- > > > > drivers/firmware/arm_scmi/base.c | 5 ++++- > > > > drivers/firmware/arm_scmi/clock.c | 8 ++++++-- > > > > drivers/firmware/arm_scmi/perf.c | 10 +++++++--- > > > > drivers/firmware/arm_scmi/power.c | 10 +++++++--- > > > > drivers/firmware/arm_scmi/reset.c | 10 +++++++--- > > > > drivers/firmware/arm_scmi/sensors.c | 4 +++- > > > > drivers/firmware/arm_scmi/system.c | 5 ++++- > > > > 7 files changed, 38 insertions(+), 14 deletions(-) > > > > > > > > Hi Sudeep, > > > > thanks for the review first of all... > > > > > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) > > > > if (!cinfo) > > > > return -ENOMEM; > > > > > > > > - scmi_clock_protocol_attributes_get(ph, cinfo); > > > > + ret = scmi_clock_protocol_attributes_get(ph, cinfo); > > > > + if (ret) > > > > + return ret; > > > > > > Does this result in removal of scmi_dev associated with devm_* calls ? > > > Otherwise we may need to free the allocated buffers ? I am not sure > > > if the dev here is individual scmi_dev or the platform scmi device. > > > I assume latter and it is unlikely to be removed/freed with the error in > > > the above path. > > > > > > Similarly in couple of other instances/protocols. > > > > So, ph->dev used in the above devm_ is indeed the arm_scmi platform device > > and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own > > code (O_o), I saw/remembered that when a protocol instance is initialized on > > it first usage, there is indeed a devres_group internally managed by > > the SCMI core, so that: > > > > scmi_get_protocol_instance() > > > > @first_protocol_usage (refcount pi->users): > > > > --> scmi_get_protocol() // just in case was LKM proto > > --> scmi_alloc_init_protocol_instance() > > gid = devres_open_group(handle->dev, NULL, GFP_KERNEL); > > > > ret = pi->proto->instance_init(&pi->ph); > > ====>>> i.e. scmi_clock_protocol_init(ph) > > if (ret) > > goto clean; > > ..... > > > > clean: > > devres_release_group(handle->dev, gid); > > > > > > So basically all that happens at initialization time in scmi_clock_protocol_init, > > BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance > > during that protocol initialization (like the events registration) is undone on > > failure transparently by the SCMI core init/free management functions > > (via devres_ groups...) > > > > All of the above is because each protocol is initialized only once on > > its first usage, no matter how many SCMI driver users (and scmi_devs) are > > using it...only in case (unsupported) we have multiple SCMI instances > > (platforms) there will be one instance of protocol initialized per SCMI > > server. > > > > ... having said that, now I'll go and double check (test) this behaviour since I > > even had forgot about having implemented this kind of design :P > > > > Makes sense, thanks for the detailed explanation. I had totally forgotten how > devres_group works 🙁, my bad. > Well I had even forgot to have used it in the SCMI core :P Thanks, Cristian