[PATCH 0/4] Davinci remoteproc cleanups/fixes

[PATCH 0/4] Davinci remoteproc cleanups/fixes

[Suman Anna]

Hi Bjorn,

The following series includes various fixes and cleanups for the TI's
Davinci DSP remoteproc driver. Patches are based on 4.12-rc1. Following
is the patch summary:

 - Patch1 updates/fixes the build dependency on DMA_CMA which is not
   selected by default in the davinci_defconfig.
 - Patch 3 fixes a DSP boot/functionality issue with repeated 'start'
   and 'stop' commands using the sysfs 'state' variable.
 - Patch 2 and 4 are cleanups/code simplifications.

regards
Suman

Suman Anna (4):
  remoteproc/davinci: Update Kconfig to depend on DMA_CMA
  remoteproc/davinci: simplify the reset function
  remoteproc/davinci: fix unbalanced reset between start and stop ops
  remoteproc/davinci: streamline the interrupt management

 drivers/remoteproc/Kconfig            |  2 +-
 drivers/remoteproc/da8xx_remoteproc.c | 60 ++++++++---------------------------
 2 files changed, 15 insertions(+), 47 deletions(-)

-- 
2.12.0

[PATCH 1/4] remoteproc/davinci: Update Kconfig to depend on DMA_CMA

[Suman Anna]

The davinci remoteproc driver requires a CMA pool for allocating
memory for virtio vrings/buffers and other sections of the firmware
image. The allocations are done using the DMA API. The CMA option is
currently selected automatically on systems with MMU when davinci
remoteproc is enabled, switch this to a saner depends on dependency
convention. The dependency is also updated to use the DMA_CMA kconfig
symbol that is used for CMA allocations using the DMA API, the CMA
dependency is inherited implicitly.

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..0aafb0d0e9fd 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -52,7 +52,7 @@ config DA8XX_REMOTEPROC
 	tristate "DA8xx/OMAP-L13x remoteproc support"
 	depends on ARCH_DAVINCI_DA8XX
 	depends on REMOTEPROC
-	select CMA if MMU
+	depends on DMA_CMA
 	select RPMSG_VIRTIO
 	help
 	  Say y here to support DA8xx/OMAP-L13x remote processors via the
-- 
2.12.0

[PATCH 2/4] remoteproc/davinci: simplify the reset function

[Suman Anna]

The reset_assert() function is used to make sure the DSP remote
processor is in a reset state regardless of its previous state.
The driver relies on davinci_clk_reset_{assert,deassert}()
functions for reset management which take in a clock parameter.
The assert_reset() performs a clk_get()/clk_put() cycle to
acquire the clock handle to use with this function. This is
totally unnecessary and the code can be simplified to use
the clock acquired during probe and directly use the reset
functions, so simplify this logic.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/da8xx_remoteproc.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/remoteproc/da8xx_remoteproc.c b/drivers/remoteproc/da8xx_remoteproc.c
index 3814de28599c..fcd3cecb4967 100644
--- a/drivers/remoteproc/da8xx_remoteproc.c
+++ b/drivers/remoteproc/da8xx_remoteproc.c
@@ -157,22 +157,6 @@ static const struct rproc_ops da8xx_rproc_ops = {
 	.kick = da8xx_rproc_kick,
 };
 
-static int reset_assert(struct device *dev)
-{
-	struct clk *dsp_clk;
-
-	dsp_clk = clk_get(dev, NULL);
-	if (IS_ERR(dsp_clk)) {
-		dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk));
-		return PTR_ERR(dsp_clk);
-	}
-
-	davinci_clk_reset_assert(dsp_clk);
-	clk_put(dsp_clk);
-
-	return 0;
-}
-
 static int da8xx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -223,6 +207,7 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
 
 	drproc = rproc->priv;
 	drproc->rproc = rproc;
+	drproc->dsp_clk = dsp_clk;
 	rproc->has_iommu = false;
 
 	platform_set_drvdata(pdev, rproc);
@@ -241,7 +226,7 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
 	 * *not* in reset, but da8xx_rproc_start() needs the DSP to be
 	 * held in reset at the time it is called.
 	 */
-	ret = reset_assert(dev);
+	ret = davinci_clk_reset_assert(drproc->dsp_clk);
 	if (ret)
 		goto free_rproc;
 
