Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 09/15] remoteproc: Introduce function rproc_detach()
To:     Mathieu Poirier <mathieu.poirier@linaro.org>,
        "ohad@wizery.com" <ohad@wizery.com>,
        "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>
CC:     "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20201126210642.897302-1-mathieu.poirier@linaro.org>
 <20201126210642.897302-10-mathieu.poirier@linaro.org>
From:   Arnaud POULIQUEN <arnaud.pouliquen@st.com>
Message-ID: <0e705760-b69a-d872-9770-c03dde85ab1c@st.com>
Date:   Tue, 8 Dec 2020 19:35:18 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201126210642.897302-10-mathieu.poirier@linaro.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Mathieu,


On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> Introduce function rproc_detach() to enable the remoteproc
> core to release the resources associated with a remote processor
> without stopping its operation.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
>  include/linux/remoteproc.h           |  1 +
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 928b3f975798..f5adf05762e9 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  /*
>   * __rproc_detach(): Does the opposite of rproc_attach()
>   */
> -static int __maybe_unused __rproc_detach(struct rproc *rproc)
> +static int __rproc_detach(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
>  	int ret;
> @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_shutdown);
>  
> +/**
> + * rproc_detach() - Detach the remote processor from the
> + * remoteproc core
> + *
> + * @rproc: the remote processor
> + *
> + * Detach a remote processor (previously attached to with rproc_actuate()).
> + *
> + * In case @rproc is still being used by an additional user(s), then
> + * this function will just decrement the power refcount and exit,
> + * without disconnecting the device.
> + *
> + * Function rproc_detach() calls __rproc_detach() in order to let a remote
> + * processor know that services provided by the application processor are
> + * no longer available.  From there it should be possible to remove the
> + * platform driver and even power cycle the application processor (if the HW
> + * supports it) without needing to switch off the remote processor.
> + */
> +int rproc_detach(struct rproc *rproc)
> +{
> +	struct device *dev = &rproc->dev;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&rproc->lock);
> +	if (ret) {
> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> +		return ret;
> +	}
> +
> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
> +	/* if the remote proc is still needed, bail out */
> +	if (!atomic_dec_and_test(&rproc->power)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = __rproc_detach(rproc);
> +	if (ret) {
> +		atomic_inc(&rproc->power);
> +		goto out;
> +	}
> +
> +	/* clean up all acquired resources */
> +	rproc_resource_cleanup(rproc);

I started to test the series, I found 2 problems testing in STM32P1 board.

1) the resource_table pointer is unmapped if the firmware has been booted by the
Linux, generating a crash in rproc_free_vring.
I attached a fix at the end of the mail.

2) After the detach, the rproc state is "detached"
but it is no longer possible to re-attach to it correctly.
Neither if the firmware is standalone, nor if it has been booted
by the Linux.

I did not investigate, but the issue is probably linked to the resource
table address which is set to NULL.

So we either have to fix the problem in order to attach or forbid the transition.


Regards,
Arnaud

> +
> +	rproc_disable_iommu(rproc);
> +
> +	/*
> +	 * Set the remote processor's table pointer to NULL.  Since mapping
> +	 * of the resource table to a virtual address is done in the platform
> +	 * driver, unmapping should also be done there.
> +	 */
> +	rproc->table_ptr = NULL;
> +out:
> +	mutex_unlock(&rproc->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(rproc_detach);
> +
>  /**
>   * rproc_get_by_phandle() - find a remote processor by phandle
>   * @phandle: phandle to the rproc
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index da15b77583d3..329c1c071dcf 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
>  
>  int rproc_boot(struct rproc *rproc);
>  void rproc_shutdown(struct rproc *rproc);
> +int rproc_detach(struct rproc *rproc);
>  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> 

From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
Date: Tue, 8 Dec 2020 18:54:51 +0100
Subject: [PATCH] remoteproc: core: fix detach for unmapped table_ptr

If the firmware has been loaded and started by the kernel, the
resource table has probably been mapped by the carveout allocation
(see rproc_elf_find_loaded_rsc_table).
In this case the memory can have been unmapped before the vrings are free.
The result is a crash that occurs in rproc_free_vring while try to use the
unmapped pointer.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
---
 drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index 2b0a52fb3398..3508ffba4a2a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1964,6 +1964,13 @@ int rproc_detach(struct rproc *rproc)
 		goto out;
 	}

+	/*
+	 * Prevent case that the installed resource table is no longer
+	 * accessible (e.g. memory unmapped), use the cache if available
+	 */
+	if (rproc->cached_table)
+		rproc->table_ptr = rproc->cached_table;
+
 	ret = __rproc_detach(rproc);
 	if (ret) {
 		atomic_inc(&rproc->power);
@@ -1975,10 +1982,14 @@ int rproc_detach(struct rproc *rproc)

 	rproc_disable_iommu(rproc);

+	/* Free the chached table memory that can has been allocated*/
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
 	/*
-	 * Set the remote processor's table pointer to NULL.  Since mapping
-	 * of the resource table to a virtual address is done in the platform
-	 * driver, unmapping should also be done there.
+	 * Set the remote processor's table pointer to NULL. If mapping
+	 * of the resource table to a virtual address has been done in the
+	 * platform driver(attachment to an existing firmware),
+	 * unmapping should also be done there.
 	 */
 	rproc->table_ptr = NULL;
 out:
-- 
2.17.1