Maximilian reported that surface_serial_hub serdev tx does not work during
system suspend. During system suspend, runtime PM gets disabled in
__device_suspend_late(), and tx is unable to wake-up the serial core port
device that we use to check if tx is safe to start. Johan summarized the
regression noting that serdev tx no longer always works as earlier when the
serdev device is runtime PM active.

The serdev device and the serial core controller devices are siblings of
the serial port hardware device. The runtime PM usage count from serdev
device does not propagate to the serial core device siblings, it only
propagates to the parent.

In addition to the tx issue for suspend, testing for the serial core port
device can cause an unnecessary delay in enabling tx while waiting for the
serial core port device to wake-up. The serial core port device wake-up is
only needed to flush pending tx when the serial port hardware device was
in runtime PM suspended state.

To fix the regression, we need to check the runtime PM state of the parent
serial port hardware device for tx instead of the serial core port device.

As the serial port device drivers may or may not implement runtime PM, we
need to also add a check for pm_runtime_enabled().

Reported-by: Maximilian Luz <luzmaximilian@gmail.com>
Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/serial_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -156,7 +156,7 @@ static void __uart_start(struct uart_state *state)
 	 * enabled, serial_port_runtime_resume() calls start_tx() again
 	 * after enabling the device.
 	 */
-	if (pm_runtime_active(&port_dev->dev))
+	if (!pm_runtime_enabled(port->dev) || pm_runtime_active(port->dev))
 		port->ops->start_tx(port);
 	pm_runtime_mark_last_busy(&port_dev->dev);
 	pm_runtime_put_autosuspend(&port_dev->dev);
-- 
2.42.0

On 10/5/23 09:56, Tony Lindgren wrote:
> Maximilian reported that surface_serial_hub serdev tx does not work during
> system suspend. During system suspend, runtime PM gets disabled in
> __device_suspend_late(), and tx is unable to wake-up the serial core port
> device that we use to check if tx is safe to start. Johan summarized the
> regression noting that serdev tx no longer always works as earlier when the
> serdev device is runtime PM active.
> 
> The serdev device and the serial core controller devices are siblings of
> the serial port hardware device. The runtime PM usage count from serdev
> device does not propagate to the serial core device siblings, it only
> propagates to the parent.
> 
> In addition to the tx issue for suspend, testing for the serial core port
> device can cause an unnecessary delay in enabling tx while waiting for the
> serial core port device to wake-up. The serial core port device wake-up is
> only needed to flush pending tx when the serial port hardware device was
> in runtime PM suspended state.
> 
> To fix the regression, we need to check the runtime PM state of the parent
> serial port hardware device for tx instead of the serial core port device.
> 
> As the serial port device drivers may or may not implement runtime PM, we
> need to also add a check for pm_runtime_enabled().
> 
> Reported-by: Maximilian Luz <luzmaximilian@gmail.com>
> Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Thanks!

Tested-by: Maximilian Luz <luzmaximilian@gmail.com>

> ---
>   drivers/tty/serial/serial_core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -156,7 +156,7 @@ static void __uart_start(struct uart_state *state)
>   	 * enabled, serial_port_runtime_resume() calls start_tx() again
>   	 * after enabling the device.
>   	 */
> -	if (pm_runtime_active(&port_dev->dev))
> +	if (!pm_runtime_enabled(port->dev) || pm_runtime_active(port->dev))
>   		port->ops->start_tx(port);
>   	pm_runtime_mark_last_busy(&port_dev->dev);
>   	pm_runtime_put_autosuspend(&port_dev->dev);

On Thu, Oct 05, 2023 at 10:56:42AM +0300, Tony Lindgren wrote:
> Maximilian reported that surface_serial_hub serdev tx does not work during
> system suspend. During system suspend, runtime PM gets disabled in
> __device_suspend_late(), and tx is unable to wake-up the serial core port
> device that we use to check if tx is safe to start. Johan summarized the
> regression noting that serdev tx no longer always works as earlier when the
> serdev device is runtime PM active.
> 
> The serdev device and the serial core controller devices are siblings of
> the serial port hardware device. The runtime PM usage count from serdev

