| Message ID | 20241009145110.16847-6-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | serial: qcom-geni: fix receiver enable | expand |
Hi, On Wed, Oct 9, 2024 at 7:51 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The receiver is supposed to be enabled in the startup() callback and not > in set_termios() which is called also during console setup. > > This specifically avoids accepting input before the port has been opened > (and interrupts enabled), something which can also break the GENI > firmware (cancel fails and after abort, the "stale" counter handling > appears to be broken so that later input is not processed until twelve > chars have been received). > > There also does not appear to be any need to keep the receiver disabled > while updating the port settings. > > 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 also lead to the receiver > being left disabled when set_termios() races with the console code (e.g. > when init opens the tty during boot). This can manifest itself as a > serial getty not accepting input. > > 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. > > Fix this by only enabling the receiver during startup and while holding > the port lock to avoid racing with the console code. > > 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(-) Reviewed-by: Douglas Anderson <dianders@chromium.org>
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index aaf24bd037a7..6c4349ea5720 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -1197,6 +1197,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; @@ -1282,7 +1287,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); @@ -1298,7 +1302,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", @@ -1389,8 +1393,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
The receiver is supposed to be enabled in the startup() callback and not in set_termios() which is called also during console setup. This specifically avoids accepting input before the port has been opened (and interrupts enabled), something which can also break the GENI firmware (cancel fails and after abort, the "stale" counter handling appears to be broken so that later input is not processed until twelve chars have been received). There also does not appear to be any need to keep the receiver disabled while updating the port settings. 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 also lead to the receiver being left disabled when set_termios() races with the console code (e.g. when init opens the tty during boot). This can manifest itself as a serial getty not accepting input. 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. Fix this by only enabling the receiver during startup and while holding the port lock to avoid racing with the console code. 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(-)