[PATCH 1/3] tty: serial: qcom-geni-serial: Fix kdb/kgdb after port shutdown (again)

[PATCH 1/3] tty: serial: qcom-geni-serial: Fix kdb/kgdb after port shutdown (again)

[Douglas Anderson]

Commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
in progress at shutdown") was basically a straight revert of the
commit it claims to fix without any explanation of why the problems
talked about in the original patch were no longer relevant. Indeed,
commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
in progress at shutdown") re-introduces the same problem that commit
e83766334f96 ("tty: serial: qcom_geni_serial: No need to stop tx/rx on
UART shutdown") fixed.

The problems are very easy to see by simply doing this on a
sc7180-based Chromebook:

1. Boot in developer mode with serial console enabled and kdb setup on
   the serial console.
2. via ssh: stop console-ttyMSM0; echo g > /proc/sysrq-trigger

When you do the above you'll see the "kdb" prompt printed on the
serial console but be unable to interact with it.

Let's fix the problem again by noting that the console port is never
configured in DMA mode and thus we don't need to stop things for the
console.

Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index d69592e5e2ec..74a0e074c2de 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1070,8 +1070,10 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
 static void qcom_geni_serial_shutdown(struct uart_port *uport)
 {
 	disable_irq(uport->irq);
-	qcom_geni_serial_stop_tx(uport);
-	qcom_geni_serial_stop_rx(uport);
+	if (!uart_console(uport)) {
+		qcom_geni_serial_stop_tx(uport);
+		qcom_geni_serial_stop_rx(uport);
+	}
 }
 
 static int qcom_geni_serial_port_setup(struct uart_port *uport)
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 2/3] serial: uart_poll_init() should power on the UART

[Douglas Anderson]

On Qualcomm devices which use the "geni" serial driver, kdb/kgdb won't
be very happy if you use it but the resources of the port haven't been
powered on. Today kdb/kgdb rely on someone else powering the port
on. This could be the normal kernel console or an agetty running.
Let's fix this to explicitly power things on when setting up a polling
driver.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/tty/serial/serial_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 2bd32c8ece39..b14b5ed6fff4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2593,6 +2593,7 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
 {
 	struct uart_driver *drv = driver->driver_state;
 	struct uart_state *state = drv->state + line;
+	enum uart_pm_state pm_state;
 	struct tty_port *tport;
 	struct uart_port *port;
 	int baud = 9600;
@@ -2610,6 +2611,9 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
 		goto out;
 	}
 
