diff mbox

[v2,4/5] serial: mctrl_gpio: add IRQ locking

Message ID 1458652329-10138-5-git-send-email-yegorslists@googlemail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yegor Yefremov March 22, 2016, 1:12 p.m. UTC
From: Yegor Yefremov <yegorslists@googlemail.com>

uart_handle_cts_change() should be called in IRQ locked state, hence
use port->lock to disable interrupts.

CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
 drivers/tty/serial/serial_mctrl_gpio.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Yegor Yefremov April 4, 2016, 1:44 p.m. UTC | #1
Hi Uwe,

On Tue, Mar 22, 2016 at 2:12 PM,  <yegorslists@googlemail.com> wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> uart_handle_cts_change() should be called in IRQ locked state, hence
> use port->lock to disable interrupts.
>
> CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
>  drivers/tty/serial/serial_mctrl_gpio.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index bb5e4f9..85b77ba 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -143,6 +143,9 @@ static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
>         struct uart_port *port = gpios->port;
>         u32 mctrl = gpios->mctrl_prev;
>         u32 mctrl_diff;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&port->lock, flags);
>
>         mctrl_gpio_get(gpios, &mctrl);
>
> @@ -165,6 +168,8 @@ static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
>                 wake_up_interruptible(&port->state->port.delta_msr_wait);
>         }
>
> +       spin_unlock_irqrestore(&port->lock, flags);
> +
>         return IRQ_HANDLED;
>  }

Could you please test this patch with Atmel UART?

Should this patch be actually removed this series and sent separately?

Yegor
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König April 4, 2016, 3:16 p.m. UTC | #2
Hello Yegor,

On Mon, Apr 04, 2016 at 03:44:23PM +0200, Yegor Yefremov wrote:
> On Tue, Mar 22, 2016 at 2:12 PM,  <yegorslists@googlemail.com> wrote:
> > From: Yegor Yefremov <yegorslists@googlemail.com>
> >
> > uart_handle_cts_change() should be called in IRQ locked state, hence
> > use port->lock to disable interrupts.
> >
> > CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> > ---
> >  drivers/tty/serial/serial_mctrl_gpio.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> > index bb5e4f9..85b77ba 100644
> > --- a/drivers/tty/serial/serial_mctrl_gpio.c
> > +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> > @@ -143,6 +143,9 @@ static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
> >         struct uart_port *port = gpios->port;
> >         u32 mctrl = gpios->mctrl_prev;
> >         u32 mctrl_diff;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&port->lock, flags);
> >
> >         mctrl_gpio_get(gpios, &mctrl);
> >
> > @@ -165,6 +168,8 @@ static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
> >                 wake_up_interruptible(&port->state->port.delta_msr_wait);
> >         }
> >
> > +       spin_unlock_irqrestore(&port->lock, flags);
> > +
> >         return IRQ_HANDLED;
> >  }
> 
> Could you please test this patch with Atmel UART?
> 
> Should this patch be actually removed this series and sent separately?

I don't have at91 ready to test. I added Nicolas to Cc who tested 
18dfef9c7f87 ("serial: atmel: convert to irq handling provided mctrl-gpio").

I guess the problem that your patch intends to fix is obvious with lock
debugging enabled?

Best regards
Uwe
Uwe Kleine-König April 27, 2016, 10:20 a.m. UTC | #3
Hello,

On Tue, Mar 22, 2016 at 02:12:08PM +0100, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> uart_handle_cts_change() should be called in IRQ locked state, hence
> use port->lock to disable interrupts.

I think the wording of the commit log is not exact.

uart_handle_cts_change should be called with port->lock held. And for
this to be save you must also disable irqs.

The patch looks fine.

Best regards
Uwe
diff mbox

Patch

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index bb5e4f9..85b77ba 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -143,6 +143,9 @@  static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
 	struct uart_port *port = gpios->port;
 	u32 mctrl = gpios->mctrl_prev;
 	u32 mctrl_diff;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
 
 	mctrl_gpio_get(gpios, &mctrl);
 
@@ -165,6 +168,8 @@  static irqreturn_t mctrl_gpio_irq_handle(int irq, void *context)
 		wake_up_interruptible(&port->state->port.delta_msr_wait);
 	}
 
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	return IRQ_HANDLED;
 }