@@ -250,7 +235,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
 	drproc->ack_fxn = irq_data->chip->irq_ack;
 	drproc->irq_data = irq_data;
 	drproc->irq = irq;
-	drproc->dsp_clk = dsp_clk;
 
 	ret = rproc_add(rproc);
 	if (ret) {
@@ -268,7 +252,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
 
 static int da8xx_rproc_remove(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
 
@@ -280,7 +263,7 @@ static int da8xx_rproc_remove(struct platform_device *pdev)
 	 * Without the reset, the DSP can lockup permanently when it
 	 * begins executing garbage.
 	 */
-	reset_assert(dev);
+	davinci_clk_reset_assert(drproc->dsp_clk);
 
 	/*
 	 * The devm subsystem might end up releasing things before
-- 
2.12.0

[PATCH 3/4] remoteproc/davinci: fix unbalanced reset between start and stop ops

[Suman Anna]

The davinci remoteproc driver is currently de-asserting the reset in
its rproc .start() ops, but is not asserting the reset in its .stop()
ops. This leaves the remote processor to not boot properly when using
the sysfs 'state' variable between multiple start and stop operations.
On the other hand, a reset is being asserted unconditionally in the
driver remove function to alleviate some of these issues.

Move this reset assertion logic into the .stop() ops implementation
to fix the sysfs state-machine and the unbalanced reset. The logic
from remove is still effective since .stop() ops will be invoked
during the remove due to the enabled 'auto-boot' support. The probe
already has support for asserting the reset in case the DSP is not
in reset for some reason.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/da8xx_remoteproc.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/remoteproc/da8xx_remoteproc.c b/drivers/remoteproc/da8xx_remoteproc.c
index fcd3cecb4967..99539cec1329 100644
--- a/drivers/remoteproc/da8xx_remoteproc.c
+++ b/drivers/remoteproc/da8xx_remoteproc.c
@@ -137,6 +137,7 @@ static int da8xx_rproc_stop(struct rproc *rproc)
 {
 	struct da8xx_rproc *drproc = rproc->priv;
 
+	davinci_clk_reset_assert(drproc->dsp_clk);
 	clk_disable(drproc->dsp_clk);
 
 	return 0;
@@ -256,16 +257,6 @@ static int da8xx_rproc_remove(struct platform_device *pdev)
 	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
 
 	/*
-	 * It's important to place the DSP in reset before going away,
-	 * since a subsequent insmod of this module may enable the DSP's
-	 * clock before its program/boot-address has been loaded and
-	 * before this module's probe has had a chance to reset the DSP.
-	 * Without the reset, the DSP can lockup permanently when it
-	 * begins executing garbage.
-	 */
-	davinci_clk_reset_assert(drproc->dsp_clk);
-
-	/*
 	 * The devm subsystem might end up releasing things before
 	 * freeing the irq, thus allowing an interrupt to sneak in while
 	 * the device is being removed.  This should prevent that.
-- 
2.12.0

[PATCH 4/4] remoteproc/davinci: streamline the interrupt management

[Suman Anna]

The davinci remoteproc driver is currently requesting its interrupt
that deals with the virtio kicks in probe, and that too before all
the associated variables used by the handler are initialized. This
is a lot in advance before the DSP remote processor is even loaded
and booted and is not essential. Streamline the interrupt request
and freeing operations instead alongside the boot and shutdown of
the remote processor.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/da8xx_remoteproc.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/remoteproc/da8xx_remoteproc.c b/drivers/remoteproc/da8xx_remoteproc.c
index 99539cec1329..c7ad818f7a48 100644
--- a/drivers/remoteproc/da8xx_remoteproc.c
+++ b/drivers/remoteproc/da8xx_remoteproc.c
@@ -117,6 +117,7 @@ static int da8xx_rproc_start(struct rproc *rproc)
 	struct device *dev = rproc->dev.parent;
 	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
 	struct clk *dsp_clk = drproc->dsp_clk;
+	int ret;
 
 	/* hw requires the start (boot) address be on 1KB boundary */
 	if (rproc->bootaddr & 0x3ff) {
@@ -125,6 +126,14 @@ static int da8xx_rproc_start(struct rproc *rproc)
 		return -EINVAL;
 	}
 
+	/* everything the ISR needs is now setup, so hook it up */
+	ret = request_threaded_irq(drproc->irq, da8xx_rproc_callback,
+				   handle_event, 0, "da8xx-remoteproc", rproc);
+	if (ret) {
+		dev_err(dev, "request_threaded_irq error: %d\n", ret);
+		return ret;
+	}
+
 	writel(rproc->bootaddr, drproc->bootreg);
 
 	clk_enable(dsp_clk);
@@ -140,6 +149,8 @@ static int da8xx_rproc_stop(struct rproc *rproc)
 	davinci_clk_reset_assert(drproc->dsp_clk);
 	clk_disable(drproc->dsp_clk);
 
+	free_irq(drproc->irq, rproc);
+
 	return 0;
 }
 
@@ -213,15 +224,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rproc);
 
-	/* everything the ISR needs is now setup, so hook it up */
-	ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback,
-					handle_event, 0, "da8xx-remoteproc",
-					rproc);
-	if (ret) {
-		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
-		goto free_rproc;
-	}
-
 	/*
 	 * rproc_add() can end up enabling the DSP's clk with the DSP
 	 * *not* in reset, but da8xx_rproc_start() needs the DSP to be
@@ -254,14 +256,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
 static int da8xx_rproc_remove(struct platform_device *pdev)
 {
 	struct rproc *rproc = platform_get_drvdata(pdev);
-	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
-
-	/*
-	 * The devm subsystem might end up releasing things before
-	 * freeing the irq, thus allowing an interrupt to sneak in while
-	 * the device is being removed.  This should prevent that.
-	 */
-	disable_irq(drproc->irq);
 
 	rproc_del(rproc);
 	rproc_free(rproc);
-- 
2.12.0

Re: [PATCH 0/4] Davinci remoteproc cleanups/fixes

[Suman Anna]

Bjorn,

On 05/18/2017 05:08 PM, Suman Anna wrote:
> Hi Bjorn,
> 
> The following series includes various fixes and cleanups for the TI's
> Davinci DSP remoteproc driver. Patches are based on 4.12-rc1. Following
> is the patch summary:
> 
>  - Patch1 updates/fixes the build dependency on DMA_CMA which is not
>    selected by default in the davinci_defconfig.
>  - Patch 3 fixes a DSP boot/functionality issue with repeated 'start'
>    and 'stop' commands using the sysfs 'state' variable.
>  - Patch 2 and 4 are cleanups/code simplifications.

If you don't have any comments, can you queue these patches for v4.13?

Thanks
Suman

> 
> regards
> Suman
> 
> Suman Anna (4):
>   remoteproc/davinci: Update Kconfig to depend on DMA_CMA
>   remoteproc/davinci: simplify the reset function
>   remoteproc/davinci: fix unbalanced reset between start and stop ops
>   remoteproc/davinci: streamline the interrupt management
> 
>  drivers/remoteproc/Kconfig            |  2 +-
>  drivers/remoteproc/da8xx_remoteproc.c | 60 ++++++++---------------------------
>  2 files changed, 15 insertions(+), 47 deletions(-)
> 

Re: [PATCH 0/4] Davinci remoteproc cleanups/fixes

[Suman Anna]

On 06/08/2017 03:47 PM, Suman Anna wrote:
> Bjorn,
> 
> On 05/18/2017 05:08 PM, Suman Anna wrote:
>> Hi Bjorn,
>>
>> The following series includes various fixes and cleanups for the TI's
>> Davinci DSP remoteproc driver. Patches are based on 4.12-rc1. Following
>> is the patch summary:
>>
>>  - Patch1 updates/fixes the build dependency on DMA_CMA which is not
>>    selected by default in the davinci_defconfig.
>>  - Patch 3 fixes a DSP boot/functionality issue with repeated 'start'
>>    and 'stop' commands using the sysfs 'state' variable.
>>  - Patch 2 and 4 are cleanups/code simplifications.
> 
> If you don't have any comments, can you queue these patches for v4.13?

ping on this..

> 
> Thanks
> Suman
> 
>>
>> regards
>> Suman
>>
>> Suman Anna (4):
>>   remoteproc/davinci: Update Kconfig to depend on DMA_CMA
>>   remoteproc/davinci: simplify the reset function
>>   remoteproc/davinci: fix unbalanced reset between start and stop ops
>>   remoteproc/davinci: streamline the interrupt management
>>
>>  drivers/remoteproc/Kconfig            |  2 +-
>>  drivers/remoteproc/da8xx_remoteproc.c | 60 ++++++++---------------------------
>>  2 files changed, 15 insertions(+), 47 deletions(-)
>>
> 

Re: [PATCH 4/4] remoteproc/davinci: streamline the interrupt management

[Bjorn Andersson]

On Thu 18 May 15:09 PDT 2017, Suman Anna wrote:

> The davinci remoteproc driver is currently requesting its interrupt
> that deals with the virtio kicks in probe, and that too before all
> the associated variables used by the handler are initialized. This
> is a lot in advance before the DSP remote processor is even loaded
> and booted and is not essential. Streamline the interrupt request
> and freeing operations instead alongside the boot and shutdown of
> the remote processor.
> 

I do prefer that all resources are acquired at probe() time, rather than
handled upon each start/stop. In the current handle_event()
implementation the remoteproc code will not find the yet unallocated
notify-id's and do nothing. So this seems okay.

[..]
> @@ -213,15 +224,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, rproc);
>  
> -	/* everything the ISR needs is now setup, so hook it up */
> -	ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback,
> -					handle_event, 0, "da8xx-remoteproc",
> -					rproc);
> -	if (ret) {
> -		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
> -		goto free_rproc;
> -	}

In the error paths after this the driver will end up freeing the rproc
context before disabling the irq, so these cases need a call to
disable_irq().

> -
>  	/*
>  	 * rproc_add() can end up enabling the DSP's clk with the DSP
>  	 * *not* in reset, but da8xx_rproc_start() needs the DSP to be
> @@ -254,14 +256,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
>  static int da8xx_rproc_remove(struct platform_device *pdev)
>  {
>  	struct rproc *rproc = platform_get_drvdata(pdev);
> -	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
> -
> -	/*
> -	 * The devm subsystem might end up releasing things before
> -	 * freeing the irq, thus allowing an interrupt to sneak in while
> -	 * the device is being removed.  This should prevent that.
> -	 */

devres _will not_ disable the IRQ until after remove() returns, making it
possible for the interrupt handler to be executed after the rproc
context is freed.

So this comment would benefit from an update.

> -	disable_irq(drproc->irq);
>  
>  	rproc_del(rproc);
>  	rproc_free(rproc);

Regards,
Bjorn

Re: [PATCH 0/4] Davinci remoteproc cleanups/fixes

[Bjorn Andersson]

On Thu 18 May 15:08 PDT 2017, Suman Anna wrote:

> Hi Bjorn,
> 
> The following series includes various fixes and cleanups for the TI's
> Davinci DSP remoteproc driver. Patches are based on 4.12-rc1. Following
> is the patch summary:
> 
>  - Patch1 updates/fixes the build dependency on DMA_CMA which is not
>    selected by default in the davinci_defconfig.
>  - Patch 3 fixes a DSP boot/functionality issue with repeated 'start'
>    and 'stop' commands using the sysfs 'state' variable.
>  - Patch 2 and 4 are cleanups/code simplifications.
> 

Patches 1, 2 and 3 applied to rproc-next.

Regards,
Bjorn

Re: [PATCH 4/4] remoteproc/davinci: streamline the interrupt management

[Suman Anna]

Hi Bjorn,

On 06/25/2017 04:19 PM, Bjorn Andersson wrote:
> On Thu 18 May 15:09 PDT 2017, Suman Anna wrote:
> 
>> The davinci remoteproc driver is currently requesting its interrupt
>> that deals with the virtio kicks in probe, and that too before all
>> the associated variables used by the handler are initialized. This
>> is a lot in advance before the DSP remote processor is even loaded
>> and booted and is not essential. Streamline the interrupt request
>> and freeing operations instead alongside the boot and shutdown of
>> the remote processor.
>>
> 
> I do prefer that all resources are acquired at probe() time, rather than
> handled upon each start/stop. In the current handle_event()
> implementation the remoteproc code will not find the yet unallocated
> notify-id's and do nothing. So this seems okay.
> 
> [..]
>> @@ -213,15 +224,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, rproc);
>>  
>> -	/* everything the ISR needs is now setup, so hook it up */
>> -	ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback,
>> -					handle_event, 0, "da8xx-remoteproc",
>> -					rproc);
>> -	if (ret) {
>> -		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
>> -		goto free_rproc;
>> -	}
> 
> In the error paths after this the driver will end up freeing the rproc
> context before disabling the irq, so these cases need a call to
> disable_irq().

