[PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Sarangdhar Joshi]

The function wkup_m3_rproc_boot_thread waits for asynchronous
firmware loading to complete successfully before calling
rproc_boot(). The same can be achieved by just setting
rproc->auto_boot flag. Change this. As a result this change
removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
initialization to the wkup_m3_ipc_probe().

Other than the current usage, the firmware_loading_complete is
only used in rproc_del() where it's no longer needed.  This
change is in preparation for removing firmware_loading_complete
completely.

CC: Dave Gerlach <d-gerlach@ti.com>
CC: Suman Anna <s-anna@ti.com>
CC: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
---

Hi Suman,

Unfortunately, I don't have a TI device and couldn't test this
change. Is it possible for you to test this change on TI device?

Thanks in advance.

Regards,
Sarang

 drivers/remoteproc/wkup_m3_rproc.c |  2 +-
 drivers/soc/ti/wkup_m3_ipc.c       | 35 ++---------------------------------
 2 files changed, 3 insertions(+), 34 deletions(-)

diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
index 18175d0..79ea022 100644
--- a/drivers/remoteproc/wkup_m3_rproc.c
+++ b/drivers/remoteproc/wkup_m3_rproc.c
@@ -167,7 +167,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	rproc->auto_boot = false;
+	rproc->auto_boot = true;
 
 	wkupm3 = rproc->priv;
 	wkupm3->rproc = rproc;
diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
index 8823cc8..31090d70 100644
--- a/drivers/soc/ti/wkup_m3_ipc.c
+++ b/drivers/soc/ti/wkup_m3_ipc.c
@@ -17,7 +17,6 @@
 
 #include <linux/err.h>
 #include <linux/kernel.h>
-#include <linux/kthread.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/module.h>
@@ -365,22 +364,6 @@ void wkup_m3_ipc_put(struct wkup_m3_ipc *m3_ipc)
 }
 EXPORT_SYMBOL_GPL(wkup_m3_ipc_put);
 
-static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc)
-{
-	struct device *dev = m3_ipc->dev;
-	int ret;
-
-	wait_for_completion(&m3_ipc->rproc->firmware_loading_complete);
-
-	init_completion(&m3_ipc->sync_complete);
-
-	ret = rproc_boot(m3_ipc->rproc);
-	if (ret)
-		dev_err(dev, "rproc_boot failed\n");
-
-	do_exit(0);
-}
-
 static int wkup_m3_ipc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -388,7 +371,6 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
 	phandle rproc_phandle;
 	struct rproc *m3_rproc;
 	struct resource *res;
-	struct task_struct *task;
 	struct wkup_m3_ipc *m3_ipc;
 
 	m3_ipc = devm_kzalloc(dev, sizeof(*m3_ipc), GFP_KERNEL);
@@ -402,6 +384,8 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
 		return PTR_ERR(m3_ipc->ipc_mem_base);
 	}
 
+	init_completion(&m3_ipc->sync_complete);
+
 	irq = platform_get_irq(pdev, 0);
 	if (!irq) {
 		dev_err(&pdev->dev, "no irq resource\n");
@@ -449,25 +433,10 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
 
 	m3_ipc->ops = &ipc_ops;
 
-	/*
-	 * Wait for firmware loading completion in a thread so we
-	 * can boot the wkup_m3 as soon as it's ready without holding
-	 * up kernel boot
-	 */
-	task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_ipc,
-			   "wkup_m3_rproc_loader");
-
-	if (IS_ERR(task)) {
-		dev_err(dev, "can't create rproc_boot thread\n");
-		goto err_put_rproc;
-	}
-
 	m3_ipc_state = m3_ipc;
 
 	return 0;
 
-err_put_rproc:
-	rproc_put(m3_rproc);
 err_free_mbox:
 	mbox_free_channel(m3_ipc->mbox);
 	return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] remoteproc: Remove firmware_loading_complete

[Sarangdhar Joshi]

rproc_del() waits on firmware_loading_complete in order to
make sure rproc_add() completed successfully before calling
rproc_shutdown().  However since rproc_add() will always be
called before rproc_del(), we do not need to wait on
firmware_loading_complete. Drop this completion variable
altogether.

Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
---

Sending this patch again since I had missed usage of
firmware_loading_complete in drivers/soc/ti/wkup_m3_ipc.c

 drivers/remoteproc/remoteproc_core.c | 12 +-----------
 include/linux/remoteproc.h           |  2 --
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 953ee29..862fa4e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -975,17 +975,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		rproc_boot(rproc);
 
 	release_firmware(fw);
