Re: [PATCH v2 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops

[Leo Yan]

Hi Doug,

On Thu, Dec 05, 2019 at 08:40:20AM +0800, Doug Anderson wrote:
> Hi,
> 
> On Wed, Nov 27, 2019 at 10:16 PM Leo Yan <leo.yan@xxxxxxxxxx> wrote:
> >
> > As the commit 677fe555cbfb ("serial: imx: Fix recursive locking bug")
> > has mentioned the uart driver might cause recursive locking between
> > normal printing and the kernel debugging facilities (e.g. sysrq and
> > oops).  In the commit it gave out suggestion for fixing recursive
> > locking issue: "The solution is to avoid locking in the sysrq case
> > and trylock in the oops_in_progress case."
> >
> > This patch follows the suggestion (also used the exactly same code with
> > other serial drivers, e.g. amba-pl011.c) to fix the recursive locking
> > issue, this can avoid stuck caused by deadlock and print out log for
> > sysrq and oops.
> >
> > Fixes: 04896a77a97b ("msm_serial: serial driver for MSM7K onboard serial peripheral.")
> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > ---
> >  drivers/tty/serial/msm_serial.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> > index 3657a24913fc..889538182e83 100644
> > --- a/drivers/tty/serial/msm_serial.c
> > +++ b/drivers/tty/serial/msm_serial.c
> > @@ -1576,6 +1576,7 @@ static void __msm_console_write(struct uart_port *port, const char *s,
> >         int num_newlines = 0;
> >         bool replaced = false;
> >         void __iomem *tf;
> > +       int locked = 1;
> >
> >         if (is_uartdm)
> >                 tf = port->membase + UARTDM_TF;
> > @@ -1588,7 +1589,13 @@ static void __msm_console_write(struct uart_port *port, const char *s,
> >                         num_newlines++;
> >         count += num_newlines;
> >
> > -       spin_lock(&port->lock);
> > +       if (port->sysrq)
> > +               locked = 0;
> > +       else if (oops_in_progress)
> > +               locked = spin_trylock(&port->lock);
> > +       else
> > +               spin_lock(&port->lock);
> 
> I don't have tons of experience with the "msm" serial driver, but the
> above snippet tickled a memory in my brain for when I was looking at
> the "qcom_geni" serial driver, which is a close cousin.

Good point and thanks for sharing info.

> I seemed to remember that the "if (port->sysrq)" was something you
> didn't want.  ...but maybe that's only if you do something like commit
> 336447b3298c ("serial: qcom_geni_serial: Process sysrq at port unlock
> time")?

The patch you mentioned can allow to read sysrq chars without
acquiring port lock.

> Any way you can try making a similar change to the msm driver
> and see if it allow you to remove the special case for "port->sysrq"?

I was deliberately to add the case for "port->sysrq" in the function
__msm_console_write() when I prepared this patch.

Let's see the issue obersved: when the UART drive is deadlock by any
reason, when sent break + h to test system is alive or not, sysrq
tries to ouput log by calling __msm_console_write(), it tries to
acquire console's spinlock but also run into deadlock; finally it
doesn't have chance to output log for sysrq.

P.s. Sorry my long late and didn't follow good practice to reply
quickly due worked on other works.

Thanks,
Leo Yan