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 26688D711BC for ; Wed, 20 Nov 2024 16:11:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:CC:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=9eQlFCNQ6D5bVENGrHZ2ywRcIPNqAYRplKluLwt9MvI=; b=k86wUIQikL3uPDUPE0w4dTC9sr bBX9FZXDEp+WW2g3H6cZLLu65PQ3J4ylknf/iFy/Squ46HU2bEVkh3W9A3keOZHxily29B8KrmXbp OltZEqWAIKjKHE9d+FArMHJNoJEc5/TnuLWTdot+d9Bkt/3xtM68ZGsMaemgSQhMHQimeCOMyc6IT Uez9jjkTrNk1yhvs6BFAlKN5mTiBUfMIKRZml+8d46mgVo21GJF7hc1MUEpPFPZEMW4j1JHpVzqbl 0oQS8LHjBxm5CbPICCHDGEU4CU3v5E7dbtvvIBWpeEVRQH1B/xTZxLKkk7hCFUQ7qH1hp0Q+ZK99/ tmReQw9w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tDnIf-0000000Fmca-2xBC; Wed, 20 Nov 2024 16:11:29 +0000 Received: from mx08-00178001.pphosted.com ([91.207.212.93] helo=mx07-00178001.pphosted.com) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tDnHi-0000000FmUO-3jjl for linux-arm-kernel@lists.infradead.org; Wed, 20 Nov 2024 16:10:32 +0000 Received: from pps.filterd (m0046661.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 4AKElasO023498; Wed, 20 Nov 2024 17:10:15 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=selector1; bh= 9eQlFCNQ6D5bVENGrHZ2ywRcIPNqAYRplKluLwt9MvI=; b=dSAp+LbUurtI1+Z5 wefQV53HDmnwQyoh4GCMTbLtPoQbAoAooxuE34RZZYGrriBNEPlYMJBPnm8oM2+T p7PAEFq8f0lCFmUAGFyXT7KCDpTpu+Ty+0z9UACqahBgC1SRO3y+m2UgZ+5SkIzz IjG1SYs55WeHK2co7HUQYX1k5Le/J0OcMRJAk5zSyfVd284GfkjGGytaaMy+YDo+ q9BB+mx6Z686At5E8UG6D/fg1eAFVcud6Mt13fnk7czNVq55dgbUckWBq+LuxmA1 qQ+NNj6IUAUeTHta0dFX9SKg5fvUt6KF31QJ3JglpKJsIO0ZAQN42xAxjdOs9CRe LydB5A== Received: from beta.dmz-ap.st.com (beta.dmz-ap.st.com [138.198.100.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 42xkqf2fvy-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 20 Nov 2024 17:10:14 +0100 (CET) Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-ap.st.com (STMicroelectronics) with ESMTP id 8228340047; Wed, 20 Nov 2024 17:08:59 +0100 (CET) Received: from Webmail-eu.st.com (eqndag1node4.st.com [10.75.129.133]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id EF9D42928BC; Wed, 20 Nov 2024 17:08:09 +0100 (CET) Received: from SAFDAG1NODE1.st.com (10.75.90.17) by EQNDAG1NODE4.st.com (10.75.129.133) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.37; Wed, 20 Nov 2024 17:08:09 +0100 Received: from [10.48.86.121] (10.48.86.121) by SAFDAG1NODE1.st.com (10.75.90.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.37; Wed, 20 Nov 2024 17:08:09 +0100 Message-ID: Date: Wed, 20 Nov 2024 17:08:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v13 4/7] remoteproc: Introduce release_fw optional operation To: Mathieu Poirier CC: Bjorn Andersson , Jens Wiklander , Rob Herring , "Krzysztof Kozlowski" , Conor Dooley , , , , , , References: <20241104133515.256497-1-arnaud.pouliquen@foss.st.com> <20241104133515.256497-5-arnaud.pouliquen@foss.st.com> <0d9075cd-68c2-49ec-9b9c-4315aa8c8517@foss.st.com> Content-Language: en-US From: Arnaud POULIQUEN Organization: STMicroelectronics In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.48.86.121] X-ClientProxiedBy: SHFCAS1NODE2.st.com (10.75.129.73) To SAFDAG1NODE1.st.com (10.75.90.17) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.60.29 definitions=2024-09-06_09,2024-09-06_01,2024-09-02_01 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241120_081031_423620_587F0048 X-CRM114-Status: GOOD ( 27.78 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 11/19/24 21:38, Mathieu Poirier wrote: > On Tue, 19 Nov 2024 at 11:14, Arnaud POULIQUEN > wrote: >> >> Hello Mathieu, >> >> On 11/18/24 18:52, Mathieu Poirier wrote: >>> On Mon, Nov 04, 2024 at 02:35:12PM +0100, Arnaud Pouliquen wrote: >>>> This patch updates the rproc_ops struct to include an optional >>>> release_fw function. >>>> >>>> The release_fw ops is responsible for releasing the remote processor >>>> firmware image. The ops is called in the following cases: >>>> >>>> - An error occurs in rproc_start() between the loading of the segments and >>>> the start of the remote processor. >>>> - after stopping the remote processor. >>>> >>>> Signed-off-by: Arnaud Pouliquen >>>> --- >>>> Updates from version V11: >>>> - fix typo in @release_fw comment >>>> --- >>>> drivers/remoteproc/remoteproc_core.c | 5 +++++ >>>> include/linux/remoteproc.h | 3 +++ >>>> 2 files changed, 8 insertions(+) >>>> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >>>> index 7694817f25d4..46863e1ca307 100644 >>>> --- a/drivers/remoteproc/remoteproc_core.c >>>> +++ b/drivers/remoteproc/remoteproc_core.c >>>> @@ -1258,6 +1258,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc) >>>> >>>> static void rproc_release_fw(struct rproc *rproc) >>>> { >>>> + if (rproc->ops->release_fw) >>>> + rproc->ops->release_fw(rproc); >>>> + >>>> /* Free the copy of the resource table */ >>>> kfree(rproc->cached_table); >>>> rproc->cached_table = NULL; >>>> @@ -1377,6 +1380,8 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) >>>> unprepare_subdevices: >>>> rproc_unprepare_subdevices(rproc); >>>> reset_table_ptr: >>>> + if (rproc->ops->release_fw) >>>> + rproc->ops->release_fw(rproc); >>>> rproc->table_ptr = rproc->cached_table; >>> >>> I suggest the following: >>> >>> 1) Create two new functions, i.e rproc_load_fw() and rproc_release_fw(). The >>> only thing those would do is call rproc->ops->load_fw() and >>> rproc->ops->release_fw(), if they are present. When a TEE interface is >>> available, ->load_fw() and ->release_fw() become rproc_tee_load_fw() and >>> rproc_tee_release_fw(). >> >> >> I'm wondering if it should be ->preload_fw() instead of ->load_fw() ops, as the >> ->load() op already exists. >> > > I agree that ->load() and ->load_fw() will lead to confusion. I would > support ->preload_fw() but there is no obvious antonyme. > > Since we already have rproc_ops::prepare() and rproc_prepare_device() > I suggest rproc_ops::prepare_fw() and rproc_prepare_fw(). The > corollary would be rproc_ops::unprepare_fw() and rproc_unprepare_fm(). > That said, I'm open to other ideas should you be interested in finding > other alternatives. > 1) Using ops::prepare_fw/unprepare_fw: My concern is that it could also lead to confusion as we would load the firmware on ops::prepare_fw and do nothing on ops::load(). That would not match with the ops action. look to me that in this option, ops::load() must be kept as mandatory ops for consistence. 2) Using ops::preload_fw: This seems to better reflect the use case. Concerning the antonym choice , could we consider that ops::release_fw() is the antonym of both ops;;preload_fw and ops::load? some other antonym proposal: - unload_fw - postunload_fw 3) Other alternatives: 3-a) using ops::rproc_prepare/unprepare_device. Same concern that prepare_fw/unprepare_fw another drawbackis that rproc_tee_load_fw() would be not directly mapped to an rproc ops but platform driver should need to call rproc_tee_load_fw() into its ops::prepare() function (a.e stm32_rproc_prepare). 3-b) Another alternative I can see is the one I proposed in version 3 [1]. The principle was to keep existing ops but propose an alternative boot sequence. Perhaps a backup solution is to reanalyze this option if no other is suitable. [1] https://lore.kernel.org/lkml/8af59b01-53cf-4fc4-9946-6c630fb7b38e@quicinc.com/T/ Please just tell/confirm me your prefered solution that I propose it in next revision. Regards, Arnaud >>> >>> 2) Call rproc_load_fw() in rproc_boot(), just before rproc_fw_boot(). If the >>> call to rproc_fw_boot() fails, call rproc_release_fw(). >>> >>> 3) The same logic applies to rproc_boot_recovery(), i.e call rproc_load_fw() >>> before rproc_start() and call rproc_release_fw() if rproc_start() fails. >> >> >> I implemented this and I'm currently testing it. >> Thise second part requires a few adjustments to work. The ->load() ops needs to >> becomes optional to not be called if the "->preload_fw()" is used. >> >> For that, I propose to return 0 in rproc_load_segments if rproc->ops->load is >> NULL and compensate by checking that at least "->preload_fw()" or ->load() is >> non-null in rproc_alloc_ops. >> > > I agree. > >> Thanks, >> Arnaud >> >> >>> >>> 4) Take rproc_tee_load_fw() out of rproc_tee_parse_fw(). It will now be called >>> in rproc_load_fw(). >>> >>> 5) As stated above function rproc_release_fw() now calls rproc_tee_release_fw(). >>> The former is already called in rproc_shutdown() so we are good in that front. >>> >>> With the above the cached_table management within the core remains the same and >>> we can get rid of patch 3.7. >> >>> >>> Thanks, >>> Mathieu >>> >>>> >>>> return ret; >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>>> index 2e0ddcb2d792..08e0187a84d9 100644 >>>> --- a/include/linux/remoteproc.h >>>> +++ b/include/linux/remoteproc.h >>>> @@ -381,6 +381,8 @@ enum rsc_handling_status { >>>> * @panic: optional callback to react to system panic, core will delay >>>> * panic at least the returned number of milliseconds >>>> * @coredump: collect firmware dump after the subsystem is shutdown >>>> + * @release_fw: optional function to release the firmware image from ROM memories. >>>> + * This function is called after stopping the remote processor or in case of an error >>>> */ >>>> struct rproc_ops { >>>> int (*prepare)(struct rproc *rproc); >>>> @@ -403,6 +405,7 @@ struct rproc_ops { >>>> u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw); >>>> unsigned long (*panic)(struct rproc *rproc); >>>> void (*coredump)(struct rproc *rproc); >>>> + void (*release_fw)(struct rproc *rproc); >>>> }; >>>> >>>> /** >>>> -- >>>> 2.25.1 >>>>