-	/* allow rproc_del() contexts, if any, to proceed */
-	complete_all(&rproc->firmware_loading_complete);
 }
 
 static int rproc_add_virtio_devices(struct rproc *rproc)
 {
 	int ret;
 
-	/* rproc_del() calls must wait until async loader completes */
-	init_completion(&rproc->firmware_loading_complete);
-
 	/*
 	 * We must retrieve early virtio configuration info from
 	 * the firmware (e.g. whether to register a virtio device,
@@ -997,10 +992,8 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
 	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG,
 				      rproc->firmware, &rproc->dev, GFP_KERNEL,
 				      rproc, rproc_fw_config_virtio);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(&rproc->dev, "request_firmware_nowait err: %d\n", ret);
-		complete_all(&rproc->firmware_loading_complete);
-	}
 
 	return ret;
 }
@@ -1483,9 +1476,6 @@ int rproc_del(struct rproc *rproc)
 	if (!rproc)
 		return -EINVAL;
 
-	/* 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->auto_boot)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e2f3a32..19d84a0 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -397,7 +397,6 @@ enum rproc_crash_type {
  * @num_traces: number of trace buffers
  * @carveouts: list of physically contiguous memory allocations
  * @mappings: list of iommu mappings we initiated, needed on shutdown
- * @firmware_loading_complete: marks e/o asynchronous firmware loading
  * @bootaddr: address of first instruction to boot rproc with (optional)
  * @rvdevs: list of remote virtio devices
  * @subdevs: list of subdevices, to following the running state
@@ -428,7 +427,6 @@ struct rproc {
 	int num_traces;
 	struct list_head carveouts;
 	struct list_head mappings;
-	struct completion firmware_loading_complete;
 	u32 bootaddr;
 	struct list_head rvdevs;
 	struct list_head subdevs;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] remoteproc: Remove firmware_loading_complete

[loic pallardy]

On 12/16/2016 01:03 AM, Sarangdhar Joshi wrote:
> rproc_del() waits on firmware_loading_complete in order to
> make sure rproc_add() completed successfully before calling
> rproc_shutdown().  However since rproc_add() will always be
> called before rproc_del(), we do not need to wait on
> firmware_loading_complete. Drop this completion variable
> altogether.
>
Hi,

firmware_loading_complete is used to synchronize all operations on rproc 
with parallel work launched by request_firmware_nowait.
rproc_add could be done and firmware loading still pending. In that case 
rproc_del mustn't be called before end of the procedure.

If you decide to remove this synchronization you need either to modify 
rproc boot sequence or to replace it by something else.

Regards,
Loic

> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
> ---
>
> Sending this patch again since I had missed usage of
> firmware_loading_complete in drivers/soc/ti/wkup_m3_ipc.c
>
>  drivers/remoteproc/remoteproc_core.c | 12 +-----------
>  include/linux/remoteproc.h           |  2 --
>  2 files changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 953ee29..862fa4e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -975,17 +975,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>  		rproc_boot(rproc);
>
>  	release_firmware(fw);
> -	/* allow rproc_del() contexts, if any, to proceed */
> -	complete_all(&rproc->firmware_loading_complete);
>  }
>
>  static int rproc_add_virtio_devices(struct rproc *rproc)
>  {
>  	int ret;
>
> -	/* rproc_del() calls must wait until async loader completes */
> -	init_completion(&rproc->firmware_loading_complete);
> -
>  	/*
>  	 * We must retrieve early virtio configuration info from
>  	 * the firmware (e.g. whether to register a virtio device,
> @@ -997,10 +992,8 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
>  	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG,
>  				      rproc->firmware, &rproc->dev, GFP_KERNEL,
>  				      rproc, rproc_fw_config_virtio);
> -	if (ret < 0) {
> +	if (ret < 0)
>  		dev_err(&rproc->dev, "request_firmware_nowait err: %d\n", ret);
> -		complete_all(&rproc->firmware_loading_complete);
> -	}
>
>  	return ret;
>  }
> @@ -1483,9 +1476,6 @@ int rproc_del(struct rproc *rproc)
>  	if (!rproc)
>  		return -EINVAL;
>
> -	/* 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->auto_boot)
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e2f3a32..19d84a0 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -397,7 +397,6 @@ enum rproc_crash_type {
>   * @num_traces: number of trace buffers
>   * @carveouts: list of physically contiguous memory allocations
>   * @mappings: list of iommu mappings we initiated, needed on shutdown
> - * @firmware_loading_complete: marks e/o asynchronous firmware loading
>   * @bootaddr: address of first instruction to boot rproc with (optional)
>   * @rvdevs: list of remote virtio devices
>   * @subdevs: list of subdevices, to following the running state
> @@ -428,7 +427,6 @@ struct rproc {
>  	int num_traces;
>  	struct list_head carveouts;
>  	struct list_head mappings;
> -	struct completion firmware_loading_complete;
>  	u32 bootaddr;
>  	struct list_head rvdevs;
>  	struct list_head subdevs;
>

Re: [PATCH 2/2] remoteproc: Remove firmware_loading_complete

[Bjorn Andersson]

On Fri 16 Dec 00:26 PST 2016, loic pallardy wrote:

> 
> 
> On 12/16/2016 01:03 AM, Sarangdhar Joshi wrote:
> >rproc_del() waits on firmware_loading_complete in order to
> >make sure rproc_add() completed successfully before calling
> >rproc_shutdown().  However since rproc_add() will always be
> >called before rproc_del(), we do not need to wait on
> >firmware_loading_complete. Drop this completion variable
> >altogether.
> >
> Hi,
> 
> firmware_loading_complete is used to synchronize all operations on rproc
> with parallel work launched by request_firmware_nowait.

We had a deadlock scenario in this code, where a call to rproc_boot()
would grab the rproc mutex and the request_firmware_nowait() callback
would wait on this lock before it would signal the completion that the
rproc_boot() was waiting for.

As the request_firmware_nowait() doesn't do anything other than handle
auto_boot and signal the completion - and there is an internal sleep
mechanism for handling concurrent request_firmware calls - I posted a
patch and dropped the rproc_boot() wait thing.

> rproc_add could be done and firmware loading still pending. In that case
> rproc_del mustn't be called before end of the procedure.

You're right.

We might have an outstanding request_firmware_nowait() when we hit
rproc_del() and we might free the underlaying rproc context.

Holding a reference over the request_firmware_nowait() would solve this,
but would cause issues if we get a rproc_add() from the same driver
(e.g. after module unload/load) before the firmware timer has fired -
and released the resources.

This issue could be remedied by moving the rproc_delete_debug_dir() to
rproc_del() and aim for not having any objects exposed outside the
remoteproc core once rproc_del() returns.

> 
> If you decide to remove this synchronization you need either to modify rproc
> boot sequence or to replace it by something else.
> 

I agree.

Regards,
Bjorn

Re: [PATCH 2/2] remoteproc: Remove firmware_loading_complete

[Sarangdhar Joshi]

On 12/16/2016 11:28 AM, Bjorn Andersson wrote:
> On Fri 16 Dec 00:26 PST 2016, loic pallardy wrote:
>
>>
>>
>> On 12/16/2016 01:03 AM, Sarangdhar Joshi wrote:
>>> rproc_del() waits on firmware_loading_complete in order to
>>> make sure rproc_add() completed successfully before calling
>>> rproc_shutdown().  However since rproc_add() will always be
>>> called before rproc_del(), we do not need to wait on
>>> firmware_loading_complete. Drop this completion variable
>>> altogether.
>>>
>> Hi,
>>
>> firmware_loading_complete is used to synchronize all operations on rproc
>> with parallel work launched by request_firmware_nowait.
>
> We had a deadlock scenario in this code, where a call to rproc_boot()
> would grab the rproc mutex and the request_firmware_nowait() callback
> would wait on this lock before it would signal the completion that the
> rproc_boot() was waiting for.
>
> As the request_firmware_nowait() doesn't do anything other than handle
> auto_boot and signal the completion - and there is an internal sleep
> mechanism for handling concurrent request_firmware calls - I posted a
> patch and dropped the rproc_boot() wait thing.

That's right. Should have added reference to commit
"e9b4f9efff5021 ("remoteproc: Drop wait in __rproc_boot()")"

>
>> rproc_add could be done and firmware loading still pending. In that case
>> rproc_del mustn't be called before end of the procedure.
>
> You're right.
>
> We might have an outstanding request_firmware_nowait() when we hit
> rproc_del() and we might free the underlaying rproc context.
>
> Holding a reference over the request_firmware_nowait() would solve this,
> but would cause issues if we get a rproc_add() from the same driver
> (e.g. after module unload/load) before the firmware timer has fired -
> and released the resources.

The asynchronous work request_firmware_work_func() is protected by 
get_device()/put_device() on remoteproc device. So we are probably 
covered for remoteproc device. However, I agree that parent device will 
still be an issue.

>
> This issue could be remedied by moving the rproc_delete_debug_dir() to
> rproc_del() and aim for not having any objects exposed outside the
> remoteproc core once rproc_del() returns.
>
>>
>> If you decide to remove this synchronization you need either to modify rproc
>> boot sequence or to replace it by something else.
>>
>
> I agree.

I agree too. rproc_boot() calls for non auto_boot case anyway calls 
request_firmware(). So calling __request_firmware asynchronously for non 
auto_boot case seems redundant. I was planning to send a patch to  call 
rproc_add_virtio_devices() for auto_boot case only. I guess I'll need to 
take care of only auto_boot case for the current issue then.

Regards,
Sarang

>
> Regards,
> Bjorn
>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Suman Anna]

Hi Sarang,

On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
> The function wkup_m3_rproc_boot_thread waits for asynchronous
> firmware loading to complete successfully before calling
> rproc_boot(). The same can be achieved by just setting
> rproc->auto_boot flag. Change this. As a result this change
> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
> initialization to the wkup_m3_ipc_probe().
> 
> Other than the current usage, the firmware_loading_complete is
> only used in rproc_del() where it's no longer needed.  This
> change is in preparation for removing firmware_loading_complete
> completely.

Based on the comments so far, I am assuming that you are dropping this
series.

In any case, this series did break our PM stack. We definitely don't
want to auto-boot the wkup_m3_rproc device, that responsibility will
need to stay with the wkup_m3_ipc driver.

regards
Suman

> 
> CC: Dave Gerlach <d-gerlach@ti.com>
> CC: Suman Anna <s-anna@ti.com>
> CC: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
> ---
> 
> Hi Suman,
> 
> Unfortunately, I don't have a TI device and couldn't test this
> change. Is it possible for you to test this change on TI device?
> 
> Thanks in advance.
> 
> Regards,
> Sarang
> 
>  drivers/remoteproc/wkup_m3_rproc.c |  2 +-
>  drivers/soc/ti/wkup_m3_ipc.c       | 35 ++---------------------------------
>  2 files changed, 3 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
> index 18175d0..79ea022 100644
> --- a/drivers/remoteproc/wkup_m3_rproc.c
> +++ b/drivers/remoteproc/wkup_m3_rproc.c
> @@ -167,7 +167,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	rproc->auto_boot = false;
> +	rproc->auto_boot = true;
>  
>  	wkupm3 = rproc->priv;
>  	wkupm3->rproc = rproc;
> diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
> index 8823cc8..31090d70 100644
> --- a/drivers/soc/ti/wkup_m3_ipc.c
> +++ b/drivers/soc/ti/wkup_m3_ipc.c
> @@ -17,7 +17,6 @@
>  
>  #include <linux/err.h>
>  #include <linux/kernel.h>
> -#include <linux/kthread.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/module.h>
> @@ -365,22 +364,6 @@ void wkup_m3_ipc_put(struct wkup_m3_ipc *m3_ipc)
>  }
>  EXPORT_SYMBOL_GPL(wkup_m3_ipc_put);
>  
> -static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc)
> -{
> -	struct device *dev = m3_ipc->dev;
> -	int ret;
> -
> -	wait_for_completion(&m3_ipc->rproc->firmware_loading_complete);
> -
> -	init_completion(&m3_ipc->sync_complete);
> -
> -	ret = rproc_boot(m3_ipc->rproc);
> -	if (ret)
> -		dev_err(dev, "rproc_boot failed\n");
> -
> -	do_exit(0);
> -}
> -
>  static int wkup_m3_ipc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -388,7 +371,6 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>  	phandle rproc_phandle;
>  	struct rproc *m3_rproc;
>  	struct resource *res;
> -	struct task_struct *task;
>  	struct wkup_m3_ipc *m3_ipc;
>  
>  	m3_ipc = devm_kzalloc(dev, sizeof(*m3_ipc), GFP_KERNEL);
> @@ -402,6 +384,8 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>  		return PTR_ERR(m3_ipc->ipc_mem_base);
>  	}
>  
> +	init_completion(&m3_ipc->sync_complete);
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (!irq) {
>  		dev_err(&pdev->dev, "no irq resource\n");
> @@ -449,25 +433,10 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>  
>  	m3_ipc->ops = &ipc_ops;
>  
> -	/*
> -	 * Wait for firmware loading completion in a thread so we
> -	 * can boot the wkup_m3 as soon as it's ready without holding
> -	 * up kernel boot
> -	 */
> -	task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_ipc,
> -			   "wkup_m3_rproc_loader");
> -
> -	if (IS_ERR(task)) {
> -		dev_err(dev, "can't create rproc_boot thread\n");
> -		goto err_put_rproc;
> -	}
> -
>  	m3_ipc_state = m3_ipc;
>  
>  	return 0;
>  
> -err_put_rproc:
> -	rproc_put(m3_rproc);
>  err_free_mbox:
>  	mbox_free_channel(m3_ipc->mbox);
>  	return ret;
> 

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Bjorn Andersson]

