[PATCH] tty/serial: at91: BUG: disable interrupts when !UART_ENABLE_MS()

[PATCH] tty/serial: at91: BUG: disable interrupts when !UART_ENABLE_MS()

[Richard Genoud]

In set_termios(), interrupts where not disabled if UART_ENABLE_MS() was
false.

Tested on at91sam9g35.

CC: stable at vger.kernel.org # >= 3.16

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 43 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 7b63677475c1..d7d4584549a5 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -527,6 +527,45 @@ static void atmel_enable_ms(struct uart_port *port)
 }
 
 /*
+ * Disable modem status interrupts
+ */
+static void atmel_disable_ms(struct uart_port *port)
+{
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+	uint32_t idr = 0;
+
+	/*
+	 * Interrupt should not be disabled twice
+	 */
+	if (!atmel_port->ms_irq_enabled)
+		return;
+
+	atmel_port->ms_irq_enabled = false;
+
+	if (atmel_port->gpio_irq[UART_GPIO_CTS] >= 0)
+		disable_irq(atmel_port->gpio_irq[UART_GPIO_CTS]);
+	else
+		idr |= ATMEL_US_CTSIC;
+
+	if (atmel_port->gpio_irq[UART_GPIO_DSR] >= 0)
+		disable_irq(atmel_port->gpio_irq[UART_GPIO_DSR]);
+	else
+		idr |= ATMEL_US_DSRIC;
+
+	if (atmel_port->gpio_irq[UART_GPIO_RI] >= 0)
+		disable_irq(atmel_port->gpio_irq[UART_GPIO_RI]);
+	else
+		idr |= ATMEL_US_RIIC;
+
+	if (atmel_port->gpio_irq[UART_GPIO_DCD] >= 0)
+		disable_irq(atmel_port->gpio_irq[UART_GPIO_DCD]);
+	else
+		idr |= ATMEL_US_DCDIC;
+
+	UART_PUT_IDR(port, idr);
+}
+
+/*
  * Control the transmission of a break signal
  */
 static void atmel_break_ctl(struct uart_port *port, int break_state)
@@ -1993,7 +2032,9 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* CTS flow-control and modem-status interrupts */
 	if (UART_ENABLE_MS(port, termios->c_cflag))
-		port->ops->enable_ms(port);
+		atmel_enable_ms(port);
+	else
+		atmel_disable_ms(port);
 
 	spin_unlock_irqrestore(&port->lock, flags);
 }
-- 
2.0.0

[PATCH] tty/serial: at91: BUG: disable interrupts when !UART_ENABLE_MS()

[Peter Hurley]

On 09/03/2014 12:09 PM, Richard Genoud wrote:
> In set_termios(), interrupts where not disabled if UART_ENABLE_MS() was
> false.
> 
> Tested on at91sam9g35.
> 
> CC: stable at vger.kernel.org # >= 3.16

Awesome, thank you.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 43 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7b63677475c1..d7d4584549a5 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -527,6 +527,45 @@ static void atmel_enable_ms(struct uart_port *port)
>  }
>  
>  /*
> + * Disable modem status interrupts
> + */
> +static void atmel_disable_ms(struct uart_port *port)
> +{
> +	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> +	uint32_t idr = 0;
> +
> +	/*
> +	 * Interrupt should not be disabled twice
> +	 */
> +	if (!atmel_port->ms_irq_enabled)
> +		return;
> +
> +	atmel_port->ms_irq_enabled = false;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_CTS] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_CTS]);
> +	else
> +		idr |= ATMEL_US_CTSIC;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_DSR] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_DSR]);
> +	else
> +		idr |= ATMEL_US_DSRIC;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_RI] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_RI]);
> +	else
> +		idr |= ATMEL_US_RIIC;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_DCD] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_DCD]);
> +	else
> +		idr |= ATMEL_US_DCDIC;
> +
> +	UART_PUT_IDR(port, idr);
> +}
> +
> +/*
>   * Control the transmission of a break signal
>   */
>  static void atmel_break_ctl(struct uart_port *port, int break_state)
> @@ -1993,7 +2032,9 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>  	/* CTS flow-control and modem-status interrupts */
>  	if (UART_ENABLE_MS(port, termios->c_cflag))
> -		port->ops->enable_ms(port);
> +		atmel_enable_ms(port);
> +	else
> +		atmel_disable_ms(port);
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
> 

[PATCH] tty/serial: at91: BUG: disable interrupts when !UART_ENABLE_MS()

[Nicolas Ferre]

On 03/09/2014 18:09, Richard Genoud :
> In set_termios(), interrupts where not disabled if UART_ENABLE_MS() was
> false.
> 
> Tested on at91sam9g35.
> 
> CC: stable at vger.kernel.org # >= 3.16
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 43 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7b63677475c1..d7d4584549a5 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -527,6 +527,45 @@ static void atmel_enable_ms(struct uart_port *port)
>  }
>  
>  /*
> + * Disable modem status interrupts
> + */
> +static void atmel_disable_ms(struct uart_port *port)
> +{
> +	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> +	uint32_t idr = 0;
> +
> +	/*
> +	 * Interrupt should not be disabled twice
> +	 */
> +	if (!atmel_port->ms_irq_enabled)
> +		return;
> +
> +	atmel_port->ms_irq_enabled = false;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_CTS] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_CTS]);
> +	else
> +		idr |= ATMEL_US_CTSIC;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_DSR] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_DSR]);
> +	else
> +		idr |= ATMEL_US_DSRIC;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_RI] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_RI]);
> +	else
> +		idr |= ATMEL_US_RIIC;
> +
> +	if (atmel_port->gpio_irq[UART_GPIO_DCD] >= 0)
> +		disable_irq(atmel_port->gpio_irq[UART_GPIO_DCD]);
> +	else
> +		idr |= ATMEL_US_DCDIC;
> +
> +	UART_PUT_IDR(port, idr);
> +}
> +
> +/*
>   * Control the transmission of a break signal
>   */
>  static void atmel_break_ctl(struct uart_port *port, int break_state)
> @@ -1993,7 +2032,9 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>  	/* CTS flow-control and modem-status interrupts */
>  	if (UART_ENABLE_MS(port, termios->c_cflag))
> -		port->ops->enable_ms(port);
> +		atmel_enable_ms(port);
> +	else
> +		atmel_disable_ms(port);
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }

Richard,

Indeed it seems needed:

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>


But BTW, I see just below a call to the atmel_enable_ms() function in
atmel_set_ldisc(). My question is, shouldn't we also add this
atmel_disable_ms() in the alternative that disables the PPS in this
ldisc function?

Thanks, bye,
-- 
Nicolas Ferre

[PATCH] tty/serial: at91: BUG: disable interrupts when !UART_ENABLE_MS()

[Peter Hurley]

Hi Nicolas,

On 09/05/2014 05:15 AM, Nicolas Ferre wrote:
> On 03/09/2014 18:09, Richard Genoud :
> Richard,
> 
> Indeed it seems needed:
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> 
> But BTW, I see just below a call to the atmel_enable_ms() function in
> atmel_set_ldisc(). My question is, shouldn't we also add this
> atmel_disable_ms() in the alternative that disables the PPS in this
> ldisc function?

I have that change in another series but it has to wait for:

1. another series that fixes races setting and clearing the controlling tty
   (and reduces the footprint of tty_mutex)
2. a series built on that which moves tty_lock() out from under tty_mutex
   when reopening ttys
   This allows the tty_lock to be held while closing the tty.
3. a series which removes the ldisc flush from the serial core, among some
   other locking fixes in the serial core.
   This fixes a lock inversion between the termios_rwsem and the port mutex.

All of which enables the set_ldisc() notification to be safely passed
termios so it can use UART_ENABLE_MS() and also claim the port mutex
to change the UPF_HARDPPS_CD flag, which is currently non-atomic and
may corrupt the uart port flags field.

The series also claims the port lock around the *_enable_ms() in the
various set_ldisc() handlers to protect the hardware re-programming :)

Right now, all of this is temporarily stuck on the most recent patch
series, which hinges on whether the kernel should continue to support
non-atomic byte stores.

Regards,
Peter Hurley

[PATCH] tty/serial: at91: BUG: disable interrupts when !UART_ENABLE_MS()

[Nicolas Ferre]

On 05/09/2014 13:06, Peter Hurley :
> Hi Nicolas,
> 
> On 09/05/2014 05:15 AM, Nicolas Ferre wrote:
>> On 03/09/2014 18:09, Richard Genoud :
>> Richard,
>>
>> Indeed it seems needed:
>>
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>
>>
>> But BTW, I see just below a call to the atmel_enable_ms() function in
>> atmel_set_ldisc(). My question is, shouldn't we also add this
>> atmel_disable_ms() in the alternative that disables the PPS in this
>> ldisc function?
> 
> I have that change in another series but it has to wait for:

Given the attractive enhancements that you describe below... I'll wait
with pleasure ;-)

> 1. another series that fixes races setting and clearing the controlling tty
>    (and reduces the footprint of tty_mutex)
> 2. a series built on that which moves tty_lock() out from under tty_mutex
>    when reopening ttys
>    This allows the tty_lock to be held while closing the tty.
> 3. a series which removes the ldisc flush from the serial core, among some
>    other locking fixes in the serial core.
>    This fixes a lock inversion between the termios_rwsem and the port mutex.
> 
> All of which enables the set_ldisc() notification to be safely passed
> termios so it can use UART_ENABLE_MS() and also claim the port mutex
> to change the UPF_HARDPPS_CD flag, which is currently non-atomic and
> may corrupt the uart port flags field.
> 
> The series also claims the port lock around the *_enable_ms() in the
> various set_ldisc() handlers to protect the hardware re-programming :)
> 
> Right now, all of this is temporarily stuck on the most recent patch
> series, which hinges on whether the kernel should continue to support
> non-atomic byte stores.

Thanks for sharing your plans. Bye,
-- 
Nicolas Ferre