linux-kernel - Re: [PATCH] serial: port: Don't suspend if the port is still busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b1518b0f-977a-58f5-3164-a6bb9a45dfbe@huawei.com>
Date: Mon, 5 Feb 2024 16:55:50 +0800
From: Yicong Yang <yangyicong@...wei.com>
To: Tony Lindgren <tony@...mide.com>
CC: <yangyicong@...ilicon.com>, <gregkh@...uxfoundation.org>,
	<jirislaby@...nel.org>, <linux-kernel@...r.kernel.org>,
	<linux-serial@...r.kernel.org>, <john.ogness@...utronix.de>,
	<andriy.shevchenko@...ux.intel.com>, <tglx@...utronix.de>,
	<linuxarm@...wei.com>, <prime.zeng@...ilicon.com>,
	<jonathan.cameron@...wei.com>, <fanghao11@...wei.com>
Subject: Re: [PATCH] serial: port: Don't suspend if the port is still busy

On 2024/2/5 14:51, Tony Lindgren wrote:
> * Yicong Yang <yangyicong@...wei.com> [240204 03:24]:
>> From: Yicong Yang <yangyicong@...ilicon.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.