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 v4 00/17] remoteproc: Add support for detaching a rproc
To:     Mathieu Poirier <mathieu.poirier@linaro.org>,
        Arnaud POULIQUEN <arnaud.pouliquen@st.com>
CC:     "ohad@wizery.com" <ohad@wizery.com>,
        "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "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: <20201218173228.2277032-1-mathieu.poirier@linaro.org>
 <64b559dc-9e89-c351-ddee-f9cebd155ed7@st.com>
 <20210202004956.GD1319650@xps15>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Message-ID: <200a464a-f6dd-480c-d7cd-d8165828fabc@foss.st.com>
Date:   Tue, 2 Feb 2021 09:54:13 +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: <20210202004956.GD1319650@xps15>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/2/21 1:49 AM, Mathieu Poirier wrote:
> On Wed, Jan 27, 2021 at 10:21:24AM +0100, Arnaud POULIQUEN wrote:
>> Hi Mathieu
>>
>> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
>>> Following the work done here [1], this set provides support for the
>>> remoteproc core to release resources associated with a remote processor
>>> without having to switch it off. That way a platform driver can be removed
>>> or the application processor power cycled while the remote processor is
>>> still operating.
>>>
>>> Of special interest in this series are patches 5 and 6 where getting the
>>> address of the resource table installed by an eternal entity if moved to
>>> the core.  This is to support scenarios where a remote process has been
>>> booted by the core but is being detached.  To re-attach the remote
>>> processor, the address of the resource table needs to be known at a later
>>> time than the platform driver's probe() function.
>>>
>>> Applies cleanly on v5.10
>>>
>>> Thanks,
>>> Mathieu
>>>
>>> [1]. https://lkml.org/lkml/2020/7/14/1600
>>>
>>> ----
>>> New for v4:
>>> - Made binding description OS agnostic (Rob)
>>> - Added functionality to set the external resource table in the core
>>> - Fixed a crash when detaching (Arnaud)
>>> - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
>>> - Added RB tags
>>
>>
>> I tested you series, attach and  detach is working well.
>>
>> Then I faced issue when tried to re-attach after a detach.
>>
> 
> Right, in this case don't expect the re-attach to work properly because function
> stm32_rproc_detach() does not exist.  As such the M4 doesn't put itself back
> in "wait-for-attach" mode as it does when booted by the boot loader.  If I
> remember correctly we talked about that during an earlier conversation and we
> agreed FW support would be needed to properly test the re-attach.

Yes you are right the remote firmware needs to be inform about the detach, and
this is the purpose of the detach ops.
But also some actions are missing on local side as some resources have also to
be reinitialized as described in my previous mail.
For instance the resource table is handled by the remoteproc framework. The
remote firmware should only have a read access to this table.

>  
>> But I don't know if this feature has to be supported in this step.
>>
>> The 2 issues I found are:
>>
>> 1) memory carveouts are released on detach so need to be reinitialized.
>> The use of prepare/unprepare for the attach and detach would solve the issue but
>> probably need to add parameter to differentiate a start/stop from a attach/detach.
>>
>> 2) The vrings in the loaded resource table (so no cached) has to be properly
>> reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
>> considered as a fixed address.
>>
>> Here is a fix which works on the stm32 platform
>>
>> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
>>  	 */
>>  	if (rproc->table_ptr) {
>>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
>> -		rsc->vring[idx].da = 0;
>> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
>>  		rsc->vring[idx].notifyid = -1;
>>  	}
>>  }
> 
> In light of the above let me know if these two issues are still relevant.  If
> so I'll investigate further.

To highlight the issue just test attach/detach/attch  with a firmware that
implements a RPMsg communication. On the second attach the virtio framework is
not properly restarted.

Then please find at the end of the mail 3 patches for test I added on top of
your series,that allow me to reattach. Of course the RPMsg channels are not
re-created as i don't implement the remote FW part, but the Linux virtio and
RPmsg frameworks are restarted.

- [PATCH 1/3] remoteproc: stm32: add capability to detach from the remoteproc
  => Add a dummy function in stm32_rproc for test.
- [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
  => Add prepare/unprepare on attach/detach + implement attach in stm32mp1 to
     reinitialize the memory region that as been cleaned on detach.
- [PATCH 3/3] remoteproc: virtio: set to vring address to FW_RSC_ADDR_ANY on free
  => Reinitialize the vring addresses on detach. For this one a better
     implementation would be to use a cached resource table to fully
     reinitialize it on re-attach.

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>>
>> Here, perhaps a better alternative would be to make a cached copy on attach
>> before updating it. On the next attach, the cached copy would be copied as it is
>> done in rproc_start.
>>
>> Thanks,
>> Arnaud
>>
>>
>>>
>>> Mathieu Poirier (17):
>>>   dt-bindings: remoteproc: Add bindind to support autonomous processors
>>>   remoteproc: Re-check state in rproc_shutdown()
>>>   remoteproc: Remove useless check in rproc_del()
>>>   remoteproc: Rename function rproc_actuate()
>>>   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
>>>   remoteproc: stm32: Move resource table setup to rproc_ops
>>>   remoteproc: Add new RPROC_ATTACHED state
>>>   remoteproc: Properly represent the attached state
>>>   remoteproc: Properly deal with a kernel panic when attached
>>>   remoteproc: Add new detach() remoteproc operation
>>>   remoteproc: Introduce function __rproc_detach()
>>>   remoteproc: Introduce function rproc_detach()
>>>   remoteproc: Add return value to function rproc_shutdown()
>>>   remoteproc: Properly deal with a stop request when attached
>>>   remoteproc: Properly deal with a start request when attached
>>>   remoteproc: Properly deal with detach request
>>>   remoteproc: Refactor rproc delete and cdev release path
>>>
>>>  .../bindings/remoteproc/remoteproc-core.yaml  |  27 +++
>>>  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
>>>  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
>>>  drivers/remoteproc/remoteproc_internal.h      |   8 +
>>>  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
>>>  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
>>>  include/linux/remoteproc.h                    |  24 +-
>>>  7 files changed, 344 insertions(+), 125 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
>>>

Subject: [PATCH 1/3] remoteproc: stm32: add capability to detach from the
 remoteproc

Add a dummy function to allow to detach. No specific action is needed

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

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 2c949725b91e..b325d28f627c 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -590,6 +590,12 @@ static int stm32_rproc_attach(struct rproc *rproc)
 	return reset_control_assert(ddata->hold_boot);
 }

