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

[Andy Shevchenko]

On Wed, Feb 07, 2024 at 03:22:17PM +0800, Yicong Yang wrote:
> On 2024/2/6 21:09, Andy Shevchenko wrote:
> > On Tue, Feb 06, 2024 at 03:33:22PM +0800, Yicong Yang wrote:

..

> >> +		pm_runtime_mark_last_busy(dev);
> > 
> > Do you think we need to call this under a lock?
> 
> I just put this close to the ops->start_tx() where I used the device. Yes I have no
> strong reason to put it in/with the lock region, but pm_runtime_mark_last_busy()
> should be no costy and safe enough to put it in the spinlock region.
> 
> Any thoughts?

As I mentioned before, moving it out makes it similar to the resume
counterpart implementation.

..

> > With the above I would rather write it as
> > 
> > static int __serial_port_busy(struct uart_port *port)
> > {
> > 	if (uart_tx_stopped(port))
> > 		return 0;
> > 
> > 	if (uart_circ_chars_pending(&port->state->xmit)
> > 		return -EBUSY;
> 
> I'm not sure but EBUSY seems not quite match here. EBUSY for
> "Device or resource busy" so the device probably cannot be used
> but we're testing whether the port is busy here. Hope I understand it
> correctly.

Port is also "device" in the broader meaning. I don't see how this is
problematic. Prototype is originally int (while returning boolean).
I assume it was an idea behind similar (if not the same) as mine at
some point, but then vanished. Yet, the function itself can be renamed
to reflect these changes, like
__serial_port_get_status() // 0 - idling, -EBUSY - busy

> > 	return 0;
> > }
> > 
> > static int serial_port_runtime_suspend(struct device *dev)
> > {
> > 	int ret;
> > 	...
> > 	uart_port_lock_irqsave(port, &flags);
> > 	ret = __serial_port_busy(port);
> > 	if (ret)
> > 		port->ops->start_tx(port);
> > 	uart_port_unlock_irqrestore(port, flags);
> > 
> > 	if (ret)
> > 		pm_runtime_mark_last_busy(dev);
> > 
> > 	return ret;
> > }
> > 
> > It also seems aligned with the resume implementation above.
> > 
> > ...
> > 
> > For the consistency's sake the resume can be refactored as
> > 
> > static int serial_port_runtime_resume(struct device *dev)
> > {
> > 	...
> > 	int ret;
> > 	...
> > 	ret = __serial_port_busy(port);
> > 	if (ret)
> > 	...
> > }
> > 
> > but this can be done later.
> > 
> 
> I agree the refactoring should go to a separate patch. But it doesn't seem
> to be more simpler or readable comparing to the current implementation? Just
> want to narrowing the spinlock region?

Yes, at bare minimum I would expect the PM call be moved out of a lock.

As this seems a fix (and hence subject to backport) I would also minimize
invasion.

-- 
With Best Regards,
Andy Shevchenko