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 X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 89C1FC433E0 for ; Mon, 1 Mar 2021 18:43:30 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2F13460C3D for ; Mon, 1 Mar 2021 18:43:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2F13460C3D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject: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=qHNzDmIlGU+FqCFaKMFIIOKE5ooyZnCIUuQMR1CCn8E=; b=snnQbGm5cq99gTzs2furXZdYs 77R1VmbMHKJhhGR9a2JO++zNOs6ygeN1ByuNdBcBEvWYhfOlAAcW9O/6Rn+c3evgtmq4xiy7tUBa/ IVHq+VELfRV0PceRWPrcujjkXzctdRPP3buvqfbtzuU6TBFtkcXoEEmdhk7MIgcYbNq2VKrkWSh1O RWt8WUTcRRIA3BveLHYoksSeeBeRzKIPor4GQPAoOXFlMctX0AsTbOxZkdayRSyDTh5vAj0e+WNZq v4+3pax/+E50Y5okg53/yxBhxJK2MeCJ1m0DHEaV4xwBWrmxNisuT1BXuJGdM87MFwQRyohADGROq s+j1wauOg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lGnUD-0008Gz-5O; Mon, 01 Mar 2021 18:41:41 +0000 Received: from mail-pf1-x42d.google.com ([2607:f8b0:4864:20::42d]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lGnU9-0008Gf-Av for linux-arm-kernel@lists.infradead.org; Mon, 01 Mar 2021 18:41:39 +0000 Received: by mail-pf1-x42d.google.com with SMTP id 192so5108818pfv.0 for ; Mon, 01 Mar 2021 10:41:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=dSwBx3y+KujkJBs7esgxnnYZ7mLC3mS3Y2RZJX56eNw=; b=fQ8ip8ZCEYG7Vk9vWVkDKVrnw3j8lawc9iDfzqfCl7Hdtz6WI0IpjzQa9z9rZzOQjq mq/iSkTF1OErZrupbvirtGFY5Yz2BDtcvA1wdgRru/4rLpQPYUzQ/Ngjwnz7KOLIRc8v iX+N+HsN/GR5OWdyMP5z84kUKQ6vIhbbI6s1wYY+3GVfFmqd+v82r3LyOgYOWWexYIaJ ZslTBZTjDlwmW49F369uE2WrXDXW/X41SmruVJHilkHLNUySmxNSFCbImbezIXnkMK15 9pVowhwUyFQBk7eFn/1TOpL7yW18Srt4QFXMrVWfe4Et3KSoxXSwemq5cNzf7n5xVMjK TsFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=dSwBx3y+KujkJBs7esgxnnYZ7mLC3mS3Y2RZJX56eNw=; b=PbKZmAsJd3TX7ixr5broF7BA9Dpk6nioqFm4nlpp9BYSAVlyQX1O9X/pMaLvPeRXO1 oCMRp8/kFatMYmOK65hXq4Q+LEifr3arBcNRVLRWuHDinh4C8Qc/enuDQpvQhbXmbfHa rMhGQZrm729ahm7F+tBUKZDRZ/bOhDVbWhiKStZ4wG/a7Ec5SuOYjyTP8orHabHJg8Xo WlTbl1UrQ83TR4mdIKkVC6oGLmQ1ensI5jwDAKGhVn3fF9plInMoyhss/HcXUkOd9O4F 9al3AqTEvyng1bFIX8Qb6IWCA7yaEj0ry2/WDRpuQrS/GBXyVv3nS9teuqC/oRbn85RK MiLw== X-Gm-Message-State: AOAM5304UBhTj2rZ1M/z8q/KdpGXrBDpSMNukFjOcYFCwDwa7nMG8lxK fZt+OBADrtv2ELq4JcP0Na0WKA== X-Google-Smtp-Source: ABdhPJw3d3qVrl8uKK7bLsd3bzLVlBxbfvGiR4uNPqmSB5Mhhm4TLP4r6j1B/Q0e8ckVymFrAlREGw== X-Received: by 2002:a63:7885:: with SMTP id t127mr149862pgc.237.1614624093940; Mon, 01 Mar 2021 10:41:33 -0800 (PST) Received: from xps15 (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id j35sm3152020pgj.45.2021.03.01.10.41.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 01 Mar 2021 10:41:33 -0800 (PST) Date: Mon, 1 Mar 2021 11:41:31 -0700 From: Mathieu Poirier To: Arnaud POULIQUEN Subject: Re: [PATCH v6 11/16] remoteproc: Properly deal with the resource table when attached Message-ID: <20210301184131.GB3690389@xps15> References: <20210223233515.3468677-1-mathieu.poirier@linaro.org> <20210223233515.3468677-12-mathieu.poirier@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210301_134137_674472_BF9444CD X-CRM114-Status: GOOD ( 46.48 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: ohad@wizery.com, alexandre.torgue@st.com, arnaud.pouliquen@st.com, linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org, bjorn.andersson@linaro.org, mcoquelin.stm32@gmail.com, linux-arm-kernel@lists.infradead.org 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 Fri, Feb 26, 2021 at 05:21:15PM +0100, Arnaud POULIQUEN wrote: > > > On 2/24/21 12:35 AM, Mathieu Poirier wrote: > > If it is possible to detach the remote processor, keep an untouched > > copy of the resource table. That way we can start from the same > > resource table without having to worry about original values or what > > elements the startup code has changed when re-attaching to the remote > > processor. > > > > Signed-off-by: Mathieu Poirier > > --- > > New for V6: > > - Double free of the cached table has been fixed. > > - rproc_reset_loaded_rsc_table() has seen a complete re-write. > > - rproc_stop() now calls rproc_reset_loaded_rsc_table() rather than > > dealing with the cached. This allows to properly shutdown a > > remote processor that was attached to. > > --- > > > > drivers/remoteproc/remoteproc_core.c | 86 +++++++++++++++++++++++++++- > > include/linux/remoteproc.h | 3 + > > 2 files changed, 88 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index fc01b29290a6..3a4692cc5220 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -1556,6 +1556,21 @@ static int rproc_set_loaded_rsc_table(struct rproc *rproc) > > return ret; > > } > > > > + /* > > + * If it is possible to detach the remote processor, keep an untouched > > + * copy of the resource table. That way we can start fresh again when > > + * the remote processor is re-attached, that is: > > + * > > + * DETACHED -> ATTACHED -> DETACHED -> ATTACHED > > + * > > + * Free'd in rproc_reset_loaded_rsc_table(). > > + */ > > + if (rproc->ops->detach) { > > + rproc->clean_table = kmemdup(table_ptr, table_sz, GFP_KERNEL); > > + if (!rproc->clean_table) > > + return -ENOMEM; > > + } > > + > > rproc->cached_table = NULL; > > rproc->table_ptr = table_ptr; > > rproc->table_sz = table_sz; > > @@ -1563,6 +1578,65 @@ static int rproc_set_loaded_rsc_table(struct rproc *rproc) > > return 0; > > } > > > > +static int rproc_reset_loaded_rsc_table(struct rproc *rproc) > > I spent some time to review this function that handles both the resource table > for both the stop and detach. To make it easier to read, I would divide it into > two functions. I suspected rproc_reset_loaded_rsc_table() was trying to do too many things for its own good and you just confirmed that. > I added a proposal at the end of this mail. > > Regards, > Arnaud > > > +{ > > + struct resource_table *table_ptr; > > + > > + /* > > + * The cached table is already set if the remote processor was started > > + * by the remoteproc core. > > + */ > > + if (rproc->state == RPROC_RUNNING) { > > + rproc->table_ptr = rproc->cached_table; > > + return 0; > > + } > > + > > + /* A resource table was never retrieved, nothing to do here */ > > + if (!rproc->table_ptr) > > + return 0; > > + > > + /* > > + * If we made it to this point a cached_table _must_ have been > > + * allocated in rproc_set_loaded_rsc_table(). If one isn't present > > + * something went really wrong and we must complain. > > + */ > > + if (WARN_ON(!rproc->clean_table)) > > + return -EINVAL; > > + > > + /* Remember where the external entity installed the resource table */ > > + table_ptr = rproc->table_ptr; > > + > > + /* > > + * Make a copy of the resource table currently used by the remote > > + * processor. Free'd in rproc_detach() or rproc_shutdown(). > > + */ > > + rproc->cached_table = kmemdup(rproc->table_ptr, > > + rproc->table_sz, GFP_KERNEL); > > + if (!rproc->cached_table) > > + return -ENOMEM; > > + > > + /* > > + * Use a copy of the resource table for the remainder of the > > + * shutdown process. > > + */ > > + rproc->table_ptr = rproc->cached_table; > > + > > + /* > > + * Reset the memory area where the firmware loaded the resource table > > + * to its original value. That way when we re-attach the remote > > + * processor the resource table is clean and ready to be used again. > > + */ > > + memcpy(table_ptr, rproc->clean_table, rproc->table_sz); > > + > > + /* > > + * The clean resource table is no longer needed. Allocated in > > + * rproc_set_loaded_rsc_table(). > > + */ > > + kfree(rproc->clean_table); > > + > > + return 0; > > +} > > + > > /* > > * Attach to remote processor - similar to rproc_fw_boot() but without > > * the steps that deal with the firmware image. > > @@ -1688,7 +1762,11 @@ static int rproc_stop(struct rproc *rproc, bool crashed) > > rproc_stop_subdevices(rproc, crashed); > > > > /* the installed resource table is no longer accessible */ > > - rproc->table_ptr = rproc->cached_table; > > + ret = rproc_reset_loaded_rsc_table(rproc); > > + if (ret) { > > + dev_err(dev, "can't reset resource table: %d\n", ret); > > + return ret; > > + } > > > > /* power off the remote processor */ > > ret = rproc->ops->stop(rproc); > > @@ -1721,6 +1799,9 @@ static int __rproc_detach(struct rproc *rproc) > > /* Stop any subdevices for the remote processor */ > > rproc_stop_subdevices(rproc, false); > > > > + /* the installed resource table is no longer accessible */ > > + ret = rproc_reset_loaded_rsc_table(rproc); > > + > > /* Tell the remote processor the core isn't available anymore */ > > ret = rproc->ops->detach(rproc); > > if (ret) { > > @@ -1997,6 +2078,9 @@ int rproc_detach(struct rproc *rproc) > > > > rproc_disable_iommu(rproc); > > > > + /* Free the copy of the resource table */ > > + kfree(rproc->cached_table); > > + rproc->cached_table = NULL; > > rproc->table_ptr = NULL; > > out: > > mutex_unlock(&rproc->lock); > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > > index e1c843c19cc6..e5f52a12a650 100644 > > --- a/include/linux/remoteproc.h > > +++ b/include/linux/remoteproc.h > > @@ -514,6 +514,8 @@ struct rproc_dump_segment { > > * @recovery_disabled: flag that state if recovery was disabled > > * @max_notifyid: largest allocated notify id. > > * @table_ptr: pointer to the resource table in effect > > + * @clean_table: copy of the resource table without modifications. Used > > + * when a remote processor is attached or detached from the core > > * @cached_table: copy of the resource table > > * @table_sz: size of @cached_table > > * @has_iommu: flag to indicate if remote processor is behind an MMU > > @@ -550,6 +552,7 @@ struct rproc { > > bool recovery_disabled; > > int max_notifyid; > > struct resource_table *table_ptr; > > + struct resource_table *clean_table; > > struct resource_table *cached_table; > > size_t table_sz; > > bool has_iommu; > > > > > --- > drivers/remoteproc/remoteproc_core.c | 101 ++++++++++++++++++++++++++- > include/linux/remoteproc.h | 3 + > 2 files changed, 103 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > index fc01b29290a6..cc24c360eb8b 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1556,6 +1556,21 @@ static int rproc_set_loaded_rsc_table(struct rproc *rproc) > return ret; > } > > + /* > + * If it is possible to detach the remote processor, keep an untouched > + * copy of the resource table. That way we can start fresh again when > + * the remote processor is re-attached, that is: > + * > + * DETACHED -> ATTACHED -> DETACHED -> ATTACHED > + * > + * Free'd in rproc_reset_loaded_rsc_table(). > + */ > + if (rproc->ops->detach) { > + rproc->clean_table = kmemdup(table_ptr, table_sz, GFP_KERNEL); > + if (!rproc->clean_table) > + return -ENOMEM; > + } > + > rproc->cached_table = NULL; > rproc->table_ptr = table_ptr; > rproc->table_sz = table_sz; > @@ -1563,6 +1578,79 @@ static int rproc_set_loaded_rsc_table(struct rproc *rproc) > return 0; > } > > +static int rproc_set_to_cached_rsc_table(struct rproc *rproc) > +{ > + /* A resource table was never retrieved, nothing to do here */ > + if (!rproc->table_ptr) > + return 0; > + > + /* > + * The cached table is already set if the remote processor was started > + * by the remoteproc core. > + */ > + if (!rproc->cached_table) { > + rproc->table_ptr = rproc->cached_table; > + return 0; > + } > + > + /* > + * Make a copy of the resource table currently used by the remote > + * processor. Free'd in rproc_detach() or rproc_shutdown(). > + */ > + rproc->cached_table = kmemdup(rproc->table_ptr, > + rproc->table_sz, GFP_KERNEL); > + if (!rproc->cached_table) > + return -ENOMEM; > + > + /* > + * Use a copy of the resource table for the remainder of the > + * shutdown process. > + */ > + rproc->table_ptr = rproc->cached_table; > + > + return 0; > +} > + > +static int rproc_reset_loaded_rsc_table(struct rproc *rproc) > +{ > + struct resource_table *table_ptr; > + int ret; > + > + /* A resource table was never retrieved, nothing to do here */ > + if (!rproc->table_ptr) > + return 0; > + > + /* > + * If we made it to this point a cached_table _mst_ have been > + * allocated in rproc_set_loaded_rsc_table(). If one isn't present > + * something went really wrong and we must complain. > + */ > + if (WARN_ON(!rproc->clean_table)) > + return -EINVAL; > + > + /* Remember where the external entity installed the resource table */ > + table_ptr = rproc->table_ptr; > + > + ret = rproc_set_to_cached_rsc_table(rproc); > + if (ret) > + return ret; Ok, let's split them. On the flip side I don't think calling rproc_set_to_cached_rsc_table() from rproc_reset_loaded_rsc_table() helps with clarity. I will simply spin off two distinct functions that do exactly what they are supposed to do. There will be code duplication but I'm good with that if it helps with readability and maintenance. I will rename rproc_set_loaded_rsc_table() to rproc_set_rsc_table() and spinoff two new functions: rproc_reset_rsc_table_stop() and rproc_reset_rsc_table_detach(). Hopefully that will work better. > + > + /* > + * Reset the memory area where the firmware loaded the resource table > + * to its original value. That way when we re-attach the remote > + * processor the resource table is clean and ready to be used again. > + */ > + memcpy(table_ptr, rproc->clean_table, rproc->table_sz); > + > + /* > + * The clean resource table is no longer needed. Allocated in > + * rproc_set_loaded_rsc_table(). > + */ > + kfree(rproc->clean_table); > + > + return 0; > +} > + > /* > * Attach to remote processor - similar to rproc_fw_boot() but without > * the steps that deal with the firmware image. > @@ -1688,7 +1776,11 @@ static int rproc_stop(struct rproc *rproc, bool crashed) > rproc_stop_subdevices(rproc, crashed); > > /* the installed resource table is no longer accessible */ > - rproc->table_ptr = rproc->cached_table; > + ret = rproc_set_to_cached_rsc_table(rproc); > + if (ret) { > + dev_err(dev, "can't reset resource table: %d\n", ret); > + return ret; > + } > > /* power off the remote processor */ > ret = rproc->ops->stop(rproc); > @@ -1721,6 +1813,9 @@ static int __rproc_detach(struct rproc *rproc) > /* Stop any subdevices for the remote processor */ > rproc_stop_subdevices(rproc, false); > > + /* The installed resource table need to be reset and no longer accessible */ > + ret = rproc_reset_loaded_rsc_table(rproc); > + > /* Tell the remote processor the core isn't available anymore */ > ret = rproc->ops->detach(rproc); > if (ret) { > @@ -1941,6 +2036,7 @@ void rproc_shutdown(struct rproc *rproc) > > /* Free the copy of the resource table */ > kfree(rproc->cached_table); > + kfree(rproc->clean_table); > rproc->cached_table = NULL; > rproc->table_ptr = NULL; > out: > @@ -1997,6 +2093,9 @@ int rproc_detach(struct rproc *rproc) > > rproc_disable_iommu(rproc); > > + /* Free the copy of the resource table */ > + kfree(rproc->cached_table); > + rproc->cached_table = NULL; > rproc->table_ptr = NULL; > out: > mutex_unlock(&rproc->lock); > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index e1c843c19cc6..e5f52a12a650 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -514,6 +514,8 @@ struct rproc_dump_segment { > * @recovery_disabled: flag that state if recovery was disabled > * @max_notifyid: largest allocated notify id. > * @table_ptr: pointer to the resource table in effect > + * @clean_table: copy of the resource table without modifications. Used > + * when a remote processor is attached or detached from the core > * @cached_table: copy of the resource table > * @table_sz: size of @cached_table > * @has_iommu: flag to indicate if remote processor is behind an MMU > @@ -550,6 +552,7 @@ struct rproc { > bool recovery_disabled; > int max_notifyid; > struct resource_table *table_ptr; > + struct resource_table *clean_table; > struct resource_table *cached_table; > size_t table_sz; > bool has_iommu; > -- _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel