| Message ID | 20170518220902.2846-5-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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
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);
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(-)