Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453404 > unrolled thread

[PATCH 1/4] remoteproc: Introduce always-on flag

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] remoteproc: Introduce always-on flag Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-01 21:00 +0200
    [PATCH 4/4] remoteproc: Move handling of cached table to boot/shutdown Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-01 21:00 +0200
    Re: [PATCH 1/4] remoteproc: Introduce always-on flag loic pallardy <loic.pallardy@st.com> - 2016-08-02 17:30 +0200
      Re: [PATCH 1/4] remoteproc: Introduce always-on flag Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-04 00:10 +0200
        Re: [PATCH 1/4] remoteproc: Introduce always-on flag loic pallardy <loic.pallardy@st.com> - 2016-08-04 11:50 +0200
          Re: [PATCH 1/4] remoteproc: Introduce always-on flag Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-04 19:30 +0200

#1453404 — [PATCH 1/4] remoteproc: Introduce always-on flag

Introduce an "always-on" flag on rprocs to make it possible to flag
remote processors without vdevs to automatically boot once the firmware
is found.

Cc: Lee Jones <lee.jones@linaro.org>
Cc: Loic Pallardy <loic.pallardy@st.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c   | 27 ++++++++++++++++++++++++++-
 drivers/remoteproc/remoteproc_virtio.c | 13 -------------
 include/linux/remoteproc.h             |  1 +
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index fe0539ed9cb5..7e7f24fcac69 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -933,6 +933,10 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	/* look for virtio devices and register them */
 	ret = rproc_handle_resources(rproc, tablesz, rproc_vdev_handler);
 
+	/* if rproc is marked always-on, request it to boot */
+	if (rproc->always_on)
+		rproc_boot_nowait(rproc);
+
 out:
 	release_firmware(fw);
 	/* allow rproc_del() contexts, if any, to proceed */
@@ -978,11 +982,16 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
 int rproc_trigger_recovery(struct rproc *rproc)
 {
 	struct rproc_vdev *rvdev, *rvtmp;
+	int ret;
 
 	dev_err(&rproc->dev, "recovering %s\n", rproc->name);
 
 	init_completion(&rproc->crash_comp);
 
+	/* shut down the remote */
+	/* TODO: make sure this works with rproc->power > 1 */
+	rproc_shutdown(rproc);
+
 	/* clean up remote vdev entries */
 	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
 		rproc_remove_virtio_dev(rvdev);
@@ -993,7 +1002,17 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	/* Free the copy of the resource table */
 	kfree(rproc->cached_table);
 
-	return rproc_add_virtio_devices(rproc);
+	ret = rproc_add_virtio_devices(rproc);
+	if (ret)
+		return ret;
+
+	/*
+	 * boot the remote processor up again, waiting for the async fw load to
+	 * finish
+	 */
+	rproc_boot(rproc);
+
+	return 0;
 }
 
 /**
@@ -1374,6 +1393,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	rproc->name = name;
 	rproc->ops = ops;
 	rproc->priv = &rproc[1];
+	rproc->always_on = true;
 
 	device_initialize(&rproc->dev);
 	rproc->dev.parent = dev;
@@ -1452,6 +1472,11 @@ int rproc_del(struct rproc *rproc)
 	/* if rproc is just being registered, wait */
 	wait_for_completion(&rproc->firmware_loading_complete);
 
+	/* if rproc is marked always-on, rproc_add() booted it */
+	/* TODO: make sure this works with rproc->power > 1 */
+	if (rproc->always_on)
+		rproc_shutdown(rproc);
+
 	/* clean up remote vdev entries */
 	list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
 		rproc_remove_virtio_dev(rvdev);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index cc91556313e1..574c90ce07f7 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -136,11 +136,6 @@ static void __rproc_virtio_del_vqs(struct virtio_device *vdev)
 
 static void rproc_virtio_del_vqs(struct virtio_device *vdev)
 {
-	struct rproc *rproc = vdev_to_rproc(vdev);
-
-	/* power down the remote processor before deleting vqs */
-	rproc_shutdown(rproc);
-
 	__rproc_virtio_del_vqs(vdev);
 }
 
@@ -149,7 +144,6 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		       vq_callback_t *callbacks[],
 		       const char * const names[])
 {
-	struct rproc *rproc = vdev_to_rproc(vdev);
 	int i, ret;
 
 	for (i = 0; i < nvqs; ++i) {
@@ -160,13 +154,6 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		}
 	}
 