I'm a bit lost in terminology here.
AFAIU there are:
1) children of the serial physical device;
2) siblings (to each other).

But may be I mistakenly deciphered the diagram from the previous discussion.

> device does not propagate to the serial core device siblings, it only
> propagates to the parent.
> 
> In addition to the tx issue for suspend, testing for the serial core port
> device can cause an unnecessary delay in enabling tx while waiting for the
> serial core port device to wake-up. The serial core port device wake-up is
> only needed to flush pending tx when the serial port hardware device was
> in runtime PM suspended state.
> 
> To fix the regression, we need to check the runtime PM state of the parent
> serial port hardware device for tx instead of the serial core port device.
> 
> As the serial port device drivers may or may not implement runtime PM, we
> need to also add a check for pm_runtime_enabled().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [231005 12:01]:
> On Thu, Oct 05, 2023 at 10:56:42AM +0300, Tony Lindgren wrote:
> > The serdev device and the serial core controller devices are siblings of
> > the serial port hardware device. The runtime PM usage count from serdev
> 
> I'm a bit lost in terminology here.
> AFAIU there are:
> 1) children of the serial physical device;
> 2) siblings (to each other).
> 
> But may be I mistakenly deciphered the diagram from the previous discussion.

You're right, so how about:

The serdev device and the serial core controller devices are children of
the serial port hardware device. The runtime PM usage count from serdev
device does not propagate to the serial core device siblings, it only
propagates to the parent.

Regards,

Tony

On Fri, Oct 06, 2023 at 10:27:38AM +0300, Tony Lindgren wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [231005 12:01]:
> > On Thu, Oct 05, 2023 at 10:56:42AM +0300, Tony Lindgren wrote:
> > > The serdev device and the serial core controller devices are siblings of
> > > the serial port hardware device. The runtime PM usage count from serdev
> > 
> > I'm a bit lost in terminology here.
> > AFAIU there are:
> > 1) children of the serial physical device;
> > 2) siblings (to each other).
> > 
> > But may be I mistakenly deciphered the diagram from the previous discussion.
> 
> You're right, so how about:
> 
> The serdev device and the serial core controller devices are children of
> the serial port hardware device. The runtime PM usage count from serdev
> device does not propagate to the serial core device siblings, it only
> propagates to the parent.

Clearer, thank you!

-- 
With Best Regards,
Andy Shevchenko

On Fri, Oct 06, 2023 at 10:27:38AM +0300, Tony Lindgren wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [231005 12:01]:
> > On Thu, Oct 05, 2023 at 10:56:42AM +0300, Tony Lindgren wrote:
> > > The serdev device and the serial core controller devices are siblings of
> > > the serial port hardware device. The runtime PM usage count from serdev
> > 
> > I'm a bit lost in terminology here.
> > AFAIU there are:
> > 1) children of the serial physical device;
> > 2) siblings (to each other).
> > 
> > But may be I mistakenly deciphered the diagram from the previous discussion.
> 
> You're right, so how about:
> 
> The serdev device and the serial core controller devices are children of
> the serial port hardware device. The runtime PM usage count from serdev
> device does not propagate to the serial core device siblings, it only
> propagates to the parent.

That's still not accurate:

 - the serdev device is not a child (but a grandchild) of the serial
   controller
 - the new serial port devices are not "siblings" (but descendants) of
   the serial controller
 - the serdev controller ignores the power state of its children so that
   bit is also incorrect

You just want to describe the fact that the serdev controller runtime PM
state is currently not propagated to your new "devices" that are
descendants to the serial controller.

I'm still not sure why it was implemented this way, or if it is even
correct, but this seems to be the state of things.

Johan

