[PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Ulrich Hecht]

Hi!

These patches make sure that the device is up while the suspend/resume code
is executed. Up-port from the BSP, tested not to break stuff.

CU
Uli


Hien Dang (2):
  serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
  serial: sh-sci: Use pm_runtime_get_sync()/put() on resume

 drivers/tty/serial/sh-sci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH 1/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend

[Ulrich Hecht]

From: Hien Dang <hien.dang.eb@renesas.com>

Since commit '39dd0f234fc37d ("PM / Domains: Allow runtime PM during system
PM phases")', runtime PM may be in suspended state when the module
registers are backed up. It is therefore necessary to ensure the device is
on during suspend by using pm_runtime_get_sync()/pm_runtime_put().

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
[uli: edited description for clarity]
Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serial/sh-sci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index fdbbff5..22d7a78 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3246,8 +3246,11 @@ static __maybe_unused int sci_suspend(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
+		pm_runtime_get_sync(sport->port.dev);
 		uart_suspend_port(&sci_uart_driver, &sport->port);
+		pm_runtime_put(sport->port.dev);
+	}
 
 	return 0;
 }
-- 
2.7.4

[PATCH 2/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on resume

[Ulrich Hecht]

From: Hien Dang <hien.dang.eb@renesas.com>

Since commit '39dd0f234fc37d ("PM / Domains: Allow runtime PM during system
PM phases")', runtime PM may be in suspended state during the system
suspend phase. It is therefore necessary to call pm_runtime_get_sync()/
pm_runtime_put() when accessing the hardware.

This modification is the counterpart for the resume case. It ensures
stability of the system, should the kernel allow the devices's runtime
suspend state to change during the system resume phase as well.

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
[uli: edited description for clarity]
Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serial/sh-sci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 22d7a78..d5a1acb 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3259,8 +3259,11 @@ static __maybe_unused int sci_resume(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
+		pm_runtime_get_sync(sport->port.dev);
 		uart_resume_port(&sci_uart_driver, &sport->port);
+		pm_runtime_put(sport->port.dev);
+	}
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> These patches make sure that the device is up while the suspend/resume code
> is executed. Up-port from the BSP, tested not to break stuff.
>
> Hien Dang (2):
>   serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
>   serial: sh-sci: Use pm_runtime_get_sync()/put() on resume

I don't think it makes much sense to split this in two parts...

Furthermore, shouldn't this be handled by the serial core, calling
uart_change_pm()?

It looks like uart_resume_port() already changes the port's state to
enabled, while  uart_suspend_port() assumes the port is already enabled,
and disables it.

Perhaps handling is not correct for some code paths?

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 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]


> > These patches make sure that the device is up while the suspend/resume code
> > is executed. Up-port from the BSP, tested not to break stuff.
> >
> > Hien Dang (2):
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on resume
> 
> I don't think it makes much sense to split this in two parts...
> 
> Furthermore, shouldn't this be handled by the serial core, calling
> uart_change_pm()?
> 
> It looks like uart_resume_port() already changes the port's state to
> enabled, while  uart_suspend_port() assumes the port is already enabled,
> and disables it.
> 
> Perhaps handling is not correct for some code paths?

Ulrich, do you have an answer to Geert's question?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Fri, Apr 13, 2018 at 07:27:59PM +0200, Geert Uytterhoeven wrote:
> Hi Ulrich,
> 
> On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht
> <ulrich.hecht+renesas@gmail.com> wrote:
> > These patches make sure that the device is up while the suspend/resume code
> > is executed. Up-port from the BSP, tested not to break stuff.
> >
> > Hien Dang (2):
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on resume
> 
> I don't think it makes much sense to split this in two parts...
> 
> Furthermore, shouldn't this be handled by the serial core, calling
> uart_change_pm()?
> 
> It looks like uart_resume_port() already changes the port's state to
> enabled, while  uart_suspend_port() assumes the port is already enabled,
> and disables it.
> 
> Perhaps handling is not correct for some code paths?

Ulrich, what is your take on this?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Ulrich Hecht]

Sorry for the delay...

> On April 13, 2018 at 7:27 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht
> <ulrich.hecht+renesas@gmail.com> wrote:
> > These patches make sure that the device is up while the suspend/resume code
> > is executed. Up-port from the BSP, tested not to break stuff.
> >
> > Hien Dang (2):
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on resume
> 
> I don't think it makes much sense to split this in two parts...
> 
> Furthermore, shouldn't this be handled by the serial core, calling
> uart_change_pm()?
> 
> It looks like uart_resume_port() already changes the port's state to
> enabled, while  uart_suspend_port() assumes the port is already enabled,
> and disables it.
> 
> Perhaps handling is not correct for some code paths?

The way I understand it, the problem this intends to fix is not the state the device ends up in, but that it needs to be powered while registers are read or written.

It seems to me that that the current "resume" code should work in that respect, because it changes the PM state to "on" in uart_resume_port() before any access to hardware registers takes place, so there is nothing that needs to be fixed.

That may be different for the "suspend" part, though, because it assumes that the PM state is "on", and I think that is what the patch asserts to not be a valid assumption anymore. It's hard to tell if that is true, though, because I cannot reproduce the issue here; it just works either way...

CU
Uli

Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

Hey guys,

I stumbled over this one again and want to make sure we have a
conclusion here.

> The way I understand it, the problem this intends to fix is not the
> state the device ends up in, but that it needs to be powered while
> registers are read or written.

I agree.

> It seems to me that that the current "resume" code should work in that
> respect, because it changes the PM state to "on" in uart_resume_port()
> before any access to hardware registers takes place, so there is
> nothing that needs to be fixed.

Ack.

> That may be different for the "suspend" part, though, because it
> assumes that the PM state is "on", and I think that is what the patch
> asserts to not be a valid assumption anymore. It's hard to tell if
> that is true, though, because I cannot reproduce the issue here; it
> just works either way...

So, do we agree then that *if* there needs to be a fix for that, it
should be in uart_suspend_port() by adding some 'uart_change_pm' shortly
before accessing the ops-callbacks? I am not super-fit with the uart
layer, but can't we assume because of the "Nothing to do if the console
is not suspending"-if-block that (for SCIF at least) it means the port
is on because it _is_ the console?

(I wonder, though, if the OMAPs won't need such a fix because they seem
to use runtime_autosuspend, so their state might be off actually?)

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]