[PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop

linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop
@ 2016-08-08 13:32 dirk.eibach
  2016-08-08 13:32 ` [PATCH 2/2] sc16is7xx: Disable regmap cache dirk.eibach
  2016-08-08 13:43 ` [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop Greg KH
  0 siblings, 2 replies; 4+ messages in thread
From: dirk.eibach @ 2016-08-08 13:32 UTC (permalink / raw)
  To: linux-kernel, linux-serial, jslaby, gregkh, jringle; +Cc: Dirk Eibach

From: Dirk Eibach <dirk.eibach@gdsys.cc>

sc16is7xx_port_irq() is laid out as an endless loop. It will exit only
when there is no more interrupt left to service. This not common
practice.
In our case it lead to some strange hangup situation when there was an
unexpected XOFF-interrupt that could not be handled.
So let's service interrupts only once and report XOFF-interrupts that
should never happen since they are never enabled.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>

Conflicts:
	drivers/tty/serial/sc16is7xx.c
---
 drivers/tty/serial/sc16is7xx.c | 48 +++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index f36e6df..098c6dc 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -664,35 +664,31 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 {
 	struct uart_port *port = &s->p[portno].port;
+	unsigned int iir, ier, msr, rxlen;
 
-	do {
-		unsigned int iir, rxlen;
-
-		iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
-		if (iir & SC16IS7XX_IIR_NO_INT_BIT)
-			break;
+	iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
+	if (iir & SC16IS7XX_IIR_NO_INT_BIT)
+		return;
 
-		iir &= SC16IS7XX_IIR_ID_MASK;
+	iir &= SC16IS7XX_IIR_ID_MASK;
 
-		switch (iir) {
-		case SC16IS7XX_IIR_RDI_SRC:
-		case SC16IS7XX_IIR_RLSE_SRC:
-		case SC16IS7XX_IIR_RTOI_SRC:
-		case SC16IS7XX_IIR_XOFFI_SRC:
-			rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
-			if (rxlen)
-				sc16is7xx_handle_rx(port, rxlen, iir);
-			break;
-		case SC16IS7XX_IIR_THRI_SRC:
-			sc16is7xx_handle_tx(port);
-			break;
-		default:
-			dev_err_ratelimited(port->dev,
-					    "ttySC%i: Unexpected interrupt: %x",
-					    port->line, iir);
-			break;
-		}
-	} while (1);
+	switch (iir) {
+	case SC16IS7XX_IIR_RDI_SRC:
+	case SC16IS7XX_IIR_RLSE_SRC:
+	case SC16IS7XX_IIR_RTOI_SRC:
+		rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
+		if (rxlen)
+			sc16is7xx_handle_rx(port, rxlen, iir);
+		break;
+	case SC16IS7XX_IIR_THRI_SRC:
+		sc16is7xx_handle_tx(port);
+		break;
+	default:
+		dev_err_ratelimited(port->dev,
+				    "Port %i: Unexpected interrupt: iir %02x, ier %02x",
+				    port->line, iir, ier);
+		break;
+	}
 }
 
 static void sc16is7xx_ist(struct kthread_work *ws)
-- 
2.1.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] sc16is7xx: Disable regmap cache
  2016-08-08 13:32 [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop dirk.eibach
@ 2016-08-08 13:32 ` dirk.eibach
  2016-08-08 13:43 ` [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop Greg KH
  1 sibling, 0 replies; 4+ messages in thread
From: dirk.eibach @ 2016-08-08 13:32 UTC (permalink / raw)
  To: linux-kernel, linux-serial, jslaby, gregkh, jringle; +Cc: Dirk Eibach

From: Dirk Eibach <dirk.eibach@gdsys.cc>

The regcache_cache_bypass() calls are not implemented to be threadsafe.
Since I don't see the point in using a regmap cache on an UART
interface it is better to disable it alltogether and avoid these
problems.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
---
 drivers/tty/serial/sc16is7xx.c | 39 ++-------------------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 098c6dc..24ac6a2 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -378,9 +378,7 @@ static void sc16is7xx_fifo_read(struct uart_port *port, unsigned int rxlen)
 	const u8 line = sc16is7xx_line(port);
 	u8 addr = (SC16IS7XX_RHR_REG << SC16IS7XX_REG_SHIFT) | line;
 
-	regcache_cache_bypass(s->regmap, true);
 	regmap_raw_read(s->regmap, addr, s->buf, rxlen);
-	regcache_cache_bypass(s->regmap, false);
 }
 
 static void sc16is7xx_fifo_write(struct uart_port *port, u8 to_send)
@@ -396,9 +394,7 @@ static void sc16is7xx_fifo_write(struct uart_port *port, u8 to_send)
 	if (unlikely(!to_send))
 		return;
 
-	regcache_cache_bypass(s->regmap, true);
 	regmap_raw_write(s->regmap, addr, s->buf, to_send);
-	regcache_cache_bypass(s->regmap, false);
 }
 
 static void sc16is7xx_port_update(struct uart_port *port, u8 reg,
@@ -461,24 +457,6 @@ static const struct sc16is7xx_devtype sc16is762_devtype = {
 	.nr_uart	= 2,
 };
 
-static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
-{
-	switch (reg >> SC16IS7XX_REG_SHIFT) {
-	case SC16IS7XX_RHR_REG:
-	case SC16IS7XX_IIR_REG:
-	case SC16IS7XX_LSR_REG:
-	case SC16IS7XX_MSR_REG:
-	case SC16IS7XX_TXLVL_REG:
-	case SC16IS7XX_RXLVL_REG:
-	case SC16IS7XX_IOSTATE_REG:
-		return true;
-	default:
-		break;
-	}
-
-	return false;
-}
-
 static bool sc16is7xx_regmap_precious(struct device *dev, unsigned int reg)
 {
 	switch (reg >> SC16IS7XX_REG_SHIFT) {
@@ -493,7 +471,6 @@ static bool sc16is7xx_regmap_precious(struct device *dev, unsigned int reg)
 
 static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 {
-	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	u8 lcr;
 	u8 prescaler = 0;
 	unsigned long clk = port->uartclk, div = clk / 16 / baud;
@@ -510,10 +487,8 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 			     SC16IS7XX_LCR_CONF_MODE_B);
 
 	/* Enable enhanced features */
-	regcache_cache_bypass(s->regmap, true);
 	sc16is7xx_port_write(port, SC16IS7XX_EFR_REG,
 			     SC16IS7XX_EFR_ENABLE_BIT);
-	regcache_cache_bypass(s->regmap, false);
 
 	/* Put LCR back to the normal mode */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
@@ -527,10 +502,8 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 			     SC16IS7XX_LCR_CONF_MODE_A);
 
 	/* Write the new divisor */
-	regcache_cache_bypass(s->regmap, true);
 	sc16is7xx_port_write(port, SC16IS7XX_DLH_REG, div / 256);
 	sc16is7xx_port_write(port, SC16IS7XX_DLL_REG, div % 256);
-	regcache_cache_bypass(s->regmap, false);
 
 	/* Put LCR back to the normal mode */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
@@ -664,7 +637,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 {
 	struct uart_port *port = &s->p[portno].port;
-	unsigned int iir, ier, msr, rxlen;
+	unsigned int iir, ier, rxlen;
 
 	iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
 	if (iir & SC16IS7XX_IIR_NO_INT_BIT)
@@ -838,7 +811,6 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 				  struct ktermios *termios,
 				  struct ktermios *old)
 {
-	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	unsigned int lcr, flow = 0;
 	int baud;
 
@@ -896,7 +868,6 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 			     SC16IS7XX_LCR_CONF_MODE_B);
 
 	/* Configure flow control */
-	regcache_cache_bypass(s->regmap, true);
 	sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios->c_cc[VSTART]);
 	sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios->c_cc[VSTOP]);
 	if (termios->c_cflag & CRTSCTS)
@@ -908,7 +879,6 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 		flow |= SC16IS7XX_EFR_SWFLOW1_BIT;
 
 	sc16is7xx_port_write(port, SC16IS7XX_EFR_REG, flow);
-	regcache_cache_bypass(s->regmap, false);
 
 	/* Update LCR register */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
@@ -960,7 +930,6 @@ static int sc16is7xx_config_rs485(struct uart_port *port,
 
 static int sc16is7xx_startup(struct uart_port *port)
 {
-	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	unsigned int val;
 
 	sc16is7xx_power(port, 1);
@@ -976,7 +945,6 @@ static int sc16is7xx_startup(struct uart_port *port)
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
 			     SC16IS7XX_LCR_CONF_MODE_B);
 
-	regcache_cache_bypass(s->regmap, true);
 
 	/* Enable write access to enhanced features and internal clock div */
 	sc16is7xx_port_write(port, SC16IS7XX_EFR_REG,
@@ -993,8 +961,6 @@ static int sc16is7xx_startup(struct uart_port *port)
 			     SC16IS7XX_TCR_RX_RESUME(24) |
 			     SC16IS7XX_TCR_RX_HALT(48));
 
-	regcache_cache_bypass(s->regmap, false);
-
 	/* Now, initialize the UART */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, SC16IS7XX_LCR_WORD_LEN_8);
 
@@ -1301,8 +1267,7 @@ static struct regmap_config regcfg = {
 	.reg_bits = 7,
 	.pad_bits = 1,
 	.val_bits = 8,
-	.cache_type = REGCACHE_RBTREE,
-	.volatile_reg = sc16is7xx_regmap_volatile,
+	.cache_type = REGCACHE_NONE,
 	.precious_reg = sc16is7xx_regmap_precious,
 };
 
-- 
2.1.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop
  2016-08-08 13:32 [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop dirk.eibach
  2016-08-08 13:32 ` [PATCH 2/2] sc16is7xx: Disable regmap cache dirk.eibach
@ 2016-08-08 13:43 ` Greg KH
  2016-08-08 14:02   ` Dirk Eibach
  1 sibling, 1 reply; 4+ messages in thread
From: Greg KH @ 2016-08-08 13:43 UTC (permalink / raw)
  To: dirk.eibach; +Cc: linux-kernel, linux-serial, jslaby, jringle

On Mon, Aug 08, 2016 at 03:32:15PM +0200, dirk.eibach@gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
> 
> sc16is7xx_port_irq() is laid out as an endless loop. It will exit only
> when there is no more interrupt left to service. This not common
> practice.
> In our case it lead to some strange hangup situation when there was an
> unexpected XOFF-interrupt that could not be handled.
> So let's service interrupts only once and report XOFF-interrupts that
> should never happen since they are never enabled.
> 
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
> 
> Conflicts:
> 	drivers/tty/serial/sc16is7xx.c

Why are these 2 lines in here?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop
  2016-08-08 13:43 ` [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop Greg KH
@ 2016-08-08 14:02   ` Dirk Eibach
  0 siblings, 0 replies; 4+ messages in thread
From: Dirk Eibach @ 2016-08-08 14:02 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-kernel, linux-serial, jslaby, jringle

2016-08-08 15:43 GMT+02:00 Greg KH <gregkh@linuxfoundation.org>:
> On Mon, Aug 08, 2016 at 03:32:15PM +0200, dirk.eibach@gdsys.cc wrote:
>> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>>
>> sc16is7xx_port_irq() is laid out as an endless loop. It will exit only
>> when there is no more interrupt left to service. This not common
>> practice.
>> In our case it lead to some strange hangup situation when there was an
>> unexpected XOFF-interrupt that could not be handled.
>> So let's service interrupts only once and report XOFF-interrupts that
>> should never happen since they are never enabled.
>>
>> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
>>
>> Conflicts:
>>       drivers/tty/serial/sc16is7xx.c
>
> Why are these 2 lines in here?

Sorry, my bad. I have no idea how they slipped in. Will remove them in v2.

Cheers
Dirk

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-08-08 14:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-08 13:32 [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop dirk.eibach
2016-08-08 13:32 ` [PATCH 2/2] sc16is7xx: Disable regmap cache dirk.eibach
2016-08-08 13:43 ` [PATCH 1/2] sc16is7xx: Do not handle irqs in endless loop Greg KH
2016-08-08 14:02   ` Dirk Eibach

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).