On Wed 21 Dec 19:16 PST 2016, Suman Anna wrote:

> Hi Sarang,
> 
> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
> > The function wkup_m3_rproc_boot_thread waits for asynchronous
> > firmware loading to complete successfully before calling
> > rproc_boot(). The same can be achieved by just setting
> > rproc->auto_boot flag. Change this. As a result this change
> > removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
> > initialization to the wkup_m3_ipc_probe().
> > 
> > Other than the current usage, the firmware_loading_complete is
> > only used in rproc_del() where it's no longer needed.  This
> > change is in preparation for removing firmware_loading_complete
> > completely.
> 
> Based on the comments so far, I am assuming that you are dropping this
> series.
> 

Following up on those comments only revealed that we have several other
similar race conditions, so I'm hoping that Sarangdhar will continue to
work on fixing those - and in this process get rid of this completion.

> In any case, this series did break our PM stack. We definitely don't
> want to auto-boot the wkup_m3_rproc device, that responsibility will
> need to stay with the wkup_m3_ipc driver.
> 

Reviewing the wkup_m3 situation again I see that as we have moved the
resource table parsing to the rproc_boot() path there's no longer any
need for the wkup_m3_ipc driver to wait for the remoteproc-core-internal
completion.

If rproc_get_by_phandle() returns non-NULL it is initialized. We still
don't want to call rproc_boot() from wkup_m3_ipc_probe(), so let's keep
the wkup_m3_rproc_boot_thread().

