[PATCH 0/6] Get rid of pm_runtime_irq_safe() for 8250_omap

[PATCH 0/6] Get rid of pm_runtime_irq_safe() for 8250_omap

[Tony Lindgren]

Hi,

Here are patches to get rid of pm_runtime_irq_safe() for the 8250_omap
driver.

For removing the pm_runtime_irq_safe() usage, serial TX is the last
remaining issue. We deal with TX by waking up the port and returning 0
bytes written from write_room() and write() if the port is not available
because of PM runtime autoidle.

This series also removes the dependency to Andy's pending generic serial
layer PM runtime patches, and hopefully makes that work a bit easier :)

Regards,

Tony


Tony Lindgren (6):
  n_tty: Start making use of -EAGAIN returned from
    process_output_block()
  tty: n_gsm: Don't ignore write return value in gsmld_output()
  serial: core: Add new prep_tx for power management
  serial: 8250: Implement prep_tx for power management
  serial: 8250_omap: Require a valid wakeirq for deeper idle states
  serial: 8250_omap: Drop the use of pm_runtime_irq_safe()

 Documentation/driver-api/serial/driver.rst |  9 ++++++
 drivers/tty/n_gsm.c                        |  5 ++-
 drivers/tty/n_tty.c                        |  8 +++--
 drivers/tty/serial/8250/8250_omap.c        | 36 +++++++++++++++-------
 drivers/tty/serial/8250/8250_port.c        | 24 +++++++++++++++
 drivers/tty/serial/serial_core.c           | 23 ++++++++++++++
 include/linux/serial_core.h                |  1 +
 7 files changed, 90 insertions(+), 16 deletions(-)

-- 
2.33.0

[PATCH 1/6] n_tty: Start making use of -EAGAIN returned from process_output_block()

[Tony Lindgren]

We check for -EAGAIN in n_tty_write() but never currently get it from
process_output_block(). Let's add -EAGAIN handling and break out with 0
bytes processed. Note that if we return -EAGAIN from n_tty_write(), it
will be treated as error by the caller rather than a retry.

