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
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