Sarangdhar, could you update the wkup_m3_ipc patch to just drop the
wait_for_completion() call?

Regards,
Bjorn

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Sarangdhar Joshi]

On 12/22/2016 5:02 AM, Bjorn Andersson wrote:
> On Wed 21 Dec 19:16 PST 2016, Suman Anna wrote:
>
>> Hi Sarang,
>>
>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
>>> The function wkup_m3_rproc_boot_thread waits for asynchronous
>>> firmware loading to complete successfully before calling
>>> rproc_boot(). The same can be achieved by just setting
>>> rproc->auto_boot flag. Change this. As a result this change
>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
>>> initialization to the wkup_m3_ipc_probe().
>>>
>>> Other than the current usage, the firmware_loading_complete is
>>> only used in rproc_del() where it's no longer needed.  This
>>> change is in preparation for removing firmware_loading_complete
>>> completely.
>>
>> Based on the comments so far, I am assuming that you are dropping this
>> series.
>>
>
> Following up on those comments only revealed that we have several other
> similar race conditions, so I'm hoping that Sarangdhar will continue to
> work on fixing those - and in this process get rid of this completion.
>
>> In any case, this series did break our PM stack. We definitely don't
>> want to auto-boot the wkup_m3_rproc device, that responsibility will
>> need to stay with the wkup_m3_ipc driver.
>>
>
> Reviewing the wkup_m3 situation again I see that as we have moved the
> resource table parsing to the rproc_boot() path there's no longer any
> need for the wkup_m3_ipc driver to wait for the remoteproc-core-internal
> completion.
>
> If rproc_get_by_phandle() returns non-NULL it is initialized. We still
> don't want to call rproc_boot() from wkup_m3_ipc_probe(), so let's keep
> the wkup_m3_rproc_boot_thread().

