Message-ID: <004b1060-3217-4ab9-805e-c1b8c95b119c@linaro.org>
Date: Mon, 17 Jun 2024 21:02:20 +0200
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Douglas Anderson <dianders@...omium.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jiri Slaby <jirislaby@...nel.org>
Cc: Yicong Yang <yangyicong@...ilicon.com>, Tony Lindgren <tony@...mide.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Johan Hovold <johan+linaro@...nel.org>,
 John Ogness <john.ogness@...utronix.de>, linux-arm-msm@...r.kernel.org,
 Bjorn Andersson <andersson@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Stephen Boyd <swboyd@...omium.org>, linux-serial@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v4 7/8] serial: qcom-geni: Fix suspend while active UART
 xfer



On 6/11/24 00:24, Douglas Anderson wrote:
> On devices using Qualcomm's GENI UART it is possible to get the UART
> stuck such that it no longer outputs data. Specifically, logging in
> via an agetty on the debug serial port (which was _not_ used for
> kernel console) and running:
>    cat /var/log/messages
> ...and then (via an SSH session) forcing a few suspend/resume cycles
> causes the UART to stop transmitting.
> 
> The root of the problems was with qcom_geni_serial_stop_tx_fifo()
> which is called as part of the suspend process. Specific problems with
> that function:
> - When an in-progress "tx" command is cancelled it doesn't appear to
>    fully drain the FIFO. That meant qcom_geni_serial_tx_empty()
>    continued to report that the FIFO wasn't empty. The
>    qcom_geni_serial_start_tx_fifo() function didn't re-enable
>    interrupts in this case so the driver would never start transferring
>    again.
> - When the driver cancelled the current "tx" command but it forgot to
>    zero out "tx_remaining". This confused logic elsewhere in the
>    driver.
> - From experimentation, it appears that cancelling the "tx" command
>    could drop some of the queued up bytes.
> 
> While qcom_geni_serial_stop_tx_fifo() could be fixed to drain the FIFO
> and shut things down properly, stop_tx() isn't supposed to be a slow
> function. It is run with local interrupts off and is documented to
> stop transmitting "as soon as possible". Change the function to just
> stop new bytes from being queued. In order to make this work, change
> qcom_geni_serial_start_tx_fifo() to remove some conditions. It's
> always safe to enable the watermark interrupt and the IRQ handler will
> disable it if it's not needed.
> 
> For system suspend the queue still needs to be drained. Failure to do
> so means that the hardware won't provide new interrupts until a
> "cancel" command is sent. Add draining logic (fixing the issues noted
> above) at suspend time.
> 
> NOTE: It would be ideal if qcom_geni_serial_stop_tx_fifo() could
> "pause" the transmitter right away. There is no obvious way to do this
> in the docs and experimentation didn't find any tricks either, so
> stopping TX "as soon as possible" isn't very soon but is the best
> possible.
> 
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---

This all looks good in my eyes, with the assumption that sending an ABORT
can't somehow be rejected by the hardware.. I wouldn't normally think of
that, but GENI is peculiar at times

Reviewed-by: Konrad Dybcio <konrad.dybcio@...aro.org>

Konrad

Powered by blists - more mailing lists