-	/* now that the vqs are all set, boot the remote processor */
-	ret = rproc_boot_nowait(rproc);
-	if (ret) {
-		dev_err(&rproc->dev, "rproc_boot() failed %d\n", ret);
-		goto error;
-	}
-
 	return 0;
 
 error:
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 1c457a8dd5a6..bd1cfcbb57b9 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -443,6 +443,7 @@ struct rproc {
 	struct resource_table *cached_table;
 	u32 table_csum;
 	bool has_iommu;
+	bool always_on;
 };
 
 /* we currently support only two vrings per rvdev */
-- 
2.5.0

[toc] | [next] | [standalone]

#1453405 — [PATCH 4/4] remoteproc: Move handling of cached table to boot/shutdown

As we moved the vdev handling to the main boot/shutdown code path we can
further simplify the resource table handling by moving the parsing spet
to boot as well. The lifespan of the resource table is changed to live
from rproc_boot() to rproc_shutdown().

Cc: Lee Jones <lee.jones@linaro.org>
Cc: Loic Pallardy <loic.pallardy@st.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++------------------------
 include/linux/remoteproc.h           |  2 --
 2 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9d64409f3839..6281b86a34cd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -802,9 +802,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	struct resource_table *table, *loaded_table;
 	int ret, tablesz;
 
-	if (!rproc->table_ptr)
-		return -ENOMEM;
-
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
 		return ret;
@@ -831,11 +828,17 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
-	/* Verify that resource table in loaded fw is unchanged */
-	if (rproc->table_csum != crc32(0, table, tablesz)) {
-		dev_err(dev, "resource checksum failed, fw changed?\n");
+	/*
+	 * Create a copy of the resource table. When a virtio device starts
+	 * and calls vring_new_virtqueue() the address of the allocated vring
+	 * will be stored in the cached_table. Before the device is started,
+	 * cached_table will be copied into devic memory.
+	 */
+	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
+	if (!rproc->cached_table)
 		goto clean_up;
-	}
+
+	rproc->table_ptr = rproc->cached_table;
 
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
@@ -893,6 +896,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 
 clean_up:
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
+	rproc->table_ptr = NULL;
+
 	rproc_resource_cleanup(rproc);
 	rproc_disable_iommu(rproc);
 	return ret;
@@ -909,36 +916,11 @@ clean_up:
 static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 {
 	struct rproc *rproc = context;
-	struct resource_table *table;
-	int tablesz;
-
-	if (rproc_fw_sanity_check(rproc, fw) < 0)
-		goto out;
-
-	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw,  &tablesz);
-	if (!table)
-		goto out;
-
-	rproc->table_csum = crc32(0, table, tablesz);
-
-	/*
-	 * Create a copy of the resource table. When a virtio device starts
-	 * and calls vring_new_virtqueue() the address of the allocated vring
-	 * will be stored in the cached_table. Before the device is started,
-	 * cached_table will be copied into devic memory.
-	 */
-	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
-	if (!rproc->cached_table)
-		goto out;
-
-	rproc->table_ptr = rproc->cached_table;
 
 	/* if rproc is marked always-on, request it to boot */
 	if (rproc->always_on)
 		rproc_boot_nowait(rproc);
 
-out:
 	release_firmware(fw);
 	/* allow rproc_del() contexts, if any, to proceed */
 	complete_all(&rproc->firmware_loading_complete);
@@ -1178,8 +1160,10 @@ void rproc_shutdown(struct rproc *rproc)
 
 	rproc_disable_iommu(rproc);
 
-	/* Give the next start a clean resource table */
-	rproc->table_ptr = rproc->cached_table;
+	/* Free the copy of the resource table */
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
+	rproc->table_ptr = NULL;
 
 	/* if in crash state, unlock crash handler */
 	if (rproc->state == RPROC_CRASHED)
@@ -1467,9 +1451,6 @@ int rproc_del(struct rproc *rproc)
 	list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
 		rproc_remove_virtio_dev(rvdev);
 