Did you mean it's okay to call rproc_boot() from wkup_m3_ipc_probe()? 
rproc_boot() calls request_firmware() anyways and so there is no need to 
wait for completion of asynchronous firmware loading.

>
> Sarangdhar, could you update the wkup_m3_ipc patch to just drop the
> wait_for_completion() call?

Sure, assuming we should still keep the rproc_boot() call in the kthread.

Regards,
Sarang

>
> Regards,
> Bjorn
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Suman Anna]

Hi Sarang,

On 12/22/2016 06:07 PM, Sarangdhar Joshi wrote:
> On 12/22/2016 5:02 AM, Bjorn Andersson wrote:
>> On Wed 21 Dec 19:16 PST 2016, Suman Anna wrote:
>>
>>> Hi Sarang,
>>>
>>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
>>>> The function wkup_m3_rproc_boot_thread waits for asynchronous
>>>> firmware loading to complete successfully before calling
>>>> rproc_boot(). The same can be achieved by just setting
>>>> rproc->auto_boot flag. Change this. As a result this change
>>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
>>>> initialization to the wkup_m3_ipc_probe().
>>>>
>>>> Other than the current usage, the firmware_loading_complete is
>>>> only used in rproc_del() where it's no longer needed.  This
>>>> change is in preparation for removing firmware_loading_complete
>>>> completely.
>>>
>>> Based on the comments so far, I am assuming that you are dropping this
>>> series.
>>>
>>
>> Following up on those comments only revealed that we have several other
>> similar race conditions, so I'm hoping that Sarangdhar will continue to
>> work on fixing those - and in this process get rid of this completion.
>>
>>> In any case, this series did break our PM stack. We definitely don't
>>> want to auto-boot the wkup_m3_rproc device, that responsibility will
>>> need to stay with the wkup_m3_ipc driver.
>>>
>>
>> Reviewing the wkup_m3 situation again I see that as we have moved the
>> resource table parsing to the rproc_boot() path there's no longer any
>> need for the wkup_m3_ipc driver to wait for the remoteproc-core-internal
>> completion.
>>
>> If rproc_get_by_phandle() returns non-NULL it is initialized. We still
>> don't want to call rproc_boot() from wkup_m3_ipc_probe(), so let's keep
>> the wkup_m3_rproc_boot_thread().
> 
> Did you mean it's okay to call rproc_boot() from wkup_m3_ipc_probe()?
> rproc_boot() calls request_firmware() anyways and so there is no need to
> wait for completion of asynchronous firmware loading.