* Johan Hovold <johan@kernel.org> [231006 08:03]:
> On Fri, Oct 06, 2023 at 10:27:38AM +0300, Tony Lindgren wrote:
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [231005 12:01]:
> > > On Thu, Oct 05, 2023 at 10:56:42AM +0300, Tony Lindgren wrote:
> > > > The serdev device and the serial core controller devices are siblings of
> > > > the serial port hardware device. The runtime PM usage count from serdev
> > > 
> > > I'm a bit lost in terminology here.
> > > AFAIU there are:
> > > 1) children of the serial physical device;
> > > 2) siblings (to each other).
> > > 
> > > But may be I mistakenly deciphered the diagram from the previous discussion.
> > 
> > You're right, so how about:
> > 
> > The serdev device and the serial core controller devices are children of
> > the serial port hardware device. The runtime PM usage count from serdev
> > device does not propagate to the serial core device siblings, it only
> > propagates to the parent.
> 
> That's still not accurate:
> 
>  - the serdev device is not a child (but a grandchild) of the serial
>    controller
>  - the new serial port devices are not "siblings" (but descendants) of
>    the serial controller
>  - the serdev controller ignores the power state of its children so that
>    bit is also incorrect
> 
> You just want to describe the fact that the serdev controller runtime PM
> state is currently not propagated to your new "devices" that are
> descendants to the serial controller.

OK so let's just use:

The serdev controller runtime PM state is not currently propagated
to the serial core controller port device. The runtime PM usage count
only propagates to the parent device.

> I'm still not sure why it was implemented this way, or if it is even
> correct, but this seems to be the state of things.

Care to clarify a bit which parts are unclear? The hierarchy of port
devices, making serial core manage runtime PM in a generic way, or
flushing tx?

Regards,

Tony

On Fri, Oct 06, 2023 at 11:37:12AM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [231006 08:03]:
> > On Fri, Oct 06, 2023 at 10:27:38AM +0300, Tony Lindgren wrote:

> > > You're right, so how about:
> > > 
> > > The serdev device and the serial core controller devices are children of
> > > the serial port hardware device. The runtime PM usage count from serdev
> > > device does not propagate to the serial core device siblings, it only
> > > propagates to the parent.
> > 
> > That's still not accurate:
> > 
> >  - the serdev device is not a child (but a grandchild) of the serial
> >    controller
> >  - the new serial port devices are not "siblings" (but descendants) of
> >    the serial controller
> >  - the serdev controller ignores the power state of its children so that
> >    bit is also incorrect
> > 
> > You just want to describe the fact that the serdev controller runtime PM
> > state is currently not propagated to your new "devices" that are
> > descendants to the serial controller.
> 
> OK so let's just use:
> 
> The serdev controller runtime PM state is not currently propagated
> to the serial core controller port device. The runtime PM usage count
> only propagates to the parent device.

That sounds better.

> > I'm still not sure why it was implemented this way, or if it is even
> > correct, but this seems to be the state of things.
> 
> Care to clarify a bit which parts are unclear? The hierarchy of port
> devices, making serial core manage runtime PM in a generic way, or
> flushing tx?

I still don't know why you added these two new abstractions (controller
and port), and that isn't really explained by the commit message either.
And if these are indeed needed, then why isn't the serdev controller now
a child of the "port" device, for example?

There are just a lot of questions and I worry that there are more
problems lurking, but unfortunately I still don't have time to review
this.

Johan

* Johan Hovold <johan@kernel.org> [231006 15:37]:
> On Fri, Oct 06, 2023 at 11:37:12AM +0300, Tony Lindgren wrote:
> > OK so let's just use:
> > 
> > The serdev controller runtime PM state is not currently propagated
> > to the serial core controller port device. The runtime PM usage count
> > only propagates to the parent device.
> 
> That sounds better.

OK

> > > I'm still not sure why it was implemented this way, or if it is even
> > > correct, but this seems to be the state of things.
> > 
> > Care to clarify a bit which parts are unclear? The hierarchy of port
> > devices, making serial core manage runtime PM in a generic way, or
> > flushing tx?
> 
> I still don't know why you added these two new abstractions (controller
> and port), and that isn't really explained by the commit message either.

We want serial core to do runtime PM in a generic way and have the usage
count propagate to the parent serial port hardware device. This way we
don't need to care much if the numerous serial port drivers implement
runtime PM or not. Well, except for now we need to check the parent state
for this fix :)

We also want serial core to know the serial port to serial port hardware
mapping as we already have multiport devices. The serial core controller
is there to group the serial ports for each serial port hardware device.
We at least now have an option to support devices with multiple controllers
and ports in case we ever happen to see such things.

