[PATCH v2 0/7] serial: qcom-geni: fix receiver enable

[PATCH v2 0/7] serial: qcom-geni: fix receiver enable

[Johan Hovold]

This series is a follow up to the console fixes in 6.12-rc1 that can
interact badly with some pre-existing bugs.

Specifically, the receiver could end up being disabled when
set_termios() races with the console code during boot.

Fixing the missing locking in set_termios() exposes another
long-standing bug in the DMA implementation (e.g. used for Bluetooth),
which is also fixed in v2.

Johan


Changes in v2
 - keep the call to stop rx in shutdown() which is called also on
   hangups
 - fix rx dma cancellation
 - fix rx cancel dma status bit
 - drop flip buffer WARN()
 - drop unused receive parameter

Johan Hovold (7):
  serial: qcom-geni: fix premature receiver enable
  serial: qcom-geni: fix shutdown race
  serial: qcom-geni: fix dma rx cancellation
  serial: qcom-geni: fix receiver enable
  serial: qcom-geni: fix rx cancel dma status bit
  serial: qcom-geni: drop flip buffer WARN()
  serial: qcom-geni: drop unused receive parameter

 drivers/tty/serial/qcom_geni_serial.c | 40 +++++++++++++++++----------
 include/linux/soc/qcom/geni-se.h      |  2 +-
 2 files changed, 27 insertions(+), 15 deletions(-)

-- 
2.45.2

[PATCH v2 1/7] serial: qcom-geni: fix premature receiver enable

[Johan Hovold]

The receiver should not be enabled until the port is opened so drop the
bogus call to start rx from the setup code which is shared with the
console implementation.

This was added for some confused implementation of hibernation support,
but the receiver must not be started unconditionally as the port may not
have been open when hibernating the system.

Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
Cc: stable@vger.kernel.org	# 6.2
Cc: Aniket Randive <quic_arandive@quicinc.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 6f0db310cf69..9ea6bd09e665 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 			       false, true, true);
 	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
 	geni_se_select_mode(&port->se, port->dev_data->mode);
-	qcom_geni_serial_start_rx(uport);
 	port->setup = true;
 
 	return 0;
-- 
2.45.2

[PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Johan Hovold]