Hmm, I am not sure I understand why we need disable_irq() when we are
not even requesting it? This is deleting code, not adding. The IRQ
request and free are now balanced in the start and stop ops. The only
call here is a platform_get_irq() which doesn't need any cleanup.

> 
>> -
>>  	/*
>>  	 * rproc_add() can end up enabling the DSP's clk with the DSP
>>  	 * *not* in reset, but da8xx_rproc_start() needs the DSP to be
>> @@ -254,14 +256,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
>>  static int da8xx_rproc_remove(struct platform_device *pdev)
>>  {
>>  	struct rproc *rproc = platform_get_drvdata(pdev);
>> -	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
>> -
>> -	/*
>> -	 * The devm subsystem might end up releasing things before
>> -	 * freeing the irq, thus allowing an interrupt to sneak in while
>> -	 * the device is being removed.  This should prevent that.
>> -	 */
> 
> devres _will not_ disable the IRQ until after remove() returns, making it
> possible for the interrupt handler to be executed after the rproc
> context is freed.
> 
> So this comment would benefit from an update.

Again, this is deleting code, not adding. The remove after this cleanup
will simply be invoking the rproc_del() and rproc_free() call, and
rproc_del() does end up calling the stop since we do use auto-boot where
we free the irq.

regards
Suman

> 
>> -	disable_irq(drproc->irq);
>>  
>>  	rproc_del(rproc);
>>  	rproc_free(rproc);
> 
> Regards,
> Bjorn
> 

Re: [PATCH 4/4] remoteproc/davinci: streamline the interrupt management

[Bjorn Andersson]

On Mon 26 Jun 09:09 PDT 2017, Suman Anna wrote:

> Hi Bjorn,
> 
> On 06/25/2017 04:19 PM, Bjorn Andersson wrote:
> > On Thu 18 May 15:09 PDT 2017, Suman Anna wrote:
> > 
> >> The davinci remoteproc driver is currently requesting its interrupt
> >> that deals with the virtio kicks in probe, and that too before all
> >> the associated variables used by the handler are initialized. This
> >> is a lot in advance before the DSP remote processor is even loaded
> >> and booted and is not essential. Streamline the interrupt request
> >> and freeing operations instead alongside the boot and shutdown of
> >> the remote processor.
> >>
> > 
> > I do prefer that all resources are acquired at probe() time, rather than
> > handled upon each start/stop. In the current handle_event()
> > implementation the remoteproc code will not find the yet unallocated
> > notify-id's and do nothing. So this seems okay.
> > 
> > [..]
> >> @@ -213,15 +224,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
> >>  
> >>  	platform_set_drvdata(pdev, rproc);
> >>  
> >> -	/* everything the ISR needs is now setup, so hook it up */
> >> -	ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback,
> >> -					handle_event, 0, "da8xx-remoteproc",
> >> -					rproc);
> >> -	if (ret) {
> >> -		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
> >> -		goto free_rproc;
> >> -	}
> > 
> > In the error paths after this the driver will end up freeing the rproc
> > context before disabling the irq, so these cases need a call to
> > disable_irq().
> 
> Hmm, I am not sure I understand why we need disable_irq() when we are
> not even requesting it? This is deleting code, not adding. The IRQ
> request and free are now balanced in the start and stop ops. The only
> call here is a platform_get_irq() which doesn't need any cleanup.
> 

I prefer to keep the initialization of the irq at probe time. What I
tried to say was that the code is currently broken in regards to the
(theoretical?) possibility of the interrupt handler being invoked after
"rproc" has been freed.

> > 
> >> -
> >>  	/*
> >>  	 * rproc_add() can end up enabling the DSP's clk with the DSP
> >>  	 * *not* in reset, but da8xx_rproc_start() needs the DSP to be
> >> @@ -254,14 +256,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
> >>  static int da8xx_rproc_remove(struct platform_device *pdev)
> >>  {
> >>  	struct rproc *rproc = platform_get_drvdata(pdev);
> >> -	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
> >> -
> >> -	/*
> >> -	 * The devm subsystem might end up releasing things before
> >> -	 * freeing the irq, thus allowing an interrupt to sneak in while
> >> -	 * the device is being removed.  This should prevent that.
> >> -	 */
> > 
> > devres _will not_ disable the IRQ until after remove() returns, making it
> > possible for the interrupt handler to be executed after the rproc
> > context is freed.
> > 
> > So this comment would benefit from an update.
> 
> Again, this is deleting code, not adding. The remove after this cleanup
> will simply be invoking the rproc_del() and rproc_free() call, and
> rproc_del() does end up calling the stop since we do use auto-boot where
> we free the irq.
> 

