Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <62d3678a-a23d-4619-95de-145026629ba8@gmail.com>
Date:   Tue, 3 Oct 2023 13:57:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v12 1/1] serial: core: Start managing serial controllers
 to enable runtime PM
To:     Tony Lindgren <tony@atomide.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>
Cc:     Andy Shevchenko <andriy.shevchenko@intel.com>,
        Dhruva Gole <d-gole@ti.com>,
        =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        John Ogness <john.ogness@linutronix.de>,
        Johan Hovold <johan@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-omap@vger.kernel.org,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
References: <20230525113034.46880-1-tony@atomide.com>
Content-Language: en-US
From:   Maximilian Luz <luzmaximilian@gmail.com>
In-Reply-To: <20230525113034.46880-1-tony@atomide.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/25/23 13:30, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
> 
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
> 
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
> 
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.

Hi,

Unfortunately, this patch (now commit 84a9582fd203 with v6.5) breaks suspend on
a bunch of Microsoft Surface devices (confirmed via git bisect).

The root cause is that when we enter system suspend with the serial port in
runtime suspend, all transfers will be paused until the serial port is
runtime-resumed, which will only happen after complete() is called, so
basically after we are done resuming the system. In short: This patch
essentially blocks all serial communication in system suspend transitions.

The affected devices have an EC (the Surface Aggregator Module) which needs
some communication when entering system suspend. In particular, we need to tell
it to stop sending us events, turn off the keyboard backlight, and transition
it to a lower power mode. With this patch, all of these operations now time
out, preventing us from entering suspend.

A bad workaround is to disable runtime PM, e.g. via

   echo on > /sys/bus/serial-base/devices/dw-apb-uart.4:0/dw-apb-uart.4:0.0/power/control

or the diff attached below, but obviously that's not a great solution and can
be broken quite easily from userspace in the same way (and without users really
actively doing so through tools like TLP).

Any ideas on how this can be fixed without reverting?

See also https://github.com/linux-surface/linux-surface/issues/1258.

Regards,
Max

---
  drivers/tty/serial/serial_port.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
index 862423237007..6ceacea5e790 100644
--- a/drivers/tty/serial/serial_port.c
+++ b/drivers/tty/serial/serial_port.c
@@ -55,6 +55,8 @@ static int serial_port_probe(struct device *dev)
  	pm_runtime_set_autosuspend_delay(dev, SERIAL_PORT_AUTOSUSPEND_DELAY_MS);
  	pm_runtime_use_autosuspend(dev);
  
+	pm_runtime_forbid(dev);
+
  	return 0;
  }
  
-- 
2.42.0