+	pm_state = state->pm_state;
+	uart_change_pm(state, UART_PM_STATE_ON);
+
 	if (port->ops->poll_init) {
 		/*
 		 * We don't set initialized as we only initialized the hw,
@@ -2626,6 +2630,8 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
 		console_list_unlock();
 	}
 out:
+	if (ret)
+		uart_change_pm(state, pm_state);
 	mutex_unlock(&tport->mutex);
 	return ret;
 }
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

Re: [PATCH 2/3] serial: uart_poll_init() should power on the UART

[Doug Anderson]

Hi,

On Tue, Mar 7, 2023 at 7:32 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> On Qualcomm devices which use the "geni" serial driver, kdb/kgdb won't
> be very happy if you use it but the resources of the port haven't been
> powered on. Today kdb/kgdb rely on someone else powering the port
> on. This could be the normal kernel console or an agetty running.
> Let's fix this to explicitly power things on when setting up a polling
> driver.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/tty/serial/serial_core.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Just in case it's not obvious, even though we ended up going with
Johan's series [1] instead of patch #1 of my series, patch #2 and #3
of my series are still relevant. I can repost the series without patch
#1 if it's helpful.

[1] https://lore.kernel.org/r/20230307164405.14218-1-johan+linaro@kernel.org

-Doug

Re: [PATCH 2/3] serial: uart_poll_init() should power on the UART

[Greg Kroah-Hartman]

On Mon, Mar 13, 2023 at 01:53:02PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Mar 7, 2023 at 7:32 AM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > On Qualcomm devices which use the "geni" serial driver, kdb/kgdb won't
> > be very happy if you use it but the resources of the port haven't been
> > powered on. Today kdb/kgdb rely on someone else powering the port
> > on. This could be the normal kernel console or an agetty running.
> > Let's fix this to explicitly power things on when setting up a polling
> > driver.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  drivers/tty/serial/serial_core.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> 
> Just in case it's not obvious, even though we ended up going with
> Johan's series [1] instead of patch #1 of my series, patch #2 and #3
> of my series are still relevant. I can repost the series without patch
> #1 if it's helpful.
> 
> [1] https://lore.kernel.org/r/20230307164405.14218-1-johan+linaro@kernel.org

Ye,s it's not obvious at all.  Please resubmit.

thanks,

greg k-h

[PATCH 3/3] tty: serial: qcom-geni-serial: Add a poll_init() function

[Douglas Anderson]

On sc7180 Chromebooks, I did the following:
* Didn't enable earlycon in the kernel command line.
* Didn't enable serial console in the kernel command line.
* Didn't enable an agetty or any other client of "/dev/ttyMSM0".
* Added "kgdboc=ttyMSM0" to the kernel command line.

After I did that, I tried to enter kdb with this command over an ssh
session:
  echo g > /proc/sysrq-trigger

When I did that the system just hung.

Although I thought I'd tested this scenario before, I couldn't go back
and find a time when it was working. Previous testing must have relied
on either the UART acting as the kernel console or an agetty running.

It turns out to be pretty easy to fix: we can just use
qcom_geni_serial_port_setup() as the .poll_init() function. This,
together with the patch ("serial: uart_poll_init() should power on the
UART"), allows the debugger to work even if there are no other users
of the serial port.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/tty/serial/qcom_geni_serial.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 74a0e074c2de..00752ff783c6 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1534,6 +1534,7 @@ static const struct uart_ops qcom_geni_console_pops = {
 #ifdef CONFIG_CONSOLE_POLL
 	.poll_get_char	= qcom_geni_serial_get_char,
 	.poll_put_char	= qcom_geni_serial_poll_put_char,
+	.poll_init = qcom_geni_serial_port_setup,
 #endif
 	.pm = qcom_geni_serial_pm,
 };
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

Re: [PATCH 1/3] tty: serial: qcom-geni-serial: Fix kdb/kgdb after port shutdown (again)

[Bartosz Golaszewski]

On Tue, Mar 7, 2023 at 4:33 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
> in progress at shutdown") was basically a straight revert of the
> commit it claims to fix without any explanation of why the problems
> talked about in the original patch were no longer relevant. Indeed,
> commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
> in progress at shutdown") re-introduces the same problem that commit
> e83766334f96 ("tty: serial: qcom_geni_serial: No need to stop tx/rx on
> UART shutdown") fixed.
>
> The problems are very easy to see by simply doing this on a
> sc7180-based Chromebook:
>
> 1. Boot in developer mode with serial console enabled and kdb setup on
>    the serial console.
> 2. via ssh: stop console-ttyMSM0; echo g > /proc/sysrq-trigger
>
> When you do the above you'll see the "kdb" prompt printed on the
> serial console but be unable to interact with it.
>
> Let's fix the problem again by noting that the console port is never
> configured in DMA mode and thus we don't need to stop things for the
> console.
>
> Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index d69592e5e2ec..74a0e074c2de 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1070,8 +1070,10 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
>  static void qcom_geni_serial_shutdown(struct uart_port *uport)
>  {
>         disable_irq(uport->irq);
> -       qcom_geni_serial_stop_tx(uport);
> -       qcom_geni_serial_stop_rx(uport);
> +       if (!uart_console(uport)) {
> +               qcom_geni_serial_stop_tx(uport);
> +               qcom_geni_serial_stop_rx(uport);
> +       }
>  }
>
>  static int qcom_geni_serial_port_setup(struct uart_port *uport)
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
>

Hey Douglas,

Thanks for this, I was about to post a similar fix, this was reported
to me by two other people.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 1/3] tty: serial: qcom-geni-serial: Fix kdb/kgdb after port shutdown (again)

[Bartosz Golaszewski]

On Tue, Mar 7, 2023 at 4:33 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
> in progress at shutdown") was basically a straight revert of the
> commit it claims to fix without any explanation of why the problems
> talked about in the original patch were no longer relevant. Indeed,
> commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
> in progress at shutdown") re-introduces the same problem that commit
> e83766334f96 ("tty: serial: qcom_geni_serial: No need to stop tx/rx on
> UART shutdown") fixed.
>
> The problems are very easy to see by simply doing this on a
> sc7180-based Chromebook:
>
> 1. Boot in developer mode with serial console enabled and kdb setup on
>    the serial console.
> 2. via ssh: stop console-ttyMSM0; echo g > /proc/sysrq-trigger
>
> When you do the above you'll see the "kdb" prompt printed on the
> serial console but be unable to interact with it.
>
> Let's fix the problem again by noting that the console port is never
> configured in DMA mode and thus we don't need to stop things for the
> console.
>
> Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index d69592e5e2ec..74a0e074c2de 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1070,8 +1070,10 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
>  static void qcom_geni_serial_shutdown(struct uart_port *uport)
>  {
>         disable_irq(uport->irq);
> -       qcom_geni_serial_stop_tx(uport);
> -       qcom_geni_serial_stop_rx(uport);
> +       if (!uart_console(uport)) {
> +               qcom_geni_serial_stop_tx(uport);
> +               qcom_geni_serial_stop_rx(uport);
> +       }
>  }
>
>  static int qcom_geni_serial_port_setup(struct uart_port *uport)
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
>

And also:

Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 1/3] tty: serial: qcom-geni-serial: Fix kdb/kgdb after port shutdown (again)

[Johan Hovold]

On Tue, Mar 07, 2023 at 07:32:11AM -0800, Douglas Anderson wrote:
> Commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
> in progress at shutdown") was basically a straight revert of the
> commit it claims to fix without any explanation of why the problems
> talked about in the original patch were no longer relevant. Indeed,
> commit d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations
> in progress at shutdown") re-introduces the same problem that commit
> e83766334f96 ("tty: serial: qcom_geni_serial: No need to stop tx/rx on
> UART shutdown") fixed.
> 
> The problems are very easy to see by simply doing this on a
> sc7180-based Chromebook:
> 
> 1. Boot in developer mode with serial console enabled and kdb setup on
>    the serial console.
> 2. via ssh: stop console-ttyMSM0; echo g > /proc/sysrq-trigger
> 
> When you do the above you'll see the "kdb" prompt printed on the
> serial console but be unable to interact with it.
> 
> Let's fix the problem again by noting that the console port is never
> configured in DMA mode and thus we don't need to stop things for the
> console.
> 
> Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")

The offending commit broke serial console more generally by breaking TX
and thus hanging the system when stopping the getty on reboot.

The underlying bug has been there since this driver was first merged,
and as fixing it properly is going to be a bit involved, I was about to
post a patch equivalent to this one to fix the immediate regression and
get us back to status quo.

> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

> ---
> 
>  drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index d69592e5e2ec..74a0e074c2de 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1070,8 +1070,10 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
>  static void qcom_geni_serial_shutdown(struct uart_port *uport)
>  {
>  	disable_irq(uport->irq);
> -	qcom_geni_serial_stop_tx(uport);
> -	qcom_geni_serial_stop_rx(uport);
> +	if (!uart_console(uport)) {
> +		qcom_geni_serial_stop_tx(uport);
> +		qcom_geni_serial_stop_rx(uport);
> +	}
>  }
>  
>  static int qcom_geni_serial_port_setup(struct uart_port *uport)

Johan