I would like to keep the request_irq in the probe() and as such a
disable_irq is needed either in stop() or here. If we leave it here
there's room to improve the comment.

Regards,
Bjorn

Re: [PATCH 4/4] remoteproc/davinci: streamline the interrupt management

[Suman Anna]

On 06/27/2017 12:38 AM, Bjorn Andersson wrote:
> On Mon 26 Jun 09:09 PDT 2017, Suman Anna wrote:
> 
>> Hi Bjorn,
>>
>> On 06/25/2017 04:19 PM, Bjorn Andersson wrote:
>>> On Thu 18 May 15:09 PDT 2017, Suman Anna wrote:
>>>
>>>> The davinci remoteproc driver is currently requesting its interrupt
>>>> that deals with the virtio kicks in probe, and that too before all
>>>> the associated variables used by the handler are initialized. This
>>>> is a lot in advance before the DSP remote processor is even loaded
>>>> and booted and is not essential. Streamline the interrupt request
>>>> and freeing operations instead alongside the boot and shutdown of
>>>> the remote processor.
>>>>
>>>
>>> I do prefer that all resources are acquired at probe() time, rather than
>>> handled upon each start/stop. In the current handle_event()
>>> implementation the remoteproc code will not find the yet unallocated
>>> notify-id's and do nothing. So this seems okay.
>>>
>>> [..]
>>>> @@ -213,15 +224,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
>>>>  
>>>>  	platform_set_drvdata(pdev, rproc);
>>>>  
>>>> -	/* everything the ISR needs is now setup, so hook it up */
>>>> -	ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback,
>>>> -					handle_event, 0, "da8xx-remoteproc",
>>>> -					rproc);
>>>> -	if (ret) {
>>>> -		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
>>>> -		goto free_rproc;
>>>> -	}
>>>
>>> In the error paths after this the driver will end up freeing the rproc
>>> context before disabling the irq, so these cases need a call to
>>> disable_irq().
>>
>> Hmm, I am not sure I understand why we need disable_irq() when we are
>> not even requesting it? This is deleting code, not adding. The IRQ
>> request and free are now balanced in the start and stop ops. The only
>> call here is a platform_get_irq() which doesn't need any cleanup.
>>
> 
> I prefer to keep the initialization of the irq at probe time. What I
> tried to say was that the code is currently broken in regards to the
> (theoretical?) possibility of the interrupt handler being invoked after
> "rproc" has been freed.
> 
>>>
>>>> -
>>>>  	/*
>>>>  	 * rproc_add() can end up enabling the DSP's clk with the DSP
>>>>  	 * *not* in reset, but da8xx_rproc_start() needs the DSP to be
>>>> @@ -254,14 +256,6 @@ static int da8xx_rproc_probe(struct platform_device *pdev)
>>>>  static int da8xx_rproc_remove(struct platform_device *pdev)
>>>>  {
>>>>  	struct rproc *rproc = platform_get_drvdata(pdev);
>>>> -	struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
>>>> -
>>>> -	/*
>>>> -	 * The devm subsystem might end up releasing things before
>>>> -	 * freeing the irq, thus allowing an interrupt to sneak in while
>>>> -	 * the device is being removed.  This should prevent that.
>>>> -	 */
>>>
>>> devres _will not_ disable the IRQ until after remove() returns, making it
>>> possible for the interrupt handler to be executed after the rproc
>>> context is freed.
>>>
>>> So this comment would benefit from an update.
>>
>> Again, this is deleting code, not adding. The remove after this cleanup
>> will simply be invoking the rproc_del() and rproc_free() call, and
>> rproc_del() does end up calling the stop since we do use auto-boot where
>> we free the irq.
>>
> 
> I would like to keep the request_irq in the probe() and as such a
> disable_irq is needed either in stop() or here. If we leave it here
> there's room to improve the comment.

Yeah, I did understand your statements after the discussion on the
keystone remoteproc thread, so ok with dropping this. The code already
has the disable_irq() in remove before rproc_free(), so just needs a
comment improvement. I will have to check the other sequences if there
are some missing dependencies, before revising this patch.

regards
Suman