A commit adding back the stopping of tx on port shutdown failed to add
back the locking which had also been removed by commit e83766334f96
("tty: serial: qcom_geni_serial: No need to stop tx/rx on UART
shutdown").

Holding the port lock is needed to serialise against the console code,
which may update the interrupt enable register and access the port
state.

Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
Fixes: 947cc4ecc06c ("serial: qcom-geni: fix soft lockup on sw flow control and suspend")
Cc: stable@vger.kernel.org	# 6.3
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 9ea6bd09e665..b6a8729cee6d 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1096,10 +1096,12 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
 {
 	disable_irq(uport->irq);
 
+	uart_port_lock_irq(uport);
 	qcom_geni_serial_stop_tx(uport);
 	qcom_geni_serial_stop_rx(uport);
 
 	qcom_geni_serial_cancel_tx_cmd(uport);
+	uart_port_unlock_irq(uport);
 }
 
 static void qcom_geni_serial_flush_buffer(struct uart_port *uport)
-- 
2.45.2

[PATCH v2 3/7] serial: qcom-geni: fix dma rx cancellation

[Johan Hovold]

Make sure to wait for the DMA transfer to complete when cancelling the
rx command on stop_rx(). This specifically prevents the DMA completion
interrupt from firing after rx has been restarted, something which can
lead to an IOMMU fault and hosed rx when the interrupt handler unmaps
the DMA buffer for the new command:

	qcom_geni_serial 988000.serial: serial engine reports 0 RX bytes in!
	arm-smmu 15000000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x563
	arm-smmu 15000000.iommu: FSYNR0 = 00210013 [S1CBNDX=33 WNR PLVL=3]
	Bluetooth: hci0: command 0xfc00 tx timeout
	Bluetooth: hci0: Reading QCA version information failed (-110)

Also add the missing state machine reset which is needed in case
cancellation fails.

Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
Cc: stable@vger.kernel.org      # 6.3
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index b6a8729cee6d..dea688db0d7c 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -787,17 +787,27 @@ static void qcom_geni_serial_start_rx_fifo(struct uart_port *uport)
 static void qcom_geni_serial_stop_rx_dma(struct uart_port *uport)
 {
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	bool done;
 
 	if (!qcom_geni_serial_secondary_active(uport))
 		return;
 
 	geni_se_cancel_s_cmd(&port->se);
-	qcom_geni_serial_poll_bit(uport, SE_GENI_S_IRQ_STATUS,
-				  S_CMD_CANCEL_EN, true);
-
-	if (qcom_geni_serial_secondary_active(uport))
+	done = qcom_geni_serial_poll_bit(uport, SE_DMA_RX_IRQ_STAT,
+			RX_EOT, true);
+	if (done) {
+		writel(RX_EOT | RX_DMA_DONE,
+				uport->membase + SE_DMA_RX_IRQ_CLR);
+	} else {
 		qcom_geni_serial_abort_rx(uport);
 
+		writel(1, uport->membase + SE_DMA_RX_FSM_RST);
+		qcom_geni_serial_poll_bit(uport, SE_DMA_RX_IRQ_STAT,
+				RX_RESET_DONE, true);
+		writel(RX_RESET_DONE | RX_DMA_DONE,
+				uport->membase + SE_DMA_RX_IRQ_CLR);
+	}
+
 	if (port->rx_dma_addr) {
 		geni_se_rx_dma_unprep(&port->se, port->rx_dma_addr,
 				      DMA_RX_BUF_SIZE);
-- 
2.45.2

[PATCH v2 4/7] serial: qcom-geni: fix receiver enable

[Johan Hovold]

The receiver should be enabled in the startup() callback and there is no
need to stop it on every termios update.

Since commit 6f3c3cafb115 ("serial: qcom-geni: disable interrupts during
console writes") the calls to manipulate the secondary interrupts, which
were done without holding the port lock, can lead to the receiver being
left disabled when set_termios() races with the console code (e.g. when
init opens the tty during boot).

The calls to stop and start rx in set_termios() can similarly race with
DMA completion and, for example, cause the DMA buffer to be unmapped
twice or the mapping to be leaked.

Fixes: 6f3c3cafb115 ("serial: qcom-geni: disable interrupts during console writes")
Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Cc: stable@vger.kernel.org      # 6.3
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index dea688db0d7c..5b6c5388efee 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1179,6 +1179,11 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
 		if (ret)
 			return ret;
 	}
+
+	uart_port_lock_irq(uport);
+	qcom_geni_serial_start_rx(uport);
+	uart_port_unlock_irq(uport);
+
 	enable_irq(uport->irq);
 
 	return 0;
@@ -1264,7 +1269,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	unsigned int avg_bw_core;
 	unsigned long timeout;
 
-	qcom_geni_serial_stop_rx(uport);
 	/* baud rate */
 	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
 
@@ -1280,7 +1284,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 		dev_err(port->se.dev,
 			"Couldn't find suitable clock rate for %u\n",
 			baud * sampling_rate);
-		goto out_restart_rx;
+		return;
 	}
 
 	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
@@ -1371,8 +1375,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
 	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
 	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
-out_restart_rx:
-	qcom_geni_serial_start_rx(uport);
 }
 
 #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
-- 
2.45.2

[PATCH v2 5/7] serial: qcom-geni: fix rx cancel dma status bit

[Johan Hovold]

Cancelling an rx command is signalled using bit 14 of the rx DMA status
register and not bit 11.

This bit is currently unused, but this error becomes apparent, for
example, when tracing the status register when closing the port.

Fixes: eddac5af0654 ("soc: qcom: Add GENI based QUP Wrapper driver")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 include/linux/soc/qcom/geni-se.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index c3bca9c0bf2c..2996a3c28ef3 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -258,8 +258,8 @@ struct geni_se {
 #define RX_DMA_PARITY_ERR		BIT(5)
 #define RX_DMA_BREAK			GENMASK(8, 7)
 #define RX_GENI_GP_IRQ			GENMASK(10, 5)
-#define RX_GENI_CANCEL_IRQ		BIT(11)
 #define RX_GENI_GP_IRQ_EXT		GENMASK(13, 12)
+#define RX_GENI_CANCEL_IRQ		BIT(14)
 
 /* SE_HW_PARAM_0 fields */
 #define TX_FIFO_WIDTH_MSK		GENMASK(29, 24)
-- 
2.45.2

[PATCH v2 6/7] serial: qcom-geni: drop flip buffer WARN()

[Johan Hovold]

Drop the unnecessary WARN() in case the TTY buffers are ever full in
favour of a rate limited dev_err() which doesn't kill the machine when
panic_on_warn is set.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 5b6c5388efee..8bc4b240bf59 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -570,9 +570,8 @@ static void handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
 
 	ret = tty_insert_flip_string(tport, port->rx_buf, bytes);
 	if (ret != bytes) {
-		dev_err(uport->dev, "%s:Unable to push data ret %d_bytes %d\n",
-				__func__, ret, bytes);
-		WARN_ON_ONCE(1);
+		dev_err_ratelimited(uport->dev, "failed to push data (%d < %u)\n",
+				ret, bytes);
 	}
 	uport->icount.rx += ret;
 	tty_flip_buffer_push(tport);
-- 
2.45.2

[PATCH v2 7/7] serial: qcom-geni: drop unused receive parameter

[Johan Hovold]

Serial drivers should not be dropping characters themselves, but at
least drop the unused 'drop' parameter from the receive handler for now.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 8bc4b240bf59..daa852785bd9 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -562,7 +562,7 @@ static void handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
 }
 #endif /* CONFIG_SERIAL_QCOM_GENI_CONSOLE */
 
-static void handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
+static void handle_rx_uart(struct uart_port *uport, u32 bytes)
 {
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
 	struct tty_port *tport = &uport->state->port;
@@ -855,7 +855,7 @@ static void qcom_geni_serial_handle_rx_dma(struct uart_port *uport, bool drop)
 	}
 
 	if (!drop)
-		handle_rx_uart(uport, rx_in, drop);
+		handle_rx_uart(uport, rx_in);
 
 	ret = geni_se_rx_dma_prep(&port->se, port->rx_buf,
 				  DMA_RX_BUF_SIZE,
-- 
2.45.2

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Bartosz Golaszewski]

On Tue, Oct 1, 2024 at 2:51 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> A commit adding back the stopping of tx on port shutdown failed to add
> back the locking which had also been removed by commit e83766334f96
> ("tty: serial: qcom_geni_serial: No need to stop tx/rx on UART
> shutdown").
>
> Holding the port lock is needed to serialise against the console code,
> which may update the interrupt enable register and access the port
> state.
>
> Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
> Fixes: 947cc4ecc06c ("serial: qcom-geni: fix soft lockup on sw flow control and suspend")
> Cc: stable@vger.kernel.org      # 6.3
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 9ea6bd09e665..b6a8729cee6d 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1096,10 +1096,12 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
>  {
>         disable_irq(uport->irq);
>
> +       uart_port_lock_irq(uport);
>         qcom_geni_serial_stop_tx(uport);
>         qcom_geni_serial_stop_rx(uport);
>
>         qcom_geni_serial_cancel_tx_cmd(uport);
> +       uart_port_unlock_irq(uport);
>  }
>
>  static void qcom_geni_serial_flush_buffer(struct uart_port *uport)
> --
> 2.45.2
>
>

I already reviewed this[1]. I suggest using b4 for automated tag pickup.

Bart

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

[1] https://lore.kernel.org/all/CAMRc=Md-B3MCdjBA6Z03Tn09Qdq_O=2Gij=0kr8HiLtpp11kVg@mail.gmail.com/#t

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Johan Hovold]

On Tue, Oct 01, 2024 at 03:36:57PM +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 1, 2024 at 2:51 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > A commit adding back the stopping of tx on port shutdown failed to add
> > back the locking which had also been removed by commit e83766334f96
> > ("tty: serial: qcom_geni_serial: No need to stop tx/rx on UART
> > shutdown").
> >
> > Holding the port lock is needed to serialise against the console code,
> > which may update the interrupt enable register and access the port
> > state.
> >
> > Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
> > Fixes: 947cc4ecc06c ("serial: qcom-geni: fix soft lockup on sw flow control and suspend")
> > Cc: stable@vger.kernel.org      # 6.3
> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

> I already reviewed this[1]. I suggest using b4 for automated tag pickup.

There were changes in v2 so I dropped your tag on purpose.

Johan

Re: [PATCH v2 1/7] serial: qcom-geni: fix premature receiver enable

[Mukesh Kumar Savaliya]

Thanks Johan for the fixes.

On 10/1/2024 6:20 PM, Johan Hovold wrote:
> The receiver should not be enabled until the port is opened so drop the
> bogus call to start rx from the setup code which is shared with the
> console implementation.
> 
> This was added for some confused implementation of hibernation support,
> but the receiver must not be started unconditionally as the port may not
> have been open when hibernating the system.
> 
> Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> Cc:stable@vger.kernel.org	# 6.2
> Cc: Aniket Randive<quic_arandive@quicinc.com>
> Signed-off-by: Johan Hovold<johan+linaro@kernel.org>
> ---
>   drivers/tty/serial/qcom_geni_serial.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 6f0db310cf69..9ea6bd09e665 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>   			       false, true, true);
>   	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
>   	geni_se_select_mode(&port->se, port->dev_data->mode);
> -	qcom_geni_serial_start_rx(uport);
Does it mean hibernation will break now ? Not sure if its tested with 
hibernation. I can see this call was added to port_setup specifically 
for hibernation but now after removing it, where is it getting fixed ?
I think RX will not be initialized after hibernation.
>   	port->setup = true;
>   
>   	return 0;
> -- 2.45.2

Re: [PATCH v2 1/7] serial: qcom-geni: fix premature receiver enable

[Bjorn Andersson]

On Tue, Oct 01, 2024 at 07:20:36PM GMT, Mukesh Kumar Savaliya wrote:
> Thanks Johan for the fixes.
> 
> On 10/1/2024 6:20 PM, Johan Hovold wrote:
> > The receiver should not be enabled until the port is opened so drop the
> > bogus call to start rx from the setup code which is shared with the
> > console implementation.
> > 
> > This was added for some confused implementation of hibernation support,
> > but the receiver must not be started unconditionally as the port may not
> > have been open when hibernating the system.
> > 
> > Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> > Cc:stable@vger.kernel.org	# 6.2
> > Cc: Aniket Randive<quic_arandive@quicinc.com>
> > Signed-off-by: Johan Hovold<johan+linaro@kernel.org>
> > ---
> >   drivers/tty/serial/qcom_geni_serial.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 6f0db310cf69..9ea6bd09e665 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> >   			       false, true, true);
> >   	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
> >   	geni_se_select_mode(&port->se, port->dev_data->mode);
> > -	qcom_geni_serial_start_rx(uport);
> Does it mean hibernation will break now ? Not sure if its tested with
> hibernation. I can see this call was added to port_setup specifically for
> hibernation but now after removing it, where is it getting fixed ?