No, please retain the kthread. I think you miss the purpose of this wait
for completion originally. Before all the core changes in 4.9, the
resource table is parsed in the first asynchronous loading step, and we
had to wait for that step to complete. Now that there's no table parsing
during the request_firmware_no_wait() for non auto-boot, we can get away
from the need for a synchronzition call.

> 
>>
>> Sarangdhar, could you update the wkup_m3_ipc patch to just drop the
>> wait_for_completion() call?
> 
> Sure, assuming we should still keep the rproc_boot() call in the kthread.

Yep.

regards
Suman

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Sarangdhar Joshi]

Hi Suman,

On 12/21/2016 7:16 PM, Suman Anna wrote:
> Hi Sarang,
>
> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
>> The function wkup_m3_rproc_boot_thread waits for asynchronous
>> firmware loading to complete successfully before calling
>> rproc_boot(). The same can be achieved by just setting
>> rproc->auto_boot flag. Change this. As a result this change
>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
>> initialization to the wkup_m3_ipc_probe().
>>
>> Other than the current usage, the firmware_loading_complete is
>> only used in rproc_del() where it's no longer needed.  This
>> change is in preparation for removing firmware_loading_complete
>> completely.
>
> Based on the comments so far, I am assuming that you are dropping this
> series.

No, may not be dropping this. Will try to handle it differently in 
rproc_del() (probably by making use of some state flag).

>
> In any case, this series did break our PM stack. We definitely don't
> want to auto-boot the wkup_m3_rproc device, that responsibility will
> need to stay with the wkup_m3_ipc driver.

Which scenario did it break? Booting up rproc device before 
wkup_m3_ipc_probe() causes issues?

Nevertheless, I think Bjorn's suggestion of just dropping the call to 
wait_for_completion() and keeping kthread looks good, also because of 
the fact that rproc_boot() anyways calls request_firmware() and no 
longer needed to wait on asynchronous loading of firmware.

Regards,
Sarang

