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