Can you explain how you're testing hibernation and on which platform
this is done? I'd like to add this to my set of tests, but last time I
tested I couldn't find a platform where we survived the restore
processes (it's been a while though).

> I think RX will not be initialized after hibernation.

qcom_geni_serial_port_setup() is invoked in multiple places, how come
we don't perform this hibernation-specific operation in
qcom_geni_serial_sys_hib_resume()? (And why is it called hib_resume when
the kernel nomenclature for what it does is "restore"?)

Regards,
Bjorn

> >   	port->setup = true;
> >   	return 0;
> > -- 2.45.2

Re: [PATCH v2 1/7] serial: qcom-geni: fix premature receiver enable

[Doug Anderson]

Hi,

On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The receiver should not be enabled until the port is opened so drop the
> bogus call to start rx from the setup code which is shared with the
> console implementation.
>
> This was added for some confused implementation of hibernation support,
> but the receiver must not be started unconditionally as the port may not
> have been open when hibernating the system.

Could you provide a motivation for your patch in the description? Is
patch needed for something (perhaps a future patch in the series)? Is
it fixing a bug? Does it save power? Is the call harmless but cleaner
to get rid of?


> Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> Cc: stable@vger.kernel.org      # 6.2
> Cc: Aniket Randive <quic_arandive@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 6f0db310cf69..9ea6bd09e665 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>                                false, true, true);
>         geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
>         geni_se_select_mode(&port->se, port->dev_data->mode);
> -       qcom_geni_serial_start_rx(uport);

FWIW, I found at least one thing that's broken by your patch. If you
enable kgdb (but _not_ "kgdboc_earlycon") and then add "kgdbwait" to
the kernel command line parameters then things will be broken after
your patch. You'll drop into the debugger but can't interact with it.
The "kgdboc_earlycon" path handles this because of
"qcom_geni_serial_enable_early_read()" but it doesn't seem like
there's anything that handles it for normal kgdb. If you drop in the
debugger later it'll probably work if you've got an "agetty" running
because that'll enable the RX path.


-Doug

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Doug Anderson]

Hi,

On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> A commit adding back the stopping of tx on port shutdown failed to add
> back the locking which had also been removed by commit e83766334f96
> ("tty: serial: qcom_geni_serial: No need to stop tx/rx on UART
> shutdown").

Hmmm, when I look at that commit it makes me think that the problem
that commit e83766334f96 ("tty: serial: qcom_geni_serial: No need to
stop tx/rx on UART shutdown") was fixing was re-introduced by commit
d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in
progress at shutdown"). ...and indeed, it was. :(

I can't interact with kgdb if I do this:

1. ssh over to DUT
2. Kill the console process (on ChromeOS stop console-ttyMSM0)
3. Drop in the debugger (echo g > /proc/sysrq-trigger)

This bug predates your series, but since it touches the same code
maybe you could fix it at the same time? I will note that commit
e83766334f96 ("tty: serial: qcom_geni_serial: No need to stop tx/rx on
UART shutdown") mentions that it wasn't required for FIFO mode--only
DMA...

Aside from the pre-existing bug, I agree that the locking should be there.


-Doug

Re: [PATCH v2 5/7] serial: qcom-geni: fix rx cancel dma status bit

[Doug Anderson]

Hi,

On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Cancelling an rx command is signalled using bit 14 of the rx DMA status
> register and not bit 11.
>
> This bit is currently unused, but this error becomes apparent, for
> example, when tracing the status register when closing the port.
>
> Fixes: eddac5af0654 ("soc: qcom: Add GENI based QUP Wrapper driver")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  include/linux/soc/qcom/geni-se.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index c3bca9c0bf2c..2996a3c28ef3 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -258,8 +258,8 @@ struct geni_se {
>  #define RX_DMA_PARITY_ERR              BIT(5)
>  #define RX_DMA_BREAK                   GENMASK(8, 7)
>  #define RX_GENI_GP_IRQ                 GENMASK(10, 5)
> -#define RX_GENI_CANCEL_IRQ             BIT(11)
>  #define RX_GENI_GP_IRQ_EXT             GENMASK(13, 12)
> +#define RX_GENI_CANCEL_IRQ             BIT(14)

This looks right, but do you want to fix all the rest of the wrong
bits in this list while you're at it? Things look OK up to the
"RX_FLUSH_DONE" and then they're wrong. Specifically:

* My datasheet doesn't have RX_DMA_PARITY_ERR. Unless maybe it's one
of the "GP" IRQs?

* My datassheet doesn't have RX_DMA_BREAK. Unless maybe it's one of
the "GP" IRQs (though why would it be two bits big?)

* RX_GENI_GP_IRQ is 12:5, not 10:5

* My datasheet has RX_GENI_CMD_FAILURE as BIT(15).

In any case, this does make it better so:

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


-Doug

Re: [PATCH v2 6/7] serial: qcom-geni: drop flip buffer WARN()

[Doug Anderson]

Hi,

On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Drop the unnecessary WARN() in case the TTY buffers are ever full in
> favour of a rate limited dev_err() which doesn't kill the machine when
> panic_on_warn is set.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 5b6c5388efee..8bc4b240bf59 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -570,9 +570,8 @@ static void handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
>
>         ret = tty_insert_flip_string(tport, port->rx_buf, bytes);
>         if (ret != bytes) {
> -               dev_err(uport->dev, "%s:Unable to push data ret %d_bytes %d\n",
> -                               __func__, ret, bytes);
> -               WARN_ON_ONCE(1);
> +               dev_err_ratelimited(uport->dev, "failed to push data (%d < %u)\n",
> +                               ret, bytes);

Not that it really matters, but since you're fixing the type of
"bytes" to %u you probably should fix "ret" to %u too, which means
changing the type of it? Officially tty_insert_flip_string returns the
(unsigned) size_t.

As a nit, I'd also say that your error message shouldn't assert "<"
unless you change your "if" test to "<". It seems safer to use != so
IMO the printout should also say "!=".

I'd hope you're not hitting this error a lot because it means you're
dropping bytes, but getting rid of the WARN_ON and changing to
ratelimited makes sense to me.

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

Re: [PATCH v2 7/7] serial: qcom-geni: drop unused receive parameter

[Doug Anderson]

Hi,

On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Serial drivers should not be dropping characters themselves, but at
> least drop the unused 'drop' parameter from the receive handler for now.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Sure. I haven't spent any time figuring out what serial drivers are
supposed to do with parity errors occur (I don't think I've ever used
a UART that enabled the parity bit in all my years), but at least I
agree that getting rid of this useless parameter makes sense.

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

I guess while reviewing this patch I can also see that, indeed, parity
errors seem to be "GP0" and break seems to be (strangely) GP2 and GP3.
I guess that answers some of the questions I had on patch #5.

Re: [PATCH v2 4/7] serial: qcom-geni: fix receiver enable

[Doug Anderson]

Hi,

On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The receiver should be enabled in the startup() callback and there is no
> need to stop it on every termios update.
>
> Since commit 6f3c3cafb115 ("serial: qcom-geni: disable interrupts during
> console writes") the calls to manipulate the secondary interrupts, which
> were done without holding the port lock, can lead to the receiver being
> left disabled when set_termios() races with the console code (e.g. when
> init opens the tty during boot).
>
> The calls to stop and start rx in set_termios() can similarly race with
> DMA completion and, for example, cause the DMA buffer to be unmapped
> twice or the mapping to be leaked.
>
> Fixes: 6f3c3cafb115 ("serial: qcom-geni: disable interrupts during console writes")
> Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Cc: stable@vger.kernel.org      # 6.3
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index dea688db0d7c..5b6c5388efee 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1179,6 +1179,11 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
>                 if (ret)
>                         return ret;
>         }
> +
> +       uart_port_lock_irq(uport);
> +       qcom_geni_serial_start_rx(uport);
> +       uart_port_unlock_irq(uport);

I _think_ you don't need the locking here. The documentation for the
"startup" callback say:

 * Interrupts: globally disabled.

Other than that, this looks reasonable to me. I seem to recall
previous discussions where _someone_ was relying on the
qcom_geni_serial_start_rx() at the end of termios for some reason
(which always felt like a bad design), but I can't find those old
discussions. I suspect that the fact that you've added the start_rx in
startup() is what we needed.

Re: [PATCH v2 1/7] serial: qcom-geni: fix premature receiver enable

[Johan Hovold]

On Tue, Oct 01, 2024 at 07:20:36PM +0530, Mukesh Kumar Savaliya wrote:
> Thanks Johan for the fixes.

Thanks for taking a look.

> On 10/1/2024 6:20 PM, Johan Hovold wrote:
> > The receiver should not be enabled until the port is opened so drop the
> > bogus call to start rx from the setup code which is shared with the
> > console implementation.
> > 
> > This was added for some confused implementation of hibernation support,
> > but the receiver must not be started unconditionally as the port may not
> > have been open when hibernating the system.
> > 
> > Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> > Cc:stable@vger.kernel.org	# 6.2
> > Cc: Aniket Randive<quic_arandive@quicinc.com>
> > Signed-off-by: Johan Hovold<johan+linaro@kernel.org>

> > @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> >   			       false, true, true);
> >   	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
> >   	geni_se_select_mode(&port->se, port->dev_data->mode);
> > -	qcom_geni_serial_start_rx(uport);

> Does it mean hibernation will break now ? Not sure if its tested with 
> hibernation. I can see this call was added to port_setup specifically 
> for hibernation but now after removing it, where is it getting fixed ?
> I think RX will not be initialized after hibernation.

Correct. As I alluded to in the commit message this "hibernation
support" is quite broken already, but I was trying to avoid spending
more time on this driver than I already have and just look the other way
for the time being.

Note that rx is enabled by the serial core resume code, but then this
hibernation hack added a call to the setup the port after resuming it,
which would disable rx again were it not for this random call to
start rx, which should never have been added here in the first place.

But as these platforms do not support hibernation in mainline, and the
code broken anyway, I'll just rip it all out for v3.

Johan

Re: [PATCH v2 1/7] serial: qcom-geni: fix premature receiver enable

[Johan Hovold]

On Thu, Oct 03, 2024 at 11:29:58AM -0700, Doug Anderson wrote:
> On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > The receiver should not be enabled until the port is opened so drop the
> > bogus call to start rx from the setup code which is shared with the
> > console implementation.
> >
> > This was added for some confused implementation of hibernation support,
> > but the receiver must not be started unconditionally as the port may not
> > have been open when hibernating the system.
> 
> Could you provide a motivation for your patch in the description? Is
> patch needed for something (perhaps a future patch in the series)? Is
> it fixing a bug? Does it save power? Is the call harmless but cleaner
> to get rid of?

I was trying to bring some order to this driver so that the receiver is
enabled when the port is opened and disabled when it is closed again as
expected, and get rid of the random calls added in places where they do
not belong (e.g., as Bjorn also mentioned, why was the call to start rx
added in the port setup code if it was needed for hibernation?).

Data "received" over the wire before opening the port should not be
processed, but it also turns out that enabling the receiver before the
port is opened can confuse the firmware and break the "stale" rx timer
handling so that data is only forwarded in chunks of 12 bytes instead of
when each char is received.

> > Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> > Cc: stable@vger.kernel.org      # 6.2
> > Cc: Aniket Randive <quic_arandive@quicinc.com>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/tty/serial/qcom_geni_serial.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 6f0db310cf69..9ea6bd09e665 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> >                                false, true, true);
> >         geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
> >         geni_se_select_mode(&port->se, port->dev_data->mode);
> > -       qcom_geni_serial_start_rx(uport);
> 
> FWIW, I found at least one thing that's broken by your patch. If you
> enable kgdb (but _not_ "kgdboc_earlycon") and then add "kgdbwait" to
> the kernel command line parameters then things will be broken after
> your patch. You'll drop into the debugger but can't interact with it.
> The "kgdboc_earlycon" path handles this because of
> "qcom_geni_serial_enable_early_read()" but it doesn't seem like
> there's anything that handles it for normal kgdb. If you drop in the
> debugger later it'll probably work if you've got an "agetty" running
> because that'll enable the RX path.

Ok, so the kgdb has started relying on this call since d8851a96ba25
("tty: serial: qcom-geni-serial: Add a poll_init() function"). Thanks
for pointing that out.

The polled console code should not be calling the port setup code
unconditionally anyway so I'll fix this up as well in v3.

Johan

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Johan Hovold]

On Thu, Oct 03, 2024 at 11:30:08AM -0700, Doug Anderson wrote:
> On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > A commit adding back the stopping of tx on port shutdown failed to add
> > back the locking which had also been removed by commit e83766334f96
> > ("tty: serial: qcom_geni_serial: No need to stop tx/rx on UART
> > shutdown").
> 
> Hmmm, when I look at that commit it makes me think that the problem
> that commit e83766334f96 ("tty: serial: qcom_geni_serial: No need to
> stop tx/rx on UART shutdown") was fixing was re-introduced by commit
> d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in
> progress at shutdown"). ...and indeed, it was. :(
> 
> I can't interact with kgdb if I do this:
> 
> 1. ssh over to DUT
> 2. Kill the console process (on ChromeOS stop console-ttyMSM0)
> 3. Drop in the debugger (echo g > /proc/sysrq-trigger)

Yeah, don't do that then. ;)

Not sure how your "console process" works, but this should only happen
if you do not enable the serial console (console=ttyMSM0) and then try
to use a polled console (as enabling the console will prevent port
shutdown from being called). That should probably just be disallowed.

The console code, and the polled console code bolted on top, is a bit of
a hack so corner cases like this are to be expected.

When the polled console code was introduced it was claimed that it would
have "absolutely zero impact as long as CONFIG_CONSOLE_POLL is
disabled". Perhaps I'm reading too much into it, but that statement is
clearly ignoring the maintenance cost...

Johan

Re: [PATCH v2 4/7] serial: qcom-geni: fix receiver enable

[Johan Hovold]

On Thu, Oct 03, 2024 at 01:10:47PM -0700, Doug Anderson wrote:
> On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:

> > @@ -1179,6 +1179,11 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
> >                 if (ret)
> >                         return ret;
> >         }
> > +
> > +       uart_port_lock_irq(uport);
> > +       qcom_geni_serial_start_rx(uport);
> > +       uart_port_unlock_irq(uport);
> 
> I _think_ you don't need the locking here. The documentation for the
> "startup" callback say:
> 
>  * Interrupts: globally disabled.

Heh, yeah, that comment dates back to 2002 and probably wasn't even
correct back then.

This function is called with the port mutex held (and interrupts
enabled), and I need to take the port lock to serialise against the
console code.

> Other than that, this looks reasonable to me. I seem to recall
> previous discussions where _someone_ was relying on the
> qcom_geni_serial_start_rx() at the end of termios for some reason
> (which always felt like a bad design), but I can't find those old
> discussions. I suspect that the fact that you've added the start_rx in
> startup() is what we needed.

Yeah, I tried to find a reason for why things were done this way, but it
was probably just copied from the vendor driver. The hardware doesn't
seem to require stopping rx in set_termios() (and tx is not stopped
anyway), which could otherwise have been a reason.

Johan

Re: [PATCH v2 5/7] serial: qcom-geni: fix rx cancel dma status bit

[Johan Hovold]

On Thu, Oct 03, 2024 at 12:55:39PM -0700, Doug Anderson wrote:
> On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:

> >  #define RX_DMA_PARITY_ERR              BIT(5)
> >  #define RX_DMA_BREAK                   GENMASK(8, 7)
> >  #define RX_GENI_GP_IRQ                 GENMASK(10, 5)
> > -#define RX_GENI_CANCEL_IRQ             BIT(11)
> >  #define RX_GENI_GP_IRQ_EXT             GENMASK(13, 12)
> > +#define RX_GENI_CANCEL_IRQ             BIT(14)
> 
> This looks right, but do you want to fix all the rest of the wrong
> bits in this list while you're at it? Things look OK up to the
> "RX_FLUSH_DONE" and then they're wrong. Specifically:
> 
> * My datasheet doesn't have RX_DMA_PARITY_ERR. Unless maybe it's one
> of the "GP" IRQs?

As you noticed, this one appears to be correct.

> * My datassheet doesn't have RX_DMA_BREAK. Unless maybe it's one of
> the "GP" IRQs (though why would it be two bits big?)

And same here, apparently one is break on and the other break off.

> * RX_GENI_GP_IRQ is 12:5, not 10:5
> 
> * My datasheet has RX_GENI_CMD_FAILURE as BIT(15).

I'll just leave the rest as is for now.

Johan

Re: [PATCH v2 6/7] serial: qcom-geni: drop flip buffer WARN()

[Johan Hovold]

On Thu, Oct 03, 2024 at 01:06:43PM -0700, Doug Anderson wrote:
> On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:

> > @@ -570,9 +570,8 @@ static void handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
> >
> >         ret = tty_insert_flip_string(tport, port->rx_buf, bytes);
> >         if (ret != bytes) {
> > -               dev_err(uport->dev, "%s:Unable to push data ret %d_bytes %d\n",
> > -                               __func__, ret, bytes);
> > -               WARN_ON_ONCE(1);
> > +               dev_err_ratelimited(uport->dev, "failed to push data (%d < %u)\n",
> > +                               ret, bytes);
> 
> Not that it really matters, but since you're fixing the type of
> "bytes" to %u you probably should fix "ret" to %u too, which means
> changing the type of it? Officially tty_insert_flip_string returns the
> (unsigned) size_t.

Yeah, that was changed recently, but apparently not all callers were
updated. I'll just leave this as is for now too.
 
> As a nit, I'd also say that your error message shouldn't assert "<"
> unless you change your "if" test to "<". It seems safer to use != so
> IMO the printout should also say "!=".

Possibly, but if we ever hit that we have bigger problems.

> I'd hope you're not hitting this error a lot because it means you're
> dropping bytes, but getting rid of the WARN_ON and changing to
> ratelimited makes sense to me.

No, this was just something I noticed when reviewing the function.

Johan

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Doug Anderson]

Hi,

On Wed, Oct 9, 2024 at 7:10 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Oct 03, 2024 at 11:30:08AM -0700, Doug Anderson wrote:
> > On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> > >
> > > A commit adding back the stopping of tx on port shutdown failed to add
> > > back the locking which had also been removed by commit e83766334f96
> > > ("tty: serial: qcom_geni_serial: No need to stop tx/rx on UART
> > > shutdown").
> >
> > Hmmm, when I look at that commit it makes me think that the problem
> > that commit e83766334f96 ("tty: serial: qcom_geni_serial: No need to
> > stop tx/rx on UART shutdown") was fixing was re-introduced by commit
> > d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in
> > progress at shutdown"). ...and indeed, it was. :(
> >
> > I can't interact with kgdb if I do this:
> >
> > 1. ssh over to DUT
> > 2. Kill the console process (on ChromeOS stop console-ttyMSM0)
> > 3. Drop in the debugger (echo g > /proc/sysrq-trigger)
>
> Yeah, don't do that then. ;)

The problem is, I don't always have a choice. As talked about in the
message of commit e83766334f96 ("tty: serial: qcom_geni_serial: No
need to stop tx/rx on UART shutdown"), the above steps attempt to
simulate what happened organically: a crash in late shutdown. During
shutdown the agetty has been killed by the init system and I don't
have a choice about it. If I get a kernel crash then (which isn't
uncommon since shutdown code tends to trigger seldom-used code paths)
then I can't debug it. :(

We need to fix this.


> Not sure how your "console process" works, but this should only happen
> if you do not enable the serial console (console=ttyMSM0) and then try
> to use a polled console (as enabling the console will prevent port
> shutdown from being called).

That simply doesn't seem to be the case for me. The port shutdown
seems to be called. To confirm, I put a printout at the start of
qcom_geni_serial_shutdown(). I see in my /proc/cmdline:

console=ttyMSM0,115200n8

...and I indeed verify that I see console messages on my UART. I then run:

stop console-ttyMSM0

...and I see on the UART:

[   92.916964] DOUG: qcom_geni_serial_shutdown
[   92.922703] init: console-ttyMSM0 main process (611) killed by TERM signal

Console messages keep coming out the UART even though the agetty isn't
there. Now I (via ssh) drop into the debugger:

echo g > /proc/sysrq-trigger

I see the "kgdb" prompt but I can't interact with it because
qcom_geni_serial_shutdown() stopped RX.


-Doug

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Johan Hovold]

On Thu, Oct 10, 2024 at 03:30:05PM -0700, Doug Anderson wrote:
> On Wed, Oct 9, 2024 at 7:10 AM Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Oct 03, 2024 at 11:30:08AM -0700, Doug Anderson wrote:

> > > Hmmm, when I look at that commit it makes me think that the problem
> > > that commit e83766334f96 ("tty: serial: qcom_geni_serial: No need to
> > > stop tx/rx on UART shutdown") was fixing was re-introduced by commit
> > > d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in
> > > progress at shutdown"). ...and indeed, it was. :(
> > >
> > > I can't interact with kgdb if I do this:
> > >
> > > 1. ssh over to DUT
> > > 2. Kill the console process (on ChromeOS stop console-ttyMSM0)
> > > 3. Drop in the debugger (echo g > /proc/sysrq-trigger)
> >
> > Yeah, don't do that then. ;)
> 
> The problem is, I don't always have a choice. As talked about in the
> message of commit e83766334f96 ("tty: serial: qcom_geni_serial: No
> need to stop tx/rx on UART shutdown"), the above steps attempt to
> simulate what happened organically: a crash in late shutdown. During
> shutdown the agetty has been killed by the init system and I don't
> have a choice about it. If I get a kernel crash then (which isn't
> uncommon since shutdown code tends to trigger seldom-used code paths)
> then I can't debug it. :(

Ok, thanks for clarifying.

> > Not sure how your "console process" works, but this should only happen
> > if you do not enable the serial console (console=ttyMSM0) and then try
> > to use a polled console (as enabling the console will prevent port
> > shutdown from being called).
> 
> That simply doesn't seem to be the case for me. The port shutdown
> seems to be called. To confirm, I put a printout at the start of
> qcom_geni_serial_shutdown(). I see in my /proc/cmdline:
> 
> console=ttyMSM0,115200n8
> 
> ...and I indeed verify that I see console messages on my UART. I then run:
> 
> stop console-ttyMSM0
> 
> ...and I see on the UART:
> 
> [   92.916964] DOUG: qcom_geni_serial_shutdown
> [   92.922703] init: console-ttyMSM0 main process (611) killed by TERM signal
> 
> Console messages keep coming out the UART even though the agetty isn't
> there.

And this is with a Chromium kernel, not mainline? 

If you take a look at tty_port_shutdown() there's a hack in there for
consoles that was added back in 2010 and that prevents shutdown() from
called for console ports.

Put perhaps you manage to hit shutdown() via some other path. Serial
core is not yet using tty_port_hangup() so a hangup might trigger
that...

Could you check that with a dump_stack()?

> Now I (via ssh) drop into the debugger:
> 
> echo g > /proc/sysrq-trigger
> 
> I see the "kgdb" prompt but I can't interact with it because
> qcom_geni_serial_shutdown() stopped RX.

How about simply amending poll_get_char() so that it enables the
receiver if it's not already enabled?

Johan

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Doug Anderson]

Hi,

On Thu, Oct 10, 2024 at 11:51 PM Johan Hovold <johan@kernel.org> wrote:
>
> > > Not sure how your "console process" works, but this should only happen
> > > if you do not enable the serial console (console=ttyMSM0) and then try
> > > to use a polled console (as enabling the console will prevent port
> > > shutdown from being called).
> >
> > That simply doesn't seem to be the case for me. The port shutdown
> > seems to be called. To confirm, I put a printout at the start of
> > qcom_geni_serial_shutdown(). I see in my /proc/cmdline:
> >
> > console=ttyMSM0,115200n8
> >
> > ...and I indeed verify that I see console messages on my UART. I then run:
> >
> > stop console-ttyMSM0
> >
> > ...and I see on the UART:
> >
> > [   92.916964] DOUG: qcom_geni_serial_shutdown
> > [   92.922703] init: console-ttyMSM0 main process (611) killed by TERM signal
> >
> > Console messages keep coming out the UART even though the agetty isn't
> > there.
>
> And this is with a Chromium kernel, not mainline?

Who do you take me for?!?!  :-P :-P :-P Of course it's with mainline.


> If you take a look at tty_port_shutdown() there's a hack in there for
> consoles that was added back in 2010 and that prevents shutdown() from
> called for console ports.
>
> Put perhaps you manage to hit shutdown() via some other path. Serial
> core is not yet using tty_port_hangup() so a hangup might trigger
> that...
>
> Could you check that with a dump_stack()?

Sure. Typed from the agetty itself, here ya go. Git hash is not a
mainline git hash because I have your patches applied. "dirty" is
because of the printout / dump_stack().

lazor-rev9 ~ # stop console-ttyMSM0
[   68.772828] DOUG: qcom_geni_serial_shutdown
[   68.777365] CPU: 2 UID: 0 PID: 589 Comm: login Not tainted
6.12.0-rc1-g0bde0d120d58-dirty #1
ac8ed1a05abcc73f4fafe0543cbc76768ea594e1
[   68.789737] Hardware name: Google Lazor (rev9) with LTE (DT)
[   68.795581] Call trace:
[   68.798124]  dump_backtrace+0xf8/0x120
[   68.802025]  show_stack+0x24/0x38
[   68.805463]  dump_stack_lvl+0x40/0xc8
[   68.809265]  dump_stack+0x18/0x38
[   68.812702]  qcom_geni_serial_shutdown+0x38/0x110
[   68.817578]  uart_port_shutdown+0x48/0x68
[   68.821736]  uart_shutdown+0xcc/0x170
[   68.825530]  uart_hangup+0x54/0x158
[   68.829154]  __tty_hangup+0x20c/0x318
[   68.832954]  tty_vhangup_session+0x20/0x38
[   68.837195]  disassociate_ctty+0xe8/0x1a8
[   68.841355]  do_exit+0x10c/0x358
[   68.844716]  do_group_exit+0x9c/0xa8
[   68.848441]  get_signal+0x408/0x4d8
[   68.852071]  do_signal+0xa8/0x770
[   68.855526]  do_notify_resume+0x78/0x118
[   68.859605]  el0_svc+0x64/0x68
[   68.862790]  el0t_64_sync_handler+0x20/0x128
[   68.867218]  el0t_64_sync+0x1a8/0x1b0
[   68.872933] init: console-ttyMSM0 main process (589) killed by TERM signal


> > Now I (via ssh) drop into the debugger:
> >
> > echo g > /proc/sysrq-trigger
> >
> > I see the "kgdb" prompt but I can't interact with it because
> > qcom_geni_serial_shutdown() stopped RX.
>
> How about simply amending poll_get_char() so that it enables the
> receiver if it's not already enabled?

Yeah, this would probably work.

-Doug

Re: [PATCH v2 2/7] serial: qcom-geni: fix shutdown race

[Johan Hovold]

On Fri, Oct 11, 2024 at 07:30:30AM -0700, Doug Anderson wrote:
> On Thu, Oct 10, 2024 at 11:51 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > > > Not sure how your "console process" works, but this should only happen
> > > > if you do not enable the serial console (console=ttyMSM0) and then try
> > > > to use a polled console (as enabling the console will prevent port
> > > > shutdown from being called).

> > And this is with a Chromium kernel, not mainline?
> 
> Who do you take me for?!?!  :-P :-P :-P Of course it's with mainline.

Heh. Just checking. I was sure about shutdown() not being called when
closing ports, but yeah, you can indeed hit this via hangup() as serial
core was only half-converted over to use the tty port implementation in
2016.

> > If you take a look at tty_port_shutdown() there's a hack in there for
> > consoles that was added back in 2010 and that prevents shutdown() from
> > called for console ports.
> >
> > Put perhaps you manage to hit shutdown() via some other path. Serial
> > core is not yet using tty_port_hangup() so a hangup might trigger
> > that...
> >
> > Could you check that with a dump_stack()?

> lazor-rev9 ~ # stop console-ttyMSM0

> [   68.812702]  qcom_geni_serial_shutdown+0x38/0x110
> [   68.817578]  uart_port_shutdown+0x48/0x68
> [   68.821736]  uart_shutdown+0xcc/0x170
> [   68.825530]  uart_hangup+0x54/0x158
> [   68.829154]  __tty_hangup+0x20c/0x318
> [   68.832954]  tty_vhangup_session+0x20/0x38
> [   68.837195]  disassociate_ctty+0xe8/0x1a8
> [   68.841355]  do_exit+0x10c/0x358
> [   68.844716]  do_group_exit+0x9c/0xa8
> [   68.848441]  get_signal+0x408/0x4d8
> [   68.852071]  do_signal+0xa8/0x770

Thanks for confirming. I see this too when stopping a getty.

> > > Now I (via ssh) drop into the debugger:
> > >
> > > echo g > /proc/sysrq-trigger
> > >
> > > I see the "kgdb" prompt but I can't interact with it because
> > > qcom_geni_serial_shutdown() stopped RX.
> >
> > How about simply amending poll_get_char() so that it enables the
> > receiver if it's not already enabled?
> 
> Yeah, this would probably work.

Seems we should clean up serial core so that it at least behaves
consistently on hangup and close.

Having someone think trough and document how these polled consoles are
supposed to work would also be good and save people modifying these
drivers a lot of work.

If they are restricted to when the console is active, there would be no
need for most of poll_init(), and we already prevent the console from
being shut down on hangup() and close().

And then we now also have the detachable console mess to consider...

Johan