-	/* Free the copy of the resource table */
-	kfree(rproc->cached_table);
-
 	/* the rproc is downref'ed as soon as it's removed from the klist */
 	mutex_lock(&rproc_list_mutex);
 	list_del(&rproc->node);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index bd1cfcbb57b9..441f0c387023 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -409,7 +409,6 @@ enum rproc_crash_type {
  * @max_notifyid: largest allocated notify id.
  * @table_ptr: pointer to the resource table in effect
  * @cached_table: copy of the resource table
- * @table_csum: checksum of the resource table
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  */
 struct rproc {
@@ -441,7 +440,6 @@ struct rproc {
 	int max_notifyid;
 	struct resource_table *table_ptr;
 	struct resource_table *cached_table;
-	u32 table_csum;
 	bool has_iommu;
 	bool always_on;
 };
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1454747

Hi Bjorn,

On 08/01/2016 08:58 PM, Bjorn Andersson wrote:
> Introduce an "always-on" flag on rprocs to make it possible to flag
> remote processors without vdevs to automatically boot once the firmware
> is found.
>
Should this flag rather be named "auto-boot"? From my pov, "always-on" 
means coprocessor can't be shutdown.

> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Loic Pallardy <loic.pallardy@st.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c   | 27 ++++++++++++++++++++++++++-
>   drivers/remoteproc/remoteproc_virtio.c | 13 -------------
>   include/linux/remoteproc.h             |  1 +
>   3 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index fe0539ed9cb5..7e7f24fcac69 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -933,6 +933,10 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>   	/* look for virtio devices and register them */
>   	ret = rproc_handle_resources(rproc, tablesz, rproc_vdev_handler);
>
> +	/* if rproc is marked always-on, request it to boot */
> +	if (rproc->always_on)
> +		rproc_boot_nowait(rproc);
> +
>   out:
>   	release_firmware(fw);
>   	/* allow rproc_del() contexts, if any, to proceed */
> @@ -978,11 +982,16 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
>   int rproc_trigger_recovery(struct rproc *rproc)
>   {
>   	struct rproc_vdev *rvdev, *rvtmp;
> +	int ret;
>
>   	dev_err(&rproc->dev, "recovering %s\n", rproc->name);
>
>   	init_completion(&rproc->crash_comp);
>
> +	/* shut down the remote */
> +	/* TODO: make sure this works with rproc->power > 1 */
> +	rproc_shutdown(rproc);
> +
>   	/* clean up remote vdev entries */
>   	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
>   		rproc_remove_virtio_dev(rvdev);
> @@ -993,7 +1002,17 @@ int rproc_trigger_recovery(struct rproc *rproc)
>   	/* Free the copy of the resource table */
>   	kfree(rproc->cached_table);
>
> -	return rproc_add_virtio_devices(rproc);
> +	ret = rproc_add_virtio_devices(rproc);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * boot the remote processor up again, waiting for the async fw load to
> +	 * finish
> +	 */
> +	rproc_boot(rproc);
You are changing current behavior by forcing rproc boot whatever 
"always-on". Moreover coprocessor already rebooted by 
rproc_add_virtio_device if "always-on" flag is set, doesn't it?
If yes, rproc->power will be equal to 2 and rproc_shutdown call will 
failed as this second rproc_boot call is unknown from customer pov.

Regards,
Loic

> +
> +	return 0;
>   }
>
>   /**
> @@ -1374,6 +1393,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   	rproc->name = name;
>   	rproc->ops = ops;
>   	rproc->priv = &rproc[1];
> +	rproc->always_on = true;
>
>   	device_initialize(&rproc->dev);
>   	rproc->dev.parent = dev;
> @@ -1452,6 +1472,11 @@ int rproc_del(struct rproc *rproc)
>   	/* if rproc is just being registered, wait */
>   	wait_for_completion(&rproc->firmware_loading_complete);
>
> +	/* if rproc is marked always-on, rproc_add() booted it */
> +	/* TODO: make sure this works with rproc->power > 1 */
> +	if (rproc->always_on)
> +		rproc_shutdown(rproc);
> +
>   	/* clean up remote vdev entries */
>   	list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
>   		rproc_remove_virtio_dev(rvdev);
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index cc91556313e1..574c90ce07f7 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -136,11 +136,6 @@ static void __rproc_virtio_del_vqs(struct virtio_device *vdev)
>
>   static void rproc_virtio_del_vqs(struct virtio_device *vdev)
>   {
> -	struct rproc *rproc = vdev_to_rproc(vdev);
> -
> -	/* power down the remote processor before deleting vqs */
> -	rproc_shutdown(rproc);
> -
>   	__rproc_virtio_del_vqs(vdev);
>   }
>
> @@ -149,7 +144,6 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>   		       vq_callback_t *callbacks[],
>   		       const char * const names[])
>   {
> -	struct rproc *rproc = vdev_to_rproc(vdev);
>   	int i, ret;
>
>   	for (i = 0; i < nvqs; ++i) {
> @@ -160,13 +154,6 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>   		}
>   	}
>
> -	/* now that the vqs are all set, boot the remote processor */
> -	ret = rproc_boot_nowait(rproc);
> -	if (ret) {
> -		dev_err(&rproc->dev, "rproc_boot() failed %d\n", ret);
> -		goto error;
> -	}
> -
>   	return 0;
>
>   error:
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 1c457a8dd5a6..bd1cfcbb57b9 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -443,6 +443,7 @@ struct rproc {
>   	struct resource_table *cached_table;
>   	u32 table_csum;
>   	bool has_iommu;
> +	bool always_on;
>   };
>
>   /* we currently support only two vrings per rvdev */
>

[toc] | [prev] | [next] | [standalone]

#1456039

On Tue 02 Aug 08:17 PDT 2016, loic pallardy wrote:

> Hi Bjorn,
> 
> On 08/01/2016 08:58 PM, Bjorn Andersson wrote:
> >Introduce an "always-on" flag on rprocs to make it possible to flag
> >remote processors without vdevs to automatically boot once the firmware
> >is found.
> >
> Should this flag rather be named "auto-boot"? From my pov, "always-on" means
> coprocessor can't be shutdown.
> 

I saw it from the view of the remoteproc driver, in which case it's
always-on. But I'm fine with naming it "auto-boot" instead.

[..]
> >diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
[..]
> >@@ -978,11 +982,16 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
> >  int rproc_trigger_recovery(struct rproc *rproc)
> >  {
> >  	struct rproc_vdev *rvdev, *rvtmp;
> >+	int ret;
> >
> >  	dev_err(&rproc->dev, "recovering %s\n", rproc->name);
> >
> >  	init_completion(&rproc->crash_comp);
> >
> >+	/* shut down the remote */
> >+	/* TODO: make sure this works with rproc->power > 1 */
> >+	rproc_shutdown(rproc);
> >+
> >  	/* clean up remote vdev entries */
> >  	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
> >  		rproc_remove_virtio_dev(rvdev);
> >@@ -993,7 +1002,17 @@ int rproc_trigger_recovery(struct rproc *rproc)
> >  	/* Free the copy of the resource table */
> >  	kfree(rproc->cached_table);
> >
> >-	return rproc_add_virtio_devices(rproc);
> >+	ret = rproc_add_virtio_devices(rproc);
> >+	if (ret)
> >+		return ret;
> >+
> >+	/*
> >+	 * boot the remote processor up again, waiting for the async fw load to
> >+	 * finish
> >+	 */
> >+	rproc_boot(rproc);
> You are changing current behavior by forcing rproc boot whatever
> "always-on". Moreover coprocessor already rebooted by
> rproc_add_virtio_device if "always-on" flag is set, doesn't it?
> If yes, rproc->power will be equal to 2 and rproc_shutdown call will failed
> as this second rproc_boot call is unknown from customer pov.
> 

rproc_add_virtio_devices() does no longer call rproc_boot(), this patch
moves that call. So for always-on rprocs "power" will go 1 -> 0 -> 1 in
this function.

What does change is that for a non-always-on case.

If we have 1 client that has requested rproc_boot() then the current
implementation will bring "power" down to 1 and we will wait until the
client for some reason calls rproc_shutdown(). After that we might boot
the system again, if there are any vdevs in the resource table.

Here we will bring "power" from 1 -> 0 -> 1, without regarding who's
holding references.

> >+
> >+	return 0;
> >  }

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1456279

On 08/04/2016 12:02 AM, Bjorn Andersson wrote:
> On Tue 02 Aug 08:17 PDT 2016, loic pallardy wrote:
>
>> Hi Bjorn,
>>
>> On 08/01/2016 08:58 PM, Bjorn Andersson wrote:
>>> Introduce an "always-on" flag on rprocs to make it possible to flag
>>> remote processors without vdevs to automatically boot once the firmware
>>> is found.
>>>
>> Should this flag rather be named "auto-boot"? From my pov, "always-on" means
>> coprocessor can't be shutdown.
>>
>
> I saw it from the view of the remoteproc driver, in which case it's
> always-on. But I'm fine with naming it "auto-boot" instead.
>
> [..]
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>>> @@ -978,11 +982,16 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
>>>   int rproc_trigger_recovery(struct rproc *rproc)
>>>   {
>>>   	struct rproc_vdev *rvdev, *rvtmp;
>>> +	int ret;
>>>
>>>   	dev_err(&rproc->dev, "recovering %s\n", rproc->name);
>>>
>>>   	init_completion(&rproc->crash_comp);
>>>
>>> +	/* shut down the remote */
>>> +	/* TODO: make sure this works with rproc->power > 1 */
>>> +	rproc_shutdown(rproc);
>>> +
>>>   	/* clean up remote vdev entries */
>>>   	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
>>>   		rproc_remove_virtio_dev(rvdev);
>>> @@ -993,7 +1002,17 @@ int rproc_trigger_recovery(struct rproc *rproc)
>>>   	/* Free the copy of the resource table */
>>>   	kfree(rproc->cached_table);
>>>
>>> -	return rproc_add_virtio_devices(rproc);
>>> +	ret = rproc_add_virtio_devices(rproc);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/*
>>> +	 * boot the remote processor up again, waiting for the async fw load to
>>> +	 * finish
>>> +	 */
>>> +	rproc_boot(rproc);
>> You are changing current behavior by forcing rproc boot whatever
>> "always-on". Moreover coprocessor already rebooted by
>> rproc_add_virtio_device if "always-on" flag is set, doesn't it?
>> If yes, rproc->power will be equal to 2 and rproc_shutdown call will failed
>> as this second rproc_boot call is unknown from customer pov.
>>
>
> rproc_add_virtio_devices() does no longer call rproc_boot(), this patch
> moves that call. So for always-on rprocs "power" will go 1 -> 0 -> 1 in
> this function.
>
> What does change is that for a non-always-on case.
>
> If we have 1 client that has requested rproc_boot() then the current
> implementation will bring "power" down to 1 and we will wait until the
> client for some reason calls rproc_shutdown(). After that we might boot
> the system again, if there are any vdevs in the resource table.
>
> Here we will bring "power" from 1 -> 0 -> 1, without regarding who's
> holding references.

I'm fine with the final sequence in which only rproc_shutdown and 
rproc_boot are called (with patch 3 modifications).

But having a look only to this patch, we have the following function call:

rproc_trigger_recovery
   |__ rproc_shutdown --> power from 1 -> 0
   |__ rproc_add_virtio_devices
	|__ rproc_fw_config_virtio
		|__ (if always_on == 1) rproc_boot_nowait --> power from 0 --> 1
   |__ rproc_boot
	if always_on == 1 power from 1 --> 2
	else power from 0 --> 1

on this patch rproc_boot should be called only is always_on flag is not set.

With patch 3, call to rproc_add_virtio_devices is suppressed and 
behavior is ok.

Regards,
Loic
>
>>> +
>>> +	return 0;
>>>   }
>
> Regards,
> Bjorn
>

[toc] | [prev] | [next] | [standalone]

#1456598

On Thu 04 Aug 02:44 PDT 2016, loic pallardy wrote:

> >>>diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
[..]
> >>>@@ -993,7 +1002,17 @@ int rproc_trigger_recovery(struct rproc *rproc)
> >>>  	/* Free the copy of the resource table */
> >>>  	kfree(rproc->cached_table);
> >>>
> >>>-	return rproc_add_virtio_devices(rproc);
> >>>+	ret = rproc_add_virtio_devices(rproc);
> >>>+	if (ret)
> >>>+		return ret;
> >>>+
> >>>+	/*
> >>>+	 * boot the remote processor up again, waiting for the async fw load to
> >>>+	 * finish
> >>>+	 */
> >>>+	rproc_boot(rproc);
> >>You are changing current behavior by forcing rproc boot whatever
> >>"always-on". Moreover coprocessor already rebooted by
> >>rproc_add_virtio_device if "always-on" flag is set, doesn't it?
> >>If yes, rproc->power will be equal to 2 and rproc_shutdown call will failed
> >>as this second rproc_boot call is unknown from customer pov.
> >>
> >
> >rproc_add_virtio_devices() does no longer call rproc_boot(), this patch
> >moves that call. So for always-on rprocs "power" will go 1 -> 0 -> 1 in
> >this function.
> >
> >What does change is that for a non-always-on case.
> >
> >If we have 1 client that has requested rproc_boot() then the current
> >implementation will bring "power" down to 1 and we will wait until the
> >client for some reason calls rproc_shutdown(). After that we might boot
> >the system again, if there are any vdevs in the resource table.
> >
> >Here we will bring "power" from 1 -> 0 -> 1, without regarding who's
> >holding references.
> 
> I'm fine with the final sequence in which only rproc_shutdown and rproc_boot
> are called (with patch 3 modifications).
> 
> But having a look only to this patch, we have the following function call:
> 
> rproc_trigger_recovery
>   |__ rproc_shutdown --> power from 1 -> 0
>   |__ rproc_add_virtio_devices
> 	|__ rproc_fw_config_virtio
> 		|__ (if always_on == 1) rproc_boot_nowait --> power from 0 --> 1
>   |__ rproc_boot
> 	if always_on == 1 power from 1 --> 2
> 	else power from 0 --> 1
> 
> on this patch rproc_boot should be called only is always_on flag is not set.
> 
> With patch 3, call to rproc_add_virtio_devices is suppressed and behavior is
> ok.
> 

Doh! Thanks for the explanation, you're of course correct.

Regards,
Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web