Re: [PATCH v2 2/2] tty: serial: msm_serial: Fix deadlock caused by recursive output

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 02, 2019 at 09:40:55AM -0700, Jeffrey Hugo wrote:
> On Wed, Nov 27, 2019 at 7:16 AM Leo Yan <leo.yan@xxxxxxxxxx> 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.

Thanks a lot for your reviewing, Jeffrey.

Agreed with you for the concern.

To be honest, I am not familiar with the msm uart driver, so have no
confidence which is the best way for uart port operations.  I can
think out one possible fixing is shown in below, if detects the lock
is not acquired then it will force to reset UART port before exit the
function __msm_console_write().

This approach is not tested yet and it looks too arbitrary; I will
give a try for it.  At the meantime, welcome any insight suggestion
with proper register operations.

@@ -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);
 
@@ -1632,6 +1642,9 @@ static void __msm_console_write(struct uart_port *port, const char *s,
                i += num_chars;
        }
 
+       if (!locked && is_uartdm)
+               msm_reset(port);
+
        if (locked)
                spin_unlock(&port->lock);
 }

Thanks,
Leo

> >
> > 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@xxxxxxxxxx>
> > ---
> >  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
> >



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux