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 A7BB0C433F5 for ; Tue, 26 Apr 2022 16:26:42 +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=Yhm9xEzSxnp2td4Q5aSVBZGAKQOI74etY05/zgERWYI=; b=NN7QZ41Oby6+XH hwQqV0KrN6Yfateu6okSb+YkBP08wwpkjTFSdxDyoFJsJ7Ood0m5fqZof0gAh714ZA23yULAtC26m fmCkdgVzNJZSueq1PM/nlYclnbtwNtGqhct/8S6M/JbbfX4ZFAhjJWX1CgRNLRndffMNjULLJDJsy Ll4ZY8XxuyebUYrdnbTOyabPQyE/WKqjf8HrMSv+alvpdkL9bkZZMQ47sP6D1w5K7RwVFNRSQSKbn 9EH3+xvI+/bV/sJt7bcPS0J5oRxkrRuu/VUbMdOforgF8QOWf2TIOPEa20edjlefZTC0T1yWw3Z2x Pvv0GVxn70RYxXilp2bg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1njO0V-00FL5b-7X; Tue, 26 Apr 2022 16:25:43 +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 1njO0R-00FL4t-Fq for linux-arm-kernel@lists.infradead.org; Tue, 26 Apr 2022 16:25:41 +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 1E12B23A; Tue, 26 Apr 2022 09:25:36 -0700 (PDT) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D78D63F73B; Tue, 26 Apr 2022 09:25:34 -0700 (PDT) Date: Tue, 26 Apr 2022 17:25:28 +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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220426153528.bhskpchq2huhjtjk@bogus> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220426_092539_663886_19CB0C5D X-CRM114-Status: GOOD ( 22.65 ) 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 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 Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel