Re: [PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Tony Lindgren]

Hi,

* Johan Hovold <johan@xxxxxxxxxx> [211018 07:09]:
> On Fri, Oct 15, 2021 at 02:26:23PM +0300, Tony Lindgren wrote:
> > @@ -1067,6 +1116,11 @@ uart_tiocmset(struct tty_struct *tty, unsigned int set, unsigned int clear)
> >  	if (!uport)
> >  		goto out;
> >  
> > +	if (uart_port_wakeup(uport) < 0) {
> > +		ret = -EAGAIN;
> > +		goto out;
> > +	}
> 
> ...this isn't right. You should just resume the device synchronously
> here and not return some random error to user space, which it is
> unlikely to even handle.

OK I'll check what we can already wake synchronously :)

> Now this may require moving more of the runtime PM into serial core,
> where it should have been added in the first place, due to a lot of the
> serial callbacks being called with the port spin lock held.

Yup.. So the good news is that Andy already has the generic serial layer
runtime PM changes in his WIP tree. I'll take a look if we can already
add some of that without bringing in all the other dependencies.

> The current implementation is just broken. Take uart_dtr_rts(), for
> example, nothing makes sure that the device is active before accessing
> the modem control registers there. You're currently just relying on
> luck and pm_runtime_irq_safe() (which you are now trying to remove).

Yeah agreed, it's broken. It is usable for at least two limited cases
though, which are a serial port console with PM, and bluetooth with PM.

The serial port console typically only has RX and TX lines connected, and
the bluetooth typically uses out-of-band GPIO pins for wakeups.

To enable the serial port PM in general, we need to make sure it is
enabled only for applications where it can be used. So it needs to be
enabled from the user space as we do for the serial console, or enabled
from the consumer device driver for things like bluetooth.

Sure the TX should work in all other cases too..

> > +
> > +	if (uart_port_wakeup(uport) < 0)
> > +		goto out;
> > +
> >  	uart_port_dtr_rts(uport, raise);
> > +out:
> >  	uart_port_deref(uport);
> >  }
> 
> Heh, here you do try to do something about dtr_rts(), but you can't just
> ignore the request and wish for the best in case the device is
> suspended. :) There needs to be a synchronous resume here too.

Well for the current use cases the port should be already awake at
this point :) But yeah, for the TX path we should be able to handle
all the cases.

Regards,

Tony