Looking at the patch description for commit 9ef8927f45f2 ("n_tty: check
for negative and zero space return from tty_write_room") it looks like we
have not made use of -EGAIN from process_output_block() so far, so this
does not seem like it's currently needed as a fix.

We can use -EAGAIN for serial layer power management changes as we now
will make use of write_room() returning 0 for an idled serial port.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/n_tty.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -549,6 +549,8 @@ static ssize_t process_output_block(struct tty_struct *tty,
 	space = tty_write_room(tty);
 	if (space <= 0) {
 		mutex_unlock(&ldata->output_lock);
+		if (!space)
+			space = -EAGAIN;
 		return space;
 	}
 	if (nr > space)
@@ -2287,8 +2289,10 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 			while (nr > 0) {
 				ssize_t num = process_output_block(tty, b, nr);
 				if (num < 0) {
-					if (num == -EAGAIN)
-						break;
+					if (num == -EAGAIN) {
+						retval = 0;
+						goto break_out;
+					}
 					retval = num;
 					goto break_out;
 				}
-- 
2.33.0

Re: [PATCH 1/6] n_tty: Start making use of -EAGAIN returned from process_output_block()

[Andy Shevchenko]

On Tue, Sep 21, 2021 at 1:34 PM Tony Lindgren <tony@atomide.com> wrote:
>
> We check for -EAGAIN in n_tty_write() but never currently get it from
> process_output_block(). Let's add -EAGAIN handling and break out with 0
> bytes processed. Note that if we return -EAGAIN from n_tty_write(), it
> will be treated as error by the caller rather than a retry.
>
> Looking at the patch description for commit 9ef8927f45f2 ("n_tty: check
> for negative and zero space return from tty_write_room") it looks like we
> have not made use of -EGAIN from process_output_block() so far, so this

-EGAIN?

> does not seem like it's currently needed as a fix.
>
> We can use -EAGAIN for serial layer power management changes as we now
> will make use of write_room() returning 0 for an idled serial port.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/n_tty.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -549,6 +549,8 @@ static ssize_t process_output_block(struct tty_struct *tty,
>         space = tty_write_room(tty);
>         if (space <= 0) {
>                 mutex_unlock(&ldata->output_lock);
> +               if (!space)
> +                       space = -EAGAIN;
>                 return space;
>         }
>         if (nr > space)
> @@ -2287,8 +2289,10 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
>                         while (nr > 0) {
>                                 ssize_t num = process_output_block(tty, b, nr);
>                                 if (num < 0) {
> -                                       if (num == -EAGAIN)
> -                                               break;
> +                                       if (num == -EAGAIN) {
> +                                               retval = 0;
> +                                               goto break_out;
> +                                       }
>                                         retval = num;
>                                         goto break_out;
>                                 }
> --
> 2.33.0



-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/6] tty: n_gsm: Don't ignore write return value in gsmld_output()

[Tony Lindgren]

We currently have gsmld_output() ignore the return value from device
write. This means we will lose packets if device write returns 0 or
an error.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/n_gsm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -687,7 +687,7 @@ static void gsm_data_kick(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 			print_hex_dump_bytes("gsm_data_kick: ",
 					     DUMP_PREFIX_OFFSET,
 					     gsm->txframe, len);
-		if (gsmld_output(gsm, gsm->txframe, len) < 0)
+		if (gsmld_output(gsm, gsm->txframe, len) <= 0)
 			break;
 		/* FIXME: Can eliminate one SOF in many more cases */
 		gsm->tx_bytes -= msg->len;
@@ -2358,8 +2358,7 @@ static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len)
 	if (debug & 4)
 		print_hex_dump_bytes("gsmld_output: ", DUMP_PREFIX_OFFSET,
 				     data, len);
-	gsm->tty->ops->write(gsm->tty, data, len);
-	return len;
+	return gsm->tty->ops->write(gsm->tty, data, len);
 }
 
 /**
-- 
2.33.0

[PATCH 3/6] serial: core: Add new prep_tx for power management

[Tony Lindgren]

If the serial driver implements PM runtime with autosuspend, the port may
be powered off for TX. To wake up the port, let's add new prep_tx() call
for serial drivers to implement as needed. We call it from serial
write_room() and write() functions. If the serial port is not enabled,
we just return 0.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/driver-api/serial/driver.rst |  9 +++++++++
 drivers/tty/serial/serial_core.c           | 23 ++++++++++++++++++++++
 include/linux/serial_core.h                |  1 +
 3 files changed, 33 insertions(+)

diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst
--- a/Documentation/driver-api/serial/driver.rst
+++ b/Documentation/driver-api/serial/driver.rst
@@ -136,6 +136,15 @@ hardware.
 
 	This call must not sleep
 
+  prep_tx(port)
+	Prepare port for transmitting characters.
+
+	Locking: port->lock taken.
+
+	Interrupts: locally disabled.
+
+	This call must not sleep
+
   start_tx(port)
 	Start transmitting characters.
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -118,6 +118,17 @@ static void uart_stop(struct tty_struct *tty)
 	uart_port_unlock(port, flags);
 }
 
+static int __uart_prep_tx(struct tty_struct *tty)
+{
+	struct uart_state *state = tty->driver_data;
+	struct uart_port *port = state->uart_port;
+
+	if (port && !uart_tx_stopped(port) && port->ops->prep_tx)
+		return port->ops->prep_tx(port);
+
+	return 0;
+}
+
 static void __uart_start(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
@@ -574,6 +585,12 @@ static int uart_write(struct tty_struct *tty,
 		return 0;
 	}
 
+	ret = __uart_prep_tx(tty);
+	if (ret < 0) {
+		uart_port_unlock(port, flags);
+		return 0;
+	}
+
 	while (port) {
 		c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
 		if (count < c)
@@ -600,6 +617,12 @@ static unsigned int uart_write_room(struct tty_struct *tty)
 	unsigned int ret;
 
 	port = uart_port_lock(state, flags);
+	ret = __uart_prep_tx(tty);
+	if (ret < 0) {
+		uart_port_unlock(port, flags);
+		return 0;
+	}
+
 	ret = uart_circ_chars_free(&state->xmit);
 	uart_port_unlock(port, flags);
 	return ret;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -40,6 +40,7 @@ struct uart_ops {
 	void		(*set_mctrl)(struct uart_port *, unsigned int mctrl);
 	unsigned int	(*get_mctrl)(struct uart_port *);
 	void		(*stop_tx)(struct uart_port *);
+	int		(*prep_tx)(struct uart_port *);
 	void		(*start_tx)(struct uart_port *);
 	void		(*throttle)(struct uart_port *);
 	void		(*unthrottle)(struct uart_port *);
-- 
2.33.0

Re: [PATCH 3/6] serial: core: Add new prep_tx for power management

[Johan Hovold]

On Tue, Sep 21, 2021 at 01:33:43PM +0300, Tony Lindgren wrote:
> If the serial driver implements PM runtime with autosuspend, the port may
> be powered off for TX. To wake up the port, let's add new prep_tx() call
> for serial drivers to implement as needed. We call it from serial
> write_room() and write() functions. If the serial port is not enabled,
> we just return 0.

This isn't right. If there's room in the driver buffer, there's no
reason to not accept those characters.

It's the drivers responsibility to resume writing when write() is
called and that me need to be done in a runtime resume callback in case
the device is suspended.

No need to be patching line disciplines for this.

Johan

Re: [PATCH 3/6] serial: core: Add new prep_tx for power management

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [210923 12:46]:
> On Tue, Sep 21, 2021 at 01:33:43PM +0300, Tony Lindgren wrote:
> > If the serial driver implements PM runtime with autosuspend, the port may
> > be powered off for TX. To wake up the port, let's add new prep_tx() call
> > for serial drivers to implement as needed. We call it from serial
> > write_room() and write() functions. If the serial port is not enabled,
> > we just return 0.
> 
> This isn't right. If there's room in the driver buffer, there's no
> reason to not accept those characters.

Maybe. We might get away with returning zero bytes written in write().
But to me it seems better to stop things early when write is known
to not succeed.

> It's the drivers responsibility to resume writing when write() is
> called and that me need to be done in a runtime resume callback in case
> the device is suspended.

I think we currently need to return zero bytes written from write()
when the serial port is not usable.

I don't think we can return a fake number of bytes written from write().

And even if we could return a fake number of bytes written, we could
run into issues doing the real write to the serial port.

> No need to be patching line disciplines for this.

Do you see issues with handling the errors in line disciplines?

Regards,

Tony

Re: [PATCH 3/6] serial: core: Add new prep_tx for power management

[Johan Hovold]

On Thu, Sep 23, 2021 at 06:02:27PM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [210923 12:46]:
> > On Tue, Sep 21, 2021 at 01:33:43PM +0300, Tony Lindgren wrote:
> > > If the serial driver implements PM runtime with autosuspend, the port may
> > > be powered off for TX. To wake up the port, let's add new prep_tx() call
> > > for serial drivers to implement as needed. We call it from serial
> > > write_room() and write() functions. If the serial port is not enabled,
> > > we just return 0.
> > 
> > This isn't right. If there's room in the driver buffer, there's no
> > reason to not accept those characters.
> 
> Maybe. We might get away with returning zero bytes written in write().
> But to me it seems better to stop things early when write is known
> to not succeed.

But you shouldn't return zero from write() either. If there's room in
the write buffer we accept the data.
 
> > It's the drivers responsibility to resume writing when write() is
> > called and that me need to be done in a runtime resume callback in case
> > the device is suspended.
> 
> I think we currently need to return zero bytes written from write()
> when the serial port is not usable.
> 
> I don't think we can return a fake number of bytes written from write().

It's not a fake number. It's similar to if you have a port that is
stalled due to flow control. We buffer the data and continue writing
when the other end is ready to accept more.

> And even if we could return a fake number of bytes written, we could
> run into issues doing the real write to the serial port.
>
> > No need to be patching line disciplines for this.
> 
> Do you see issues with handling the errors in line disciplines?

It's just conceptually wrong to push retrying up the stack, possible all
the way to user space in case of non-blocking opens, just because the
device isn't already runtime active.

Johan

Re: [PATCH 3/6] serial: core: Add new prep_tx for power management

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [210924 14:38]:
> On Thu, Sep 23, 2021 at 06:02:27PM +0300, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [210923 12:46]:
> > > On Tue, Sep 21, 2021 at 01:33:43PM +0300, Tony Lindgren wrote:
> > > > If the serial driver implements PM runtime with autosuspend, the port may
> > > > be powered off for TX. To wake up the port, let's add new prep_tx() call
> > > > for serial drivers to implement as needed. We call it from serial
> > > > write_room() and write() functions. If the serial port is not enabled,
> > > > we just return 0.
> > > 
> > > This isn't right. If there's room in the driver buffer, there's no
> > > reason to not accept those characters.
> > 
> > Maybe. We might get away with returning zero bytes written in write().
> > But to me it seems better to stop things early when write is known
> > to not succeed.
> 
> But you shouldn't return zero from write() either. If there's room in
> the write buffer we accept the data.

And then waking up the serial port takes several tens of ms and the
buffer is full and we still need to deal with it :) But yeah I see
your point for the write buffer.

> > > It's the drivers responsibility to resume writing when write() is
> > > called and that me need to be done in a runtime resume callback in case
> > > the device is suspended.
> > 
> > I think we currently need to return zero bytes written from write()
> > when the serial port is not usable.
> > 
> > I don't think we can return a fake number of bytes written from write().
> 
> It's not a fake number. It's similar to if you have a port that is
> stalled due to flow control. We buffer the data and continue writing
> when the other end is ready to accept more.

OK. So based on what you suggested earlier I'll take a look at moving
the wake-up to __uart_start(), then have the device driver runtime PM
resume call uart_start() again. Looks like uart_start() is a void
function anyways.. If you have some better ideas there, please let me
know.

> > > No need to be patching line disciplines for this.
> > 
> > Do you see issues with handling the errors in line disciplines?
> 
> It's just conceptually wrong to push retrying up the stack, possible all
> the way to user space in case of non-blocking opens, just because the
> device isn't already runtime active.

Yes, I don't see a way around that currently. Maybe if we start making
use of uart_tx_stopped() or something similar that could be simplified.
And we'll be still hit these line discipline error handling cases
anyways depending on how long the serial port wake up takes.

Regards,

Tony

Re: [PATCH 3/6] serial: core: Add new prep_tx for power management

[Johan Hovold]

On Fri, Sep 24, 2021 at 06:09:18PM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [210924 14:38]:
> > On Thu, Sep 23, 2021 at 06:02:27PM +0300, Tony Lindgren wrote:

> > > > No need to be patching line disciplines for this.
> > > 
> > > Do you see issues with handling the errors in line disciplines?
> > 
> > It's just conceptually wrong to push retrying up the stack, possible all
> > the way to user space in case of non-blocking opens, just because the
> > device isn't already runtime active.
> 
> Yes, I don't see a way around that currently. Maybe if we start making
> use of uart_tx_stopped() or something similar that could be simplified.
> And we'll be still hit these line discipline error handling cases
> anyways depending on how long the serial port wake up takes.

I didn't really look at the ldisc change so not saying it isn't needed
for other reasons such as a full write buffer. But then I'd expect it to
be presented as a bug fix (perhaps it was).

Johan

[PATCH 4/6] serial: 8250: Implement prep_tx for power management

[Tony Lindgren]

We can use the prep_tx() call to wake up an idle serial port. This allows
ust to remove the depedency to pm_runtime_irq_safe() for 8250_omap driver
in the following patches.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_port.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1650,6 +1650,29 @@ static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t)
 	return HRTIMER_NORESTART;
 }
 
+static int serial8250_prep_tx(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	struct device *dev = up->port.dev;
+	int err;
+
+	if (!(up->capabilities & UART_CAP_RPM))
+		return 0;
+
+	if (!pm_runtime_suspended(dev)) {
+		pm_runtime_mark_last_busy(dev);
+		return 0;
+	}
+
+	err = pm_request_resume(dev);
+	if (err < 0) {
+		dev_warn(dev, "prep_tx wakeup failed: %d\n", err);
+		return err;
+	}
+
+	return -EINPROGRESS;
+}
+
 static void serial8250_start_tx(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
@@ -3227,6 +3250,7 @@ static const struct uart_ops serial8250_pops = {
 	.set_mctrl	= serial8250_set_mctrl,
 	.get_mctrl	= serial8250_get_mctrl,
 	.stop_tx	= serial8250_stop_tx,
+	.prep_tx	= serial8250_prep_tx,
 	.start_tx	= serial8250_start_tx,
 	.throttle	= serial8250_throttle,
 	.unthrottle	= serial8250_unthrottle,
-- 
2.33.0

Re: [PATCH 4/6] serial: 8250: Implement prep_tx for power management

[Johan Hovold]

On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote:
> We can use the prep_tx() call to wake up an idle serial port. This allows
> ust to remove the depedency to pm_runtime_irq_safe() for 8250_omap driver
> in the following patches.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1650,6 +1650,29 @@ static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t)
>  	return HRTIMER_NORESTART;
>  }
>  
> +static int serial8250_prep_tx(struct uart_port *port)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	struct device *dev = up->port.dev;
> +	int err;
> +
> +	if (!(up->capabilities & UART_CAP_RPM))
> +		return 0;
> +
> +	if (!pm_runtime_suspended(dev)) {
> +		pm_runtime_mark_last_busy(dev);
> +		return 0;
> +	}
> +
> +	err = pm_request_resume(dev);
> +	if (err < 0) {
> +		dev_warn(dev, "prep_tx wakeup failed: %d\n", err);
> +		return err;
> +	}

How is this supposed to work without a runtime PM usage-counter
increment? What's to prevent the port from suspending again while it's
transmitting?

> +
> +	return -EINPROGRESS;
> +}

Johan

Re: [PATCH 4/6] serial: 8250: Implement prep_tx for power management

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [210923 12:50]:
> On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote:
> > +static int serial8250_prep_tx(struct uart_port *port)
> > +{
> > +	struct uart_8250_port *up = up_to_u8250p(port);
> > +	struct device *dev = up->port.dev;
> > +	int err;
> > +
> > +	if (!(up->capabilities & UART_CAP_RPM))
> > +		return 0;
> > +
> > +	if (!pm_runtime_suspended(dev)) {
> > +		pm_runtime_mark_last_busy(dev);
> > +		return 0;
> > +	}
> > +
> > +	err = pm_request_resume(dev);
> > +	if (err < 0) {
> > +		dev_warn(dev, "prep_tx wakeup failed: %d\n", err);
> > +		return err;
> > +	}
> 
> How is this supposed to work without a runtime PM usage-counter
> increment? What's to prevent the port from suspending again while it's
> transmitting?

Hmm yeah we should at pm_runtime_get() and pm_runtime_put() to write()
unless serial8250_rpm_get() and serial8250_rpm_put() are doing it.

Or pair prep with finish and deal with the usage count there.

Regards,

Tony

Re: [PATCH 4/6] serial: 8250: Implement prep_tx for power management

[Johan Hovold]

On Thu, Sep 23, 2021 at 06:05:36PM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [210923 12:50]:
> > On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote:
> > > +static int serial8250_prep_tx(struct uart_port *port)
> > > +{
> > > +	struct uart_8250_port *up = up_to_u8250p(port);
> > > +	struct device *dev = up->port.dev;
> > > +	int err;
> > > +
> > > +	if (!(up->capabilities & UART_CAP_RPM))
> > > +		return 0;
> > > +
> > > +	if (!pm_runtime_suspended(dev)) {
> > > +		pm_runtime_mark_last_busy(dev);
> > > +		return 0;
> > > +	}
> > > +
> > > +	err = pm_request_resume(dev);
> > > +	if (err < 0) {
> > > +		dev_warn(dev, "prep_tx wakeup failed: %d\n", err);
> > > +		return err;
> > > +	}
> > 
> > How is this supposed to work without a runtime PM usage-counter
> > increment? What's to prevent the port from suspending again while it's
> > transmitting?
> 
> Hmm yeah we should at pm_runtime_get() and pm_runtime_put() to write()
> unless serial8250_rpm_get() and serial8250_rpm_put() are doing it.

If you do the put after just buffering the data it doesn't really solve
anything.

> Or pair prep with finish and deal with the usage count there.

Problem is where to call it from. How do you tell the device is done
transmitting? And how should we deal with flow control? Etc.

Johan

Re: [PATCH 4/6] serial: 8250: Implement prep_tx for power management

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [210924 14:44]:
> On Thu, Sep 23, 2021 at 06:05:36PM +0300, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [210923 12:50]:
> > > On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote:
> > > > +static int serial8250_prep_tx(struct uart_port *port)
> > > > +{
> > > > +	struct uart_8250_port *up = up_to_u8250p(port);
> > > > +	struct device *dev = up->port.dev;
> > > > +	int err;
> > > > +
> > > > +	if (!(up->capabilities & UART_CAP_RPM))
> > > > +		return 0;
> > > > +
> > > > +	if (!pm_runtime_suspended(dev)) {
> > > > +		pm_runtime_mark_last_busy(dev);
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	err = pm_request_resume(dev);
> > > > +	if (err < 0) {
> > > > +		dev_warn(dev, "prep_tx wakeup failed: %d\n", err);
> > > > +		return err;
> > > > +	}
> > > 
> > > How is this supposed to work without a runtime PM usage-counter
> > > increment? What's to prevent the port from suspending again while it's
> > > transmitting?
> > 
> > Hmm yeah we should at pm_runtime_get() and pm_runtime_put() to write()
> > unless serial8250_rpm_get() and serial8250_rpm_put() are doing it.
> 
> If you do the put after just buffering the data it doesn't really solve
> anything.

Right, sounds like we currently rely on the autosuspend_timeout
there.

> > Or pair prep with finish and deal with the usage count there.
> 
> Problem is where to call it from. How do you tell the device is done
> transmitting? And how should we deal with flow control? Etc.

Maybe if the device driver needs to call uart_start() also from runtime
PM idle function and if no data allow suspend. Then if there is
more data, uart_write() calls uart_start() again, device wakes up
and so on.

Regards,

Tony

[PATCH 5/6] serial: 8250_omap: Require a valid wakeirq for deeper idle states

[Tony Lindgren]

For deeper idle states the 8250 device gets powered off. The wakeup is
handled with a separate wakeirq controller monitoring the RX pin.

Let's check for a valid wakeirq before enabling deeper idle states.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -133,6 +133,7 @@ struct omap8250_priv {
 	spinlock_t rx_dma_lock;
 	bool rx_dma_broken;
 	bool throttled;
+	unsigned int allow_rpm:1;
 };
 
 struct omap8250_dma_params {
@@ -676,6 +677,7 @@ static int omap_8250_startup(struct uart_port *port)
 		ret = dev_pm_set_dedicated_wake_irq(port->dev, priv->wakeirq);
 		if (ret)
 			return ret;
+		priv->allow_rpm = 1;
 	}
 
 	pm_runtime_get_sync(port->dev);
@@ -722,6 +724,10 @@ static int omap_8250_startup(struct uart_port *port)
 	if (up->dma && !(priv->habit & UART_HAS_EFR2))
 		up->dma->rx_dma(up);
 
+	/* Block runtime PM if no wakeirq, paired with shutdown */
+	if (!priv->allow_rpm)
+		pm_runtime_get(port->dev);
+
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 	return 0;
@@ -760,6 +766,10 @@ static void omap_8250_shutdown(struct uart_port *port)
 		serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
 	serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
+	/* Clear possible PM runtime block to pair with startup */
+	if (!priv->allow_rpm)
+		pm_runtime_put(port->dev);
+
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 	free_irq(port->irq, port);
-- 
2.33.0

[PATCH 6/6] serial: 8250_omap: Drop the use of pm_runtime_irq_safe()

[Tony Lindgren]

We already have the serial layer RX wake path fixed for power management.
We no longer allow deeper idle states unless the kernel console has been
detached, and we require that the RX wakeirq is configured.

For TX path, we may have the serial port autoidled, and need to wake it
up before writing to it. With the serial_core prep_tx() changes, we can
finally drop the dependency to pm_runtime_irq_safe() for 8250_omap driver.

To drop pm_runtime_irq_safe(), we remove all PM runtime calls from the
interrupt context. If we ever see an interrupt for an idled port, we just
bail out. We now also need to restore the port context with interrupts
disabled to prevent interrupts from happening while restoring the port.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -134,6 +134,7 @@ struct omap8250_priv {
 	bool rx_dma_broken;
 	bool throttled;
 	unsigned int allow_rpm:1;
+	unsigned int clocks_off:1;
 };
 
 struct omap8250_dma_params {
@@ -621,6 +622,9 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	unsigned int iir, lsr;
 	int ret;
 
+	if (priv->clocks_off)
+		return IRQ_NONE;
+
 #ifdef CONFIG_SERIAL_8250_DMA
 	if (up->dma) {
 		ret = omap_8250_dma_handle_irq(port);
@@ -628,7 +632,6 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	}
 #endif
 
-	serial8250_rpm_get(up);
 	lsr = serial_port_in(port, UART_LSR);
 	iir = serial_port_in(port, UART_IIR);
 	ret = serial8250_handle_irq(port, iir);
@@ -662,8 +665,6 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 		schedule_delayed_work(&up->overrun_backoff, delay);
 	}
 
-	serial8250_rpm_put(up);
-
 	return IRQ_RETVAL(ret);
 }
 
@@ -1191,13 +1192,9 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	unsigned char status;
 	u8 iir;
 
-	serial8250_rpm_get(up);
-
 	iir = serial_port_in(port, UART_IIR);
-	if (iir & UART_IIR_NO_INT) {
-		serial8250_rpm_put(up);
+	if (iir & UART_IIR_NO_INT)
 		return IRQ_HANDLED;
-	}
 
 	spin_lock(&port->lock);
 
@@ -1226,7 +1223,6 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 
 	uart_unlock_and_check_sysrq(port);
 
-	serial8250_rpm_put(up);
 	return 1;
 }
 
@@ -1420,8 +1416,6 @@ static int omap8250_probe(struct platform_device *pdev)
 	if (!of_get_available_child_count(pdev->dev.of_node))
 		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
 
-	pm_runtime_irq_safe(&pdev->dev);
-
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(&up, priv);
@@ -1637,6 +1631,8 @@ static int omap8250_runtime_suspend(struct device *dev)
 		omap_8250_rx_dma_flush(up);
 
 	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
+	priv->clocks_off = 1;
+
 	schedule_work(&priv->qos_work);
 
 	return 0;
@@ -1646,13 +1642,18 @@ static int omap8250_runtime_resume(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up;
+	struct uart_port *port;
+	unsigned long flags;
 
 	/* In case runtime-pm tries this before we are setup */
 	if (!priv)
 		return 0;
 
 	up = serial8250_get_port(priv->line);
+	port = &up->port;
 
+	/* Restore state with interrupts disabled */
+	spin_lock_irqsave(&port->lock, flags);
 	if (omap8250_lost_context(up))
 		omap8250_restore_regs(up);
 
@@ -1660,6 +1661,9 @@ static int omap8250_runtime_resume(struct device *dev)
 		omap_8250_rx_dma(up);
 
 	priv->latency = priv->calc_latency;
+	priv->clocks_off = 0;
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	schedule_work(&priv->qos_work);
 	return 0;
 }
-- 
2.33.0

Re: [PATCH 0/6] Get rid of pm_runtime_irq_safe() for 8250_omap

[Andy Shevchenko]

On Tue, Sep 21, 2021 at 1:34 PM Tony Lindgren <tony@atomide.com> wrote:
>
> Hi,
>
> Here are patches to get rid of pm_runtime_irq_safe() for the 8250_omap
> driver.
>
> For removing the pm_runtime_irq_safe() usage, serial TX is the last
> remaining issue. We deal with TX by waking up the port and returning 0
> bytes written from write_room() and write() if the port is not available
> because of PM runtime autoidle.

of the PM

>
> This series also removes the dependency to Andy's pending generic serial
> layer PM runtime patches, and hopefully makes that work a bit easier :)


Thank you, Tony, very much! The series looks to me in a good
condition, hence, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Regards,
>
> Tony
>
>
> Tony Lindgren (6):
>   n_tty: Start making use of -EAGAIN returned from
>     process_output_block()
>   tty: n_gsm: Don't ignore write return value in gsmld_output()
>   serial: core: Add new prep_tx for power management
>   serial: 8250: Implement prep_tx for power management
>   serial: 8250_omap: Require a valid wakeirq for deeper idle states
>   serial: 8250_omap: Drop the use of pm_runtime_irq_safe()
>
>  Documentation/driver-api/serial/driver.rst |  9 ++++++
>  drivers/tty/n_gsm.c                        |  5 ++-
>  drivers/tty/n_tty.c                        |  8 +++--
>  drivers/tty/serial/8250/8250_omap.c        | 36 +++++++++++++++-------
>  drivers/tty/serial/8250/8250_port.c        | 24 +++++++++++++++
>  drivers/tty/serial/serial_core.c           | 23 ++++++++++++++
>  include/linux/serial_core.h                |  1 +
>  7 files changed, 90 insertions(+), 16 deletions(-)
>
> --
> 2.33.0



-- 
With Best Regards,
Andy Shevchenko