Received-SPF: pass (google.com: domain of linux-kernel+bounces-52232-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
CC: <yangyicong@hisilicon.com>, <gregkh@linuxfoundation.org>,
	<jirislaby@kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-serial@vger.kernel.org>, <john.ogness@linutronix.de>,
	<andriy.shevchenko@linux.intel.com>, <tglx@linutronix.de>,
	<linuxarm@huawei.com>, <prime.zeng@hisilicon.com>,
	<jonathan.cameron@huawei.com>, <fanghao11@huawei.com>
Subject: Re: [PATCH] serial: port: Don't suspend if the port is still busy
To: Tony Lindgren <tony@atomide.com>
References: <20240204031957.58176-1-yangyicong@huawei.com>
 <20240205065126.GB5185@atomide.com>
From: Yicong Yang <yangyicong@huawei.com>
Message-ID: <b1518b0f-977a-58f5-3164-a6bb9a45dfbe@huawei.com>
Date: Mon, 5 Feb 2024 16:55:50 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.1
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240205065126.GB5185@atomide.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

On 2024/2/5 14:51, Tony Lindgren wrote:
> * Yicong Yang <yangyicong@huawei.com> [240204 03:24]:
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> We accidently met the issue that the bash prompt is not shown after the
>> previous command done and until the next input if there's only one CPU
>> (In our issue other CPUs are isolated by isolcpus=). Further analysis
>> shows it's because the port entering runtime suspend even if there's
>> still pending chars in the buffer and the pending chars will only be
>> processed in next device resuming. We are using amba-pl011 and the
>> problematic flow is like below:
>>
>> Bash                                         kworker
>> tty_write()
>>   file_tty_write()
>>     n_tty_write()
>>       uart_write()
>>         __uart_start()
>>           pm_runtime_get() // wakeup waker
>>             queue_work()
>>                                              pm_runtime_work()
>>                                                rpm_resume()
>>                                                 status = RPM_RESUMING
>>                                                 serial_port_runtime_resume()
>>                                                   port->ops->start_tx()
>>                                                     pl011_tx_chars()
>>                                                       uart_write_wakeup()
>>         […]
>>         __uart_start()
>>           pm_runtime_get() < 0 // because runtime status = RPM_RESUMING
>>                                // later data are not commit to the port driver
>>                                                 status = RPM_ACTIVE
>>                                                 rpm_idle() -> rpm_suspend()
> 
> Can you please confirm if this still happens also with commit 6f699743aebf
> ("serial: core: Fix runtime PM handling for pending tx")? It adds a check
> for -EINPROGRESS.

Tested nagetive on latest v6.8-rc3. Paste the current code snippet below in __uart_start():

	/* Increment the runtime PM usage count for the active check below */
	err = pm_runtime_get(&port_dev->dev);
	if (err < 0 && err != -EINPROGRESS) {
		pm_runtime_put_noidle(&port_dev->dev);
		return;
	}

	/*
	 * Start TX if enabled, and kick runtime PM. If the device is not
	 * enabled, serial_port_runtime_resume() calls start_tx() again
	 * after enabling the device.
	 */
	if (pm_runtime_active(&port_dev->dev))
		port->ops->start_tx(port);

In our issue case, the dev->power.runtime_status == RPM_RESUMING as analyzed in
commit. So we cannot pass the pm_runtime_active() check and the chars will still
be pending.

> 
>> This patch tries to fix this by checking the port busy before entering
>> runtime suspending. A runtime_suspend callback is added for the port
>> driver. When entering runtime suspend the callback is invoked, if there's
>> still pending chars in the buffer then request an runtime resume for
>> handling this.
> 
> OK. Let's look at this further after you have checked what happens with
> commit 6f699743aebf.
> 
> If needed, to me it seems that flushing tx and returning -EBUSY from
> serial_port_runtime_suspend() if busy might do the trick though.
> 

Do you mean something like below?

static int serial_port_runtime_suspend(struct device *dev)
{
	struct serial_port_device *port_dev = to_serial_base_port_device(dev);
	struct uart_port *port;
	unsigned long flags;
	int ret = 0;

	port = port_dev->port;

	if (port->flags & UPF_DEAD)
		return ret;

	spin_lock_irqsave(&port->lock, flags);
	if (__serial_port_busy(port)) {
		port->ops->start_tx(port);
		ret = -EBUSY;
	}
	spin_unlock_irqrestore(&port->lock, flags);

	return ret;
}

If so will the port fail to suspend after flushing the pending chars? Considering
underlay driver like amba-pl011 doesn't implement runtime power management, does
anyone will get the port into suspend routine later? I'm not quite sure about it.

In the patch's implementation the pending chars will be flushed in runtime_resume()
callback and rpm_resume() will try to call rpm_idle() later.

Thanks.