> And if these are indeed needed, then why isn't the serdev controller now
> a child of the "port" device, for example?

Yes I agree we should now move serdev controller to be a child of the
serial core port device. Then this $subject patch can be reverted.

Moving serdev controller should also help serdev to deal with multiport
devices I think?

In the long run serial port specific functions could live there too.

> There are just a lot of questions and I worry that there are more
> problems lurking, but unfortunately I still don't have time to review
> this.

Well incremental steps should be easier to do now.

Regards,

Tony

On Sat, Oct 07, 2023 at 08:45:41AM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [231006 15:37]:
> > On Fri, Oct 06, 2023 at 11:37:12AM +0300, Tony Lindgren wrote:

> > > Care to clarify a bit which parts are unclear? The hierarchy of port
> > > devices, making serial core manage runtime PM in a generic way, or
> > > flushing tx?
> > 
> > I still don't know why you added these two new abstractions (controller
> > and port), and that isn't really explained by the commit message either.
> 
> We want serial core to do runtime PM in a generic way and have the usage
> count propagate to the parent serial port hardware device. This way we
> don't need to care much if the numerous serial port drivers implement
> runtime PM or not. Well, except for now we need to check the parent state
> for this fix :)

That sounds like a lot of complexity to avoid checking if (the single
instance of) pm_runtime_get() returns -EACCESS.
 
> We also want serial core to know the serial port to serial port hardware
> mapping as we already have multiport devices. The serial core controller
> is there to group the serial ports for each serial port hardware device.
> We at least now have an option to support devices with multiple controllers
> and ports in case we ever happen to see such things.

Hypothetical multiple serial controllers should be modelled as separate
controllers, but yeah, perhaps we want to describe the ports.
 
> > And if these are indeed needed, then why isn't the serdev controller now
> > a child of the "port" device, for example?
> 
> Yes I agree we should now move serdev controller to be a child of the
> serial core port device. Then this $subject patch can be reverted.
> 
> Moving serdev controller should also help serdev to deal with multiport
> devices I think?

It wouldn't help currently I think, since we already resume the
controller and don't manage ports individually, but if we now have port
devices then it probably should be moved.

Johan

* Johan Hovold <johan@kernel.org> [231016 15:18]:
> On Sat, Oct 07, 2023 at 08:45:41AM +0300, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [231006 15:37]:
> > > On Fri, Oct 06, 2023 at 11:37:12AM +0300, Tony Lindgren wrote:
> 
> > > > Care to clarify a bit which parts are unclear? The hierarchy of port
> > > > devices, making serial core manage runtime PM in a generic way, or
> > > > flushing tx?
> > > 
> > > I still don't know why you added these two new abstractions (controller
> > > and port), and that isn't really explained by the commit message either.
> > 
> > We want serial core to do runtime PM in a generic way and have the usage
> > count propagate to the parent serial port hardware device. This way we
> > don't need to care much if the numerous serial port drivers implement
> > runtime PM or not. Well, except for now we need to check the parent state
> > for this fix :)
> 
> That sounds like a lot of complexity to avoid checking if (the single
> instance of) pm_runtime_get() returns -EACCESS.

Yes only one call so far. but we have the serial core generic PM patch(es)
from Andy and Ilpo that are still coming.

> > We also want serial core to know the serial port to serial port hardware
> > mapping as we already have multiport devices. The serial core controller
> > is there to group the serial ports for each serial port hardware device.
> > We at least now have an option to support devices with multiple controllers
> > and ports in case we ever happen to see such things.
> 
> Hypothetical multiple serial controllers should be modelled as separate
> controllers, but yeah, perhaps we want to describe the ports.

Yes and we already have multiport controllers.

> > > And if these are indeed needed, then why isn't the serdev controller now
> > > a child of the "port" device, for example?
> > 
> > Yes I agree we should now move serdev controller to be a child of the
> > serial core port device. Then this $subject patch can be reverted.
> > 
> > Moving serdev controller should also help serdev to deal with multiport
> > devices I think?
> 
> It wouldn't help currently I think, since we already resume the
> controller and don't manage ports individually, but if we now have port
> devices then it probably should be moved.

I'll post a patch for that after some more checks.

Regards,

Tony