>
> regards
> Suman
>
>>
>> CC: Dave Gerlach <d-gerlach@ti.com>
>> CC: Suman Anna <s-anna@ti.com>
>> CC: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org>
>> ---
>>
>> Hi Suman,
>>
>> Unfortunately, I don't have a TI device and couldn't test this
>> change. Is it possible for you to test this change on TI device?
>>
>> Thanks in advance.
>>
>> Regards,
>> Sarang
>>
>>  drivers/remoteproc/wkup_m3_rproc.c |  2 +-
>>  drivers/soc/ti/wkup_m3_ipc.c       | 35 ++---------------------------------
>>  2 files changed, 3 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
>> index 18175d0..79ea022 100644
>> --- a/drivers/remoteproc/wkup_m3_rproc.c
>> +++ b/drivers/remoteproc/wkup_m3_rproc.c
>> @@ -167,7 +167,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
>>  		goto err;
>>  	}
>>
>> -	rproc->auto_boot = false;
>> +	rproc->auto_boot = true;
>>
>>  	wkupm3 = rproc->priv;
>>  	wkupm3->rproc = rproc;
>> diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
>> index 8823cc8..31090d70 100644
>> --- a/drivers/soc/ti/wkup_m3_ipc.c
>> +++ b/drivers/soc/ti/wkup_m3_ipc.c
>> @@ -17,7 +17,6 @@
>>
>>  #include <linux/err.h>
>>  #include <linux/kernel.h>
>> -#include <linux/kthread.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/irq.h>
>>  #include <linux/module.h>
>> @@ -365,22 +364,6 @@ void wkup_m3_ipc_put(struct wkup_m3_ipc *m3_ipc)
>>  }
>>  EXPORT_SYMBOL_GPL(wkup_m3_ipc_put);
>>
>> -static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc)
>> -{
>> -	struct device *dev = m3_ipc->dev;
>> -	int ret;
>> -
>> -	wait_for_completion(&m3_ipc->rproc->firmware_loading_complete);
>> -
>> -	init_completion(&m3_ipc->sync_complete);
>> -
>> -	ret = rproc_boot(m3_ipc->rproc);
>> -	if (ret)
>> -		dev_err(dev, "rproc_boot failed\n");
>> -
>> -	do_exit(0);
>> -}
>> -
>>  static int wkup_m3_ipc_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> @@ -388,7 +371,6 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>>  	phandle rproc_phandle;
>>  	struct rproc *m3_rproc;
>>  	struct resource *res;
>> -	struct task_struct *task;
>>  	struct wkup_m3_ipc *m3_ipc;
>>
>>  	m3_ipc = devm_kzalloc(dev, sizeof(*m3_ipc), GFP_KERNEL);
>> @@ -402,6 +384,8 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>>  		return PTR_ERR(m3_ipc->ipc_mem_base);
>>  	}
>>
>> +	init_completion(&m3_ipc->sync_complete);
>> +
>>  	irq = platform_get_irq(pdev, 0);
>>  	if (!irq) {
>>  		dev_err(&pdev->dev, "no irq resource\n");
>> @@ -449,25 +433,10 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>>
>>  	m3_ipc->ops = &ipc_ops;
>>
>> -	/*
>> -	 * Wait for firmware loading completion in a thread so we
>> -	 * can boot the wkup_m3 as soon as it's ready without holding
>> -	 * up kernel boot
>> -	 */
>> -	task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_ipc,
>> -			   "wkup_m3_rproc_loader");
>> -
>> -	if (IS_ERR(task)) {
>> -		dev_err(dev, "can't create rproc_boot thread\n");
>> -		goto err_put_rproc;
>> -	}
>> -
>>  	m3_ipc_state = m3_ipc;
>>
>>  	return 0;
>>
>> -err_put_rproc:
>> -	rproc_put(m3_rproc);
>>  err_free_mbox:
>>  	mbox_free_channel(m3_ipc->mbox);
>>  	return ret;
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Suman Anna]

Hi Sarang,

>>
>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
>>> The function wkup_m3_rproc_boot_thread waits for asynchronous
>>> firmware loading to complete successfully before calling
>>> rproc_boot(). The same can be achieved by just setting
>>> rproc->auto_boot flag. Change this. As a result this change
>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
>>> initialization to the wkup_m3_ipc_probe().
>>>
>>> Other than the current usage, the firmware_loading_complete is
>>> only used in rproc_del() where it's no longer needed.  This
>>> change is in preparation for removing firmware_loading_complete
>>> completely.
>>
>> Based on the comments so far, I am assuming that you are dropping this
>> series.
> 
> No, may not be dropping this. Will try to handle it differently in
> rproc_del() (probably by making use of some state flag).
>>
>> In any case, this series did break our PM stack. We definitely don't
>> want to auto-boot the wkup_m3_rproc device, that responsibility will
>> need to stay with the wkup_m3_ipc driver.
> 
> Which scenario did it break? Booting up rproc device before
> wkup_m3_ipc_probe() causes issues?

Yes, our state machine requires the wkup_m3_ipc driver to control the
boot of the wkup_m3 remoteproc. The wkup_m3 is not a typical remoteproc,
it is our PM master and is responsible for putting the host processor
into suspend and waking it up in system suspend/cpuidle paths.
The remoteproc infrastructure is only used for load/boot, but the Linux
PM state machine and communication is all controlled by the wkup_m3_ipc
driver.

> 
> Nevertheless, I think Bjorn's suggestion of just dropping the call to
> wait_for_completion() and keeping kthread looks good, also because of
> the fact that rproc_boot() anyways calls request_firmware() and no
> longer needed to wait on asynchronous loading of firmware.

Yeah, I will have to test this, but looking at current code, this should
mostly be ok because of the remoteproc core changes w.r.t resource table
parsing.

regards
Suman

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Suman Anna]

