linux-kernel - Re: [PATCH v3 6/7] serial: qcom-geni: Fix suspend while active UART xfer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=XDcFsStOcViTQLLQqPtbCOqeVeoU0T7fi81jhHodpa=A@mail.gmail.com>
Date: Mon, 10 Jun 2024 15:26:24 -0700
From: Doug Anderson <dianders@...omium.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, linux-arm-msm@...r.kernel.org, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, LKML <linux-kernel@...r.kernel.org>, 
	linux-serial <linux-serial@...r.kernel.org>, John Ogness <john.ogness@...utronix.de>, 
	Yicong Yang <yangyicong@...ilicon.com>, Tony Lindgren <tony@...mide.com>, 
	Stephen Boyd <swboyd@...omium.org>, Johan Hovold <johan+linaro@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
Subject: Re: [PATCH v3 6/7] serial: qcom-geni: Fix suspend while active UART xfer

Hi,

On Fri, Jun 7, 2024 at 12:43 AM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> > @@ -311,11 +312,14 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> >
> >  static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
> >  {
> > +     struct qcom_geni_serial_port *port = to_dev_port(uport);
> >       u32 m_cmd;
> >
> >       writel(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
> >       m_cmd = UART_START_TX << M_OPCODE_SHFT;
>
> Unrelated to this patch and won't belong to this patch but I noticed it
> while reviewing. This could be converted into:
>
>         m_cmd = FIELD_PREP(M_OPCODE_MSK, UART_START_TX);
>
> (and after converting the other use in the header file, the SHFT define
> becomes unused).

Sure. I'm going to leave that to someone in the future, though. I've
already spent more time than I should on this series and, if we're
going to do this, we should convert the whole driver (and perhaps all
the geni drivers).


> > @@ -335,6 +339,64 @@ static void qcom_geni_serial_poll_tx_done(struct uart_port *uport)
> >       writel(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
> >  }
> >
> > +static void qcom_geni_serial_drain_tx_fifo(struct uart_port *uport)
> > +{
> > +     struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +
> > +     /*
> > +      * If the main sequencer is inactive it means that the TX command has
> > +      * been completed and all bytes have been sent. Nothing to do in that
> > +      * case.
> > +      */
> > +     if (!qcom_geni_serial_main_active(uport))
> > +             return;
> > +
> > +     /*
> > +      * Wait until the FIFO has been drained. We've already taken bytes out
> > +      * of the higher level queue in qcom_geni_serial_send_chunk_fifo() so
> > +      * if we don't drain the FIFO but send the "cancel" below they seem to
> > +      * get lost.
> > +      */
> > +     qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GENMASK(31, 0),
>
> That GENMASK(31, 0) is a field in a register (even if it covers the
> entire register)? It should be named with a define instead of creating the
> field mask here in an online fashion.

Sure. Done.


> > +                                    port->tx_total - port->tx_remaining);
> > +
> > +     /*
> > +      * If clearing the FIFO made us inactive then we're done--no need for
> > +      * a cancel.
> > +      */
> > +     if (!qcom_geni_serial_main_active(uport))
> > +             return;
> > +
> > +     /*
> > +      * Cancel the current command. After this the main sequencer will
> > +      * stop reporting that it's active and we'll have to start a new
> > +      * transfer command.
> > +      *
> > +      * If we skip doing this cancel and then continue with a system
> > +      * suspend while there's an active command in the main sequencer
> > +      * then after resume time we won't get any more interrupts on the
> > +      * main sequencer until we send the cancel.
> > +      */
> > +     geni_se_cancel_m_cmd(&port->se);
> > +     if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> > +                                    M_CMD_CANCEL_EN, true)) {
> > +             /* The cancel failed; try an abort as a fallback. */
> > +             geni_se_abort_m_cmd(&port->se);
> > +             qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> > +                                             M_CMD_ABORT_EN, true);
>
> Misaligned.

Done.