+static int stm32_rproc_detach(struct rproc *rproc)
+{
+	/* Nothing to do but ops mandatory to support the detach feature */
+	return 0;
+}
+
 static int stm32_rproc_stop(struct rproc *rproc)
 {
 	struct stm32_rproc *ddata = rproc->priv;
@@ -712,6 +718,7 @@ static struct rproc_ops st_rproc_ops = {
 	.start		= stm32_rproc_start,
 	.stop		= stm32_rproc_stop,
 	.attach		= stm32_rproc_attach,
+	.detach		= stm32_rproc_detach,
 	.kick		= stm32_rproc_kick,
 	.load		= rproc_elf_load_segments,
 	.parse_fw	= stm32_rproc_parse_fw,
-- 
2.17.1


------------------------------------------------------------------------


Subject: [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach

Some actions such as memory resources reallocation are needed when try
to reattach. Use the prepare ops for these actions.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++++
 drivers/remoteproc/stm32_rproc.c     | 14 +++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index f1f51ad1a1d6..f177561b8863 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1557,6 +1557,13 @@ static int rproc_attach(struct rproc *rproc)
 		return ret;
 	}

+	/* Prepare rproc for firmware loading if needed */
+	ret = rproc_prepare_device(rproc);
+	if (ret) {
+		dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
+		goto disable_iommu;
+	}
+
 	ret = rproc_get_loaded_rsc_table(rproc);
 	if (ret) {
 		dev_err(dev, "can't load resource table: %d\n", ret);
@@ -1990,6 +1997,13 @@ int rproc_detach(struct rproc *rproc)
 	/* clean up all acquired resources */
 	rproc_resource_cleanup(rproc);

+	/* Release HW resources if needed */
+	ret = rproc_unprepare_device(rproc);
+	if (ret) {
+		atomic_inc(&rproc->power);
+		goto out;
+	}
+
 	rproc_disable_iommu(rproc);

 	/*
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index b325d28f627c..bf50d79b1f09 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -413,9 +413,6 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const
struct firmware *fw)
 	struct stm32_rproc *ddata = rproc->priv;
 	int ret;

-	ret  = stm32_rproc_parse_memory_regions(rproc);
-	if (ret)
-		return ret;

 	if (ddata->trproc)
 		ret = rproc_tee_get_rsc_table(ddata->trproc);
@@ -580,6 +577,12 @@ static int stm32_rproc_start(struct rproc *rproc)

 	return reset_control_assert(ddata->hold_boot);
 }
+static int stm32_rproc_prepare(struct rproc *rproc)
+{
+	dev_err(&rproc->dev, "%s: %d\n", __func__, __LINE__);
+
+	return stm32_rproc_parse_memory_regions(rproc);
+}

 static int stm32_rproc_attach(struct rproc *rproc)
 {
@@ -717,6 +720,7 @@ static int stm32_rproc_get_loaded_rsc_table(struct rproc *rproc)
 static struct rproc_ops st_rproc_ops = {
 	.start		= stm32_rproc_start,
 	.stop		= stm32_rproc_stop,
+	.prepare	= stm32_rproc_prepare,
 	.attach		= stm32_rproc_attach,
 	.detach		= stm32_rproc_detach,
 	.kick		= stm32_rproc_kick,
@@ -921,10 +925,6 @@ static int stm32_rproc_probe(struct platform_device *pdev)

 	if (state == M4_STATE_CRUN) {
 		rproc->state = RPROC_DETACHED;
-
-		ret = stm32_rproc_parse_memory_regions(rproc);
-		if (ret)
-			goto free_resources;
 	}

 	rproc->has_iommu = false;
-- 
2.17.1


------------------------------------------------------------------------

Subject: [PATCH 3/3] remoteproc: virtio: set to vring address to
 FW_RSC_ADDR_ANY on free

The resource table vring structure is cleaned on free. But value is set
to 0. This value is considered as a valid address. Set the value
to  FW_RSC_ADDR_ANY instead.
This is needed to allow to reattach to an autonomous firmware.
An alternative would be to save the resource table before updating it.
On free the value would be reset to initial value.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index f177561b8863..5b5de4db3981 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
 	 */
 	if (rproc->table_ptr) {
 		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
-		rsc->vring[idx].da = 0;
+		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
 		rsc->vring[idx].notifyid = -1;
 	}
 }
-- 
2.17.1