Re: [PATCH v2 2/2] tty: serial: msm_serial: Fix deadlock caused by recursive output
From: Leo Yan
Date: Tue Dec 03 2019 - 03:23:36 EST
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
> >