On 12/23/2016 11:05 AM, Suman Anna wrote:
> Hi Sarang,
> 
>>>
>>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
>>>> The function wkup_m3_rproc_boot_thread waits for asynchronous
>>>> firmware loading to complete successfully before calling
>>>> rproc_boot(). The same can be achieved by just setting
>>>> rproc->auto_boot flag. Change this. As a result this change
>>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
>>>> initialization to the wkup_m3_ipc_probe().
>>>>
>>>> Other than the current usage, the firmware_loading_complete is
>>>> only used in rproc_del() where it's no longer needed.  This
>>>> change is in preparation for removing firmware_loading_complete
>>>> completely.
>>>
>>> Based on the comments so far, I am assuming that you are dropping this
>>> series.
>>
>> No, may not be dropping this. Will try to handle it differently in
>> rproc_del() (probably by making use of some state flag).
>>>
>>> In any case, this series did break our PM stack. We definitely don't
>>> want to auto-boot the wkup_m3_rproc device, that responsibility will
>>> need to stay with the wkup_m3_ipc driver.
>>
>> Which scenario did it break? Booting up rproc device before
>> wkup_m3_ipc_probe() causes issues?
> 
> Yes, our state machine requires the wkup_m3_ipc driver to control the
> boot of the wkup_m3 remoteproc. The wkup_m3 is not a typical remoteproc,
> it is our PM master and is responsible for putting the host processor
> into suspend and waking it up in system suspend/cpuidle paths.
> The remoteproc infrastructure is only used for load/boot, but the Linux
> PM state machine and communication is all controlled by the wkup_m3_ipc
> driver.
> 
>>
>> Nevertheless, I think Bjorn's suggestion of just dropping the call to
>> wait_for_completion() and keeping kthread looks good, also because of
>> the fact that rproc_boot() anyways calls request_firmware() and no
>> longer needed to wait on asynchronous loading of firmware.
> 
> Yeah, I will have to test this, but looking at current code, this should
> mostly be ok because of the remoteproc core changes w.r.t resource table
> parsing.

Tested with just the wait_for_completion() removed and it works fine. I
can send a patch for the same if you prefer me to send it.

regards
Suman

> 
> regards
> Suman
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] soc: ti: Use remoteproc auto_boot feature

[Sarangdhar Joshi]

On 12/23/2016 03:57 PM, Suman Anna wrote:
> On 12/23/2016 11:05 AM, Suman Anna wrote:
>> Hi Sarang,
>>
>>>>
>>>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote:
>>>>> The function wkup_m3_rproc_boot_thread waits for asynchronous
>>>>> firmware loading to complete successfully before calling
>>>>> rproc_boot(). The same can be achieved by just setting
>>>>> rproc->auto_boot flag. Change this. As a result this change
>>>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete
>>>>> initialization to the wkup_m3_ipc_probe().
>>>>>
>>>>> Other than the current usage, the firmware_loading_complete is
>>>>> only used in rproc_del() where it's no longer needed.  This
>>>>> change is in preparation for removing firmware_loading_complete
>>>>> completely.
>>>>
>>>> Based on the comments so far, I am assuming that you are dropping this
>>>> series.
>>>
>>> No, may not be dropping this. Will try to handle it differently in
>>> rproc_del() (probably by making use of some state flag).
>>>>
>>>> In any case, this series did break our PM stack. We definitely don't
>>>> want to auto-boot the wkup_m3_rproc device, that responsibility will
>>>> need to stay with the wkup_m3_ipc driver.
>>>
>>> Which scenario did it break? Booting up rproc device before
>>> wkup_m3_ipc_probe() causes issues?
>>
>> Yes, our state machine requires the wkup_m3_ipc driver to control the
>> boot of the wkup_m3 remoteproc. The wkup_m3 is not a typical remoteproc,
>> it is our PM master and is responsible for putting the host processor
>> into suspend and waking it up in system suspend/cpuidle paths.
>> The remoteproc infrastructure is only used for load/boot, but the Linux
>> PM state machine and communication is all controlled by the wkup_m3_ipc
>> driver.

Thanks for explaining. I was missing the fact that the resource table 
parsing during asynchronous call and the one in rproc_boot() are 
different.

>>
>>>
>>> Nevertheless, I think Bjorn's suggestion of just dropping the call to
>>> wait_for_completion() and keeping kthread looks good, also because of
>>> the fact that rproc_boot() anyways calls request_firmware() and no
>>> longer needed to wait on asynchronous loading of firmware.
>>
>> Yeah, I will have to test this, but looking at current code, this should
>> mostly be ok because of the remoteproc core changes w.r.t resource table
>> parsing.
>
> Tested with just the wait_for_completion() removed and it works fine. I
> can send a patch for the same if you prefer me to send it.

Thanks for testing it. I have sent the patch.

Regards,
Sarang

>
> regards
> Suman
>
>>
>> regards
>> Suman
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project