| Message ID | 1409760567-13186-1-git-send-email-richard.genoud@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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); > } >
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@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,
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
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,
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); }
In set_termios(), interrupts where not disabled if UART_ENABLE_MS() was false. Tested on at91sam9g35. CC: stable@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(-)