Date:   Mon, 2 Dec 2019 09:40:55 -0700
From:   Jeffrey Hugo <jeffrey.l.hugo@...il.com>
To:     Leo Yan <leo.yan@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Nicolas Dechesne <nicolas.dechesne@...aro.org>,
        MSM <linux-arm-msm@...r.kernel.org>,
        linux-serial@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] tty: serial: msm_serial: Fix deadlock caused by
 recursive output

On Wed, Nov 27, 2019 at 7:16 AM Leo Yan <leo.yan@...aro.org> wrote:
>
> The uart driver might run into deadlock caused by recursive output; the
> basic flow is after uart driver has acquired the port spinlock, then it
> invokes some functions, e.g. dma engine operations and allocate dma
> descriptor with kzalloc(), if the system has very less free memory, the
> kernel will give out warning by printing logs, thus recursive output
> will happen and at the second time the attempting to acquire lock will
> cause deadlock.  The detailed flow is shown as below:
>
>   msm_uart_irq()
>     spin_lock_irqsave(&port->lock, flags)  => First time to acquire lock
>     msm_handle_tx(port)
>       msm_handle_tx_dma()
>         dmaengine_prep_slave_single()
>           bam_prep_slave_sg()
>             kzalloc()
>                __kmalloc()
>                  ___slab_alloc()
>                    alloc_pages_current()
>                      __alloc_pages_nodemask()
>                        warn_alloc()
>                          printk()
>                            msm_console_write()
>                              __msm_console_write()
>                                spin_lock(&port->lock) => Cause deadlock
>
> This patch fixes the deadlock issue for recursive output; it adds a
> variable 'curr_user' to indicate the uart port is used by which CPU, if
> the CPU has acquired spinlock and wants to execute recursive output,
> it will directly bail out.  Here we don't choose to avoid locking and
> print out log, the reason is in this case we don't want to reset the
> uart port with function msm_reset_dm_count(); otherwise it can introduce
> confliction with other flows and results in uart port malfunction and
> later cannot output anymore.

Is this not fixable?  Sure, fixing the deadlock is an improvement, but
dropping logs (particularly a memory warning like in your example)
seems undesirable.

>
> Fixes: 99693945013a ("tty: serial: msm: Add RX DMA support")
> Fixes: 3a878c430fd6 ("tty: serial: msm: Add TX DMA support")
> Signed-off-by: Leo Yan <leo.yan@...aro.org>
> ---
>  drivers/tty/serial/msm_serial.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index 889538182e83..06076cd2948f 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -182,6 +182,7 @@ struct msm_port {
>         bool                    break_detected;
>         struct msm_dma          tx_dma;
>         struct msm_dma          rx_dma;
> +       struct cpumask          curr_user;
>  };
>
>  #define UART_TO_MSM(uart_port) container_of(uart_port, struct msm_port, uart)
> @@ -436,6 +437,7 @@ static void msm_complete_tx_dma(void *args)
>         u32 val;
>
>         spin_lock_irqsave(&port->lock, flags);
> +       cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
>
>         /* Already stopped */
>         if (!dma->count)
> @@ -470,6 +472,7 @@ static void msm_complete_tx_dma(void *args)
>
>         msm_handle_tx(port);
>  done:
> +       cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
>         spin_unlock_irqrestore(&port->lock, flags);
>  }
>
> @@ -544,6 +547,7 @@ static void msm_complete_rx_dma(void *args)
>         u32 val;
>
>         spin_lock_irqsave(&port->lock, flags);
> +       cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
>
>         /* Already stopped */
>         if (!dma->count)
> @@ -590,6 +594,7 @@ static void msm_complete_rx_dma(void *args)
>
>         msm_start_rx_dma(msm_port);
>  done:
> +       cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
>         spin_unlock_irqrestore(&port->lock, flags);
>
>         if (count)
> @@ -931,6 +936,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
>         u32 val;
>
>         spin_lock_irqsave(&port->lock, flags);
> +       cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
>         misr = msm_read(port, UART_MISR);
>         msm_write(port, 0, UART_IMR); /* disable interrupt */
>
> @@ -962,6 +968,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
>                 msm_handle_delta_cts(port);
>
>         msm_write(port, msm_port->imr, UART_IMR); /* restore interrupt */
> +       cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
>         spin_unlock_irqrestore(&port->lock, flags);
>
>         return IRQ_HANDLED;
> @@ -1572,6 +1579,7 @@ static inline struct uart_port *msm_get_port_from_line(unsigned int line)
>  static void __msm_console_write(struct uart_port *port, const char *s,
>                                 unsigned int count, bool is_uartdm)
>  {
> +       struct msm_port *msm_port = UART_TO_MSM(port);
>         int i;
>         int num_newlines = 0;
>         bool replaced = false;
> @@ -1593,6 +1601,8 @@ static void __msm_console_write(struct uart_port *port, const char *s,
>                 locked = 0;
>         else if (oops_in_progress)
>                 locked = spin_trylock(&port->lock);
> +       else if (cpumask_test_cpu(smp_processor_id(), &msm_port->curr_user))
> +               return;
>         else
>                 spin_lock(&port->lock);
>
> --
> 2.17.1
>

Powered by blists - more mailing lists