[PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Geert Uytterhoeven]

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@glider.be>
---
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);
+	pm_runtime_put(&pdev->dev);
 }
 
 static int sh_dmae_runtime_suspend(struct device *dev)
-- 
1.9.1

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Geert Uytterhoeven]

Ping?

On Mon, Jan 5, 2015 at 11:46 AM, Geert Uytterhoeven
<geert+renesas@glider.be> 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@glider.be>
> ---
> 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);
> +       pm_runtime_put(&pdev->dev);
>  }
>
>  static int sh_dmae_runtime_suspend(struct device *dev)
> --
> 1.9.1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Krzysztof Kozlowski]

2015-01-05 11:46 GMT+01:00 Geert Uytterhoeven <geert+renesas@glider.be>:
> 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@glider.be>
> ---
> 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))

This still looks a little racy. What if runtime resume happens exactly
here? I think safer would be disabling runtime PM before checking for
suspend state.

Best regards,
Krzysztof

> +               return;
> +
> +       pm_runtime_get_sync(&pdev->dev);
>         sh_dmae_ctl_stop(shdev);
> +       pm_runtime_put(&pdev->dev);
>  }
>
>  static int sh_dmae_runtime_suspend(struct device *dev)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Geert Uytterhoeven]

Hi Krzysztof,

On Fri, Jan 30, 2015 at 10:46 AM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-01-05 11:46 GMT+01:00 Geert Uytterhoeven <geert+renesas@glider.be>:
>> 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@glider.be>
>> ---
>> 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))
>
> This still looks a little racy. What if runtime resume happens exactly
> here? I think safer would be disabling runtime PM before checking for
> suspend state.

I don't think new requests can come in while .shutdown() is called, so
the device cannot be resumed here.

>> +               return;
>> +
>> +       pm_runtime_get_sync(&pdev->dev);
>>         sh_dmae_ctl_stop(shdev);
>> +       pm_runtime_put(&pdev->dev);
>>  }
>>
>>  static int sh_dmae_runtime_suspend(struct device *dev)
>> --
>> 1.9.1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Ulf Hansson]

On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@glider.be> 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@glider.be>
> ---
> 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?

> +       pm_runtime_put(&pdev->dev);
>  }
>
>  static int sh_dmae_runtime_suspend(struct device *dev)
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Vinod Koul]

On Mon, Feb 02, 2015 at 10:35:10AM +0100, Ulf Hansson wrote:
> On 5 January 2015 at 11:46, Geert Uytterhoeven <geert+renesas@glider.be> 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@glider.be>
> > ---
> > 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.

-- 
~Vinod

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Ulf Hansson]

On 4 February 2015 at 03:08, Vinod Koul <vinod.koul@intel.com> 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@glider.be> 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@glider.be>
>> > ---
>> > 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.

So, if it's only about gating clocks then why do even bother invoking
sh_dmae_ctl_stop() in this path.

Kind regards
Uffe

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Feb 4, 2015 at 10:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 4 February 2015 at 03:08, Vinod Koul <vinod.koul@intel.com> 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@glider.be> 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@glider.be>
>>> > ---
>>> > 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

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?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Ulf Hansson]

On 4 February 2015 at 10:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Wed, Feb 4, 2015 at 10:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 4 February 2015 at 03:08, Vinod Koul <vinod.koul@intel.com> 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@glider.be> 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@glider.be>
>>>> > ---
>>>> > 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

Re: [PATCH v2] dmaengine: shdma: Runtime-resume device in .shutdown()

[Vinod Koul]

On Wed, Feb 04, 2015 at 10:56:00AM +0100, Geert Uytterhoeven wrote:
> >>> 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
That forces device to resume when we are doinga  shutdown...
> 
> 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?
I think thats a btter idea..

-- 
~Vinod