[6/8] serial: qcom-geni: fix console corruption

The Qualcomm serial console implementation is broken and can lose
characters when the serial port is also used for tty output.

Specifically, the console code only waits for the current tx command to
complete when all data has already been written to the fifo. When there
are on-going longer transfers this often means that console output is
lost when the console code inadvertently "hijacks" the current tx
command instead of starting a new one.

This can, for example, be observed during boot when console output that
should have been interspersed with init output is truncated:

	[    9.462317] qcom-snps-eusb2-hsphy fde000.phy: Registered Qcom-eUSB2 phy
	[  OK  ] Found device KBG50ZNS256G KIOXIA Wi[    9.471743ndows.
	[    9.539915] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller

Add a new state variable to track how much data has been written to the
fifo and use it to determine when the fifo and shift register are both
empty. This is needed since there is currently no other known way to
determine when the shift register is empty.

This in turn allows the console code to interrupt long transfers without
losing data.

Note that the oops-in-progress case is similarly broken as it does not
cancel any active command and also waits for the wrong status flag when
attempting to drain the fifo (TX_FIFO_NOT_EMPTY_EN is only set when
cancelling a command leaves data in the fifo).

Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Fixes: a1fee899e5be ("tty: serial: qcom_geni_serial: Fix softlock")
Fixes: 9e957a155005 ("serial: qcom-geni: Don't cancel/abort if we can't get the port lock")
Cc: stable@vger.kernel.org	# 4.17
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 48 ++++++++++++++-------------
 1 file changed, 25 insertions(+), 23 deletions(-)

Hi,

On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> +{
> +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> +
> +       if (!qcom_geni_serial_main_active(uport))
> +               return;

It seems like all callers already do the check and only ever call you
if the port is active. Do you really need to re-check?


> @@ -308,6 +311,17 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>         return qcom_geni_serial_poll_bitfield(uport, offset, field, set ? field : 0);
>  }
>
> +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> +{
> +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> +
> +       if (!qcom_geni_serial_main_active(uport))
> +               return;
> +
> +       qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GP_LENGTH,
> +                       port->tx_queued);

nit: indent "port->tx_queued" to match open parenthesis?

...also: as the kernel test robot reported, w/ certain CONFIGs this is
defined / not used.

Aside from the nit / robot issue, this solution looks reasonable to
me. It's been long enough that I've already paged out much of the past
digging I did into this driver, but this seems like it should work.
Feel free to add my Reviewed-by when the robot issue is fixed.



-Doug

On Wed, Sep 04, 2024 at 02:51:15PM -0700, Doug Anderson wrote:
> On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> > +{
> > +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +
> > +       if (!qcom_geni_serial_main_active(uport))
> > +               return;
> 
> It seems like all callers already do the check and only ever call you
> if the port is active. Do you really need to re-check?

I wanted to make the helper self-contained and work in both cases. But
since I ended up only using this helper only in the console code and
will need to move it anyway (under the console ifdef), perhaps I can
consider dropping it. But then again, it's just one register read.

> > @@ -308,6 +311,17 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> >         return qcom_geni_serial_poll_bitfield(uport, offset, field, set ? field : 0);
> >  }
> >
> > +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> > +{
> > +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +
> > +       if (!qcom_geni_serial_main_active(uport))
> > +               return;
> > +
> > +       qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GP_LENGTH,
> > +                       port->tx_queued);
> 
> nit: indent "port->tx_queued" to match open parenthesis?

No, I don't use open-parenthesis alignment unless that's the
(consistent) style of the code I'm changing (e.g. to avoid unnecessary
realignments when symbol names change and to make a point about
checkpatch --pedantic warnings not being part of the coding style).
 
> ...also: as the kernel test robot reported, w/ certain CONFIGs this is
> defined / not used.

Yes, I need to move the helper under the console ifdef. I was just
waiting to see if there was any further feedback before respinning.

> Aside from the nit / robot issue, this solution looks reasonable to
> me. It's been long enough that I've already paged out much of the past
> digging I did into this driver, but this seems like it should work.
> Feel free to add my Reviewed-by when the robot issue is fixed.

Thanks for reviewing.

Johan