On 4 February 2015 at 10:56, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > Hi Ulf, > > On Wed, Feb 4, 2015 at 10:35 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> On 4 February 2015 at 03:08, Vinod Koul <vinod.koul@xxxxxxxxx> wrote: >>> On Mon, Feb 02, 2015 at 10:35:10AM +0100, Ulf Hansson wrote: >>>> On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@xxxxxxxxx> wrote: >>>> > During system reboot, the sh-dma-engine device may be runtime-suspended, >>>> > causing a crash: >>>> > >>>> > Unhandled fault: imprecise external abort (0x1406) at 0x0002c02c >>>> > Internal error: : 1406 [#1] SMP ARM >>>> > ... >>>> > PC is at sh_dmae_ctl_stop+0x28/0x64 >>>> > LR is at sh_dmae_ctl_stop+0x24/0x64 >>>> > >>>> > If the sh-dma-engine is runtime-suspended, its module clock is turned >>>> > off, and its registers cannot be accessed. >>>> > >>>> > To fix this, change the driver's .shutdown() callback: >>>> > - If the device is runtime suspended, do nothing, >>>> > - Else, explicitly runtime-resume the device, to avoid the device from >>>> > being suspended while sh_dmae_ctl_stop() is being executed. >>>> > >>>> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> >>>> > --- >>>> > v2: >>>> > - Do nothing if we're runtime suspended. >>>> > --- >>>> > drivers/dma/sh/shdmac.c | 6 ++++++ >>>> > 1 file changed, 6 insertions(+) >>>> > >>>> > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c >>>> > index aec8a84784a469d7..2de30e8e7d9290b9 100644 >>>> > --- a/drivers/dma/sh/shdmac.c >>>> > +++ b/drivers/dma/sh/shdmac.c >>>> > @@ -585,7 +585,13 @@ static void sh_dmae_chan_remove(struct sh_dmae_device *shdev) >>>> > static void sh_dmae_shutdown(struct platform_device *pdev) >>>> > { >>>> > struct sh_dmae_device *shdev = platform_get_drvdata(pdev); >>>> > + >>>> > + if (pm_runtime_suspended(&pdev->dev)) >>>> > + return; >>>> > + >>>> > + pm_runtime_get_sync(&pdev->dev); >>>> > sh_dmae_ctl_stop(shdev); >>>> >>>> I can't find that sh_dmae_ctl_stop() is invoked from the runtime PM >>>> suspend callback. That means the device will be "removed" differently, >>>> depending on it's runtime PM status (due to the upper check for >>>> pm_runtime_suspended() ) . Is that really what you want? >>> I think the patch description is the key. "During system reboot, the >>> sh-dma-engine device may be runtime-suspended, causing a crash" >>> >>> The runtime-suspended device is already idle and has removed its clock. >> >> That's not my point. The device will be shutdown differently, >> depending if it's runtime PM suspended or not. > > You're right. > >> So, if it's only about gating clocks then why do even bother invoking >> sh_dmae_ctl_stop() in this path. > > sh_dmae_ctl_stop() stops the DMA controller. But this is the "master stop". > At this time, the individual channels should have been stopped by dmae_halt(). > > So you prefer my V1 patch instead, which unconditionally runtime-resumed > the device, so sh_dmae_ctl_stop() can be called? > http://www.spinics.net/lists/dmaengine/msg02954.html Well, that one didn't disable runtime PM, but did pm_runtime_put() when done. I guess that's because you have a PM domain controlling PM clocks which you would like to gate as well? Maybe pm_runtime_put_sync() would be better? Then disable runtime PM? > > Alternatively... > > Both sh_dmae_resume() and sh_dmae_runtime_resume() call sh_dmae_rst(), > which re-initializes the DMAOR register. > To make it symmetric, we can move the call to sh_dmae_ctl_stop() to > sh_dmae_suspend() and sh_dmae_runtime_suspend() instead? That could make sense. In principle that would enable you to remove the ->shutdown() callback, if that's possible I would have done that. As I had a look in the driver in more detail, I believe there are a similar issue to fix as @subject patch does, but for the ->remove() callback. In there, I doubt runtime PM is handled properly. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html