Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1612245 > unrolled thread

[PATCH] serial: Do not treat the IIR register as a bitfield

Back to article view | Back to linux.kernel

Contents

  [PATCH] serial: Do not treat the IIR register as a bitfield Olliver Schinagl <oliver@schinagl.nl> - 2017-03-29 20:50 +0200
    Re: [PATCH] serial: Do not treat the IIR register as a bitfield Vignesh R <vigneshr@ti.com> - 2017-03-30 08:20 +0200
      Re: [PATCH] serial: Do not treat the IIR register as a bitfield Olliver Schinagl <oliver@schinagl.nl> - 2017-03-30 08:50 +0200
        Re: [PATCH] serial: Do not treat the IIR register as a bitfield Vignesh R <vigneshr@ti.com> - 2017-03-30 10:10 +0200
          Re: [PATCH] serial: Do not treat the IIR register as a bitfield Olliver Schinagl <oliver@schinagl.nl> - 2017-03-30 17:50 +0200
        Re: [PATCH] serial: Do not treat the IIR register as a bitfield Theodore Ts'o <tytso@mit.edu> - 2017-03-30 16:20 +0200
          Re: [PATCH] serial: Do not treat the IIR register as a bitfield Olliver Schinagl <o.schinagl@ultimaker.com> - 2017-03-31 13:30 +0200
    Re: [PATCH] serial: Do not treat the IIR register as a bitfield Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-30 12:10 +0200
      Re: [PATCH] serial: Do not treat the IIR register as a bitfield Olliver Schinagl <o.schinagl@ultimaker.com> - 2017-03-31 16:00 +0200
        Re: [PATCH] serial: Do not treat the IIR register as a bitfield Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-31 16:50 +0200
    Re: [PATCH] serial: Do not treat the IIR register as a bitfield kbuild test robot <lkp@intel.com> - 2017-03-30 14:20 +0200

#1612245 — [PATCH] serial: Do not treat the IIR register as a bitfield

It seems that at some point, someone made the assumption that the UART
Interrupt ID Register was a bitfield and started to check if certain
bits where set.

Actually however the register contains interrupt ID's where only the MSB
seems to be used singular and the rest share at least one bit. Thus
doing bitfield operations is wrong.

This patch cleans up the serial_reg include file by ordering it and
replacing the UART_IIR_ID 'mask' with a proper mask for the register.
The OMAP uart appears to have used the two commonly 'reserved' bits 4
and 5 and thus get an UART_IIR_EXT_MASK for these two bits.

This patch then goes over all UART_IIR_* users and changes the code from
bitfield checking, to ID checking instead.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---

Note, that I do not have all this hardware and used the fact that UART_IIR_*
yields ID's rather then bitfields and thus mentioned as above, cannot be
bit-checked. Please carefully look at the changes, as they do make sense to me
a slip up is quickly made. Additionally, note the old code worked probably 100%
of the time, but was written wrong I think.

If I was wrong, I do apologize for the noise.

 drivers/tty/serial/8250/8250_core.c |  8 +++++---
 drivers/tty/serial/8250/8250_dw.c   |  5 +++--
 drivers/tty/serial/8250/8250_fsl.c  |  3 ++-
 drivers/tty/serial/8250/8250_omap.c |  4 ++--
 drivers/tty/serial/8250/8250_port.c | 11 ++++++-----
 drivers/tty/serial/omap-serial.c    | 12 ++++++------
 drivers/tty/serial/pxa.c            |  2 +-
 drivers/tty/serial/serial-tegra.c   |  3 ++-
 drivers/tty/serial/sunsu.c          |  2 +-
 drivers/tty/serial/vr41xx_siu.c     |  2 +-
 include/uapi/linux/serial_reg.h     |  8 ++++----
 11 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 76e03a7de9cc..11b4e3fb0c1d 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -288,6 +288,7 @@ static void serial8250_backup_timeout(unsigned long data)
 	}
 
 	iir = serial_in(up, UART_IIR);
+	iir &= UART_IIR_MASK;
 
 	/*
 	 * This should be a safe test for anyone who doesn't trust the
@@ -297,14 +298,15 @@ static void serial8250_backup_timeout(unsigned long data)
 	 */
 	lsr = serial_in(up, UART_LSR);
 	up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
-	if ((iir & UART_IIR_NO_INT) && (up->ier & UART_IER_THRI) &&
+	if ((iir == UART_IIR_NO_INT) &&
+	    (up->ier & UART_IER_THRI) &&
 	    (!uart_circ_empty(&up->port.state->xmit) || up->port.x_char) &&
 	    (lsr & UART_LSR_THRE)) {
-		iir &= ~(UART_IIR_ID | UART_IIR_NO_INT);
+		iir &= ~(UART_IIR_MASK);
 		iir |= UART_IIR_THRI;
 	}
 
-	if (!(iir & UART_IIR_NO_INT))
+	if (iir != UART_IIR_NO_INT)
 		serial8250_tx_chars(up);
 
 	if (up->port.irq)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 56aded887771..90cf0e6b4c5b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -218,7 +218,8 @@ static int dw8250_handle_irq(struct uart_port *p)
 	 * This problem has only been observed so far when not in DMA mode
 	 * so we limit the workaround only to non-DMA mode.
 	 */
-	if (!up->dma && ((iir & 0x3f) == UART_IIR_RX_TIMEOUT)) {
+	iir &= UART_IIR_MASK;
+	if (!up->dma && (iir == UART_IIR_RX_TIMEOUT)) {
 		spin_lock_irqsave(&p->lock, flags);
 		status = p->serial_in(p, UART_LSR);
 
@@ -231,7 +232,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 	if (serial8250_handle_irq(p, iir))
 		return 1;
 
-	if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
+	if (iir == UART_IIR_BUSY) {
 		/* Clear the USR */
 		(void)p->serial_in(p, d->usr_reg);
 
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 910bfee5a88b..b866343bb485 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -33,7 +33,8 @@ int fsl8250_handle_irq(struct uart_port *port)
 	spin_lock_irqsave(&up->port.lock, flags);
 
 	iir = port->serial_in(port, UART_IIR);
-	if (iir & UART_IIR_NO_INT) {
+	iir &= UART_IIR_MASK;
+	if (iir == UART_IIR_NO_INT) {
 		spin_unlock_irqrestore(&up->port.lock, flags);
 		return 0;
 	}
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index e7e64913a748..ab7c2327d410 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -996,7 +996,7 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
 
 static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 {
-	switch (iir & 0x3f) {
+	switch (iir & (UART_IIR_MASK | UART_IIR_EXT_MASK)) {
 	case UART_IIR_RLSI:
 	case UART_IIR_RX_TIMEOUT:
 	case UART_IIR_RDI:
@@ -1021,7 +1021,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	serial8250_rpm_get(up);
 
 	iir = serial_port_in(port, UART_IIR);
-	if (iir & UART_IIR_NO_INT) {
+	if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT) {
 		serial8250_rpm_put(up);
 		return 0;
 	}
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 6119516ef5fc..8ee3204d63ba 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1806,7 +1806,7 @@ EXPORT_SYMBOL_GPL(serial8250_modem_status);
 
 static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 {
-	switch (iir & 0x3f) {
+	switch (iir & UART_IIR_MASK) {
 	case UART_IIR_RX_TIMEOUT:
 		serial8250_rx_dma_flush(up);
 		/* fall-through */
@@ -1825,7 +1825,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 	unsigned long flags;
 	struct uart_8250_port *up = up_to_u8250p(port);
 
-	if (iir & UART_IIR_NO_INT)
+	if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)
 		return 0;
 
 	spin_lock_irqsave(&port->lock, flags);
@@ -1896,7 +1896,7 @@ static int serial8250_tx_threshold_handle_irq(struct uart_port *port)
 	unsigned int iir = serial_port_in(port, UART_IIR);
 
 	/* TX Threshold IRQ triggered so load up FIFO */
-	if ((iir & UART_IIR_ID) == UART_IIR_THRI) {
+	if ((iir & UART_IIR_MASK) == UART_IIR_THRI) {
 		struct uart_8250_port *up = up_to_u8250p(port);
 
 		spin_lock_irqsave(&port->lock, flags);
@@ -2262,7 +2262,8 @@ int serial8250_do_startup(struct uart_port *port)
 		 * don't trust the iir, setup a timer to kick the UART
 		 * on a regular basis.
 		 */
-		if ((!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) ||
+		if ((((iir1 & UART_IIR_MASK) != UART_IIR_NO_INT) &&
+		     ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)) ||
 		    up->port.flags & UPF_BUG_THRE) {
 			up->bugs |= UART_BUG_THRE;
 		}
@@ -2313,7 +2314,7 @@ int serial8250_do_startup(struct uart_port *port)
 	iir = serial_port_in(port, UART_IIR);
 	serial_port_out(port, UART_IER, 0);
 
-	if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) {
+	if (lsr & UART_LSR_TEMT && ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)) {
 		if (!(up->bugs & UART_BUG_TXEN)) {
 			up->bugs |= UART_BUG_TXEN;
 			pr_debug("ttyS%d - enabling bad tx status workarounds\n",
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 6c6f82ad8d5c..4e7269b8bf1b 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -569,7 +569,6 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
 {
 	struct uart_omap_port *up = dev_id;
 	unsigned int iir, lsr;
-	unsigned int type;
 	irqreturn_t ret = IRQ_NONE;
 	int max_count = 256;
 
@@ -578,16 +577,15 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
 
 	do {
 		iir = serial_in(up, UART_IIR);
-		if (iir & UART_IIR_NO_INT)
+		iir &= (UART_IIR_MASK | UART_IIR_EXT_MASK);
+		if (iir == UART_IIR_NO_INT)
 			break;
 
 		ret = IRQ_HANDLED;
 		lsr = serial_in(up, UART_LSR);
 
 		/* extract IRQ type from IIR register */
-		type = iir & 0x3e;
-
-		switch (type) {
+		switch (iir) {
 		case UART_IIR_MSI:
 			check_modem_status(up);
 			break;
@@ -607,10 +605,12 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
 			break;
 		case UART_IIR_XOFF:
 			/* FALLTHROUGH */
+		case UART_IIR_BUSY:
+			/* FALLTHROUGH */
 		default:
 			break;
 		}
-	} while (!(iir & UART_IIR_NO_INT) && max_count--);
+	} while ((iir != UART_IIR_NO_INT) && max_count--);
 
 	spin_unlock(&up->port.lock);
 
diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 905631df1f8b..37cb17a11b34 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c
@@ -253,7 +253,7 @@ static inline irqreturn_t serial_pxa_irq(int irq, void *dev_id)
 	unsigned int iir, lsr;
 
 	iir = serial_in(up, UART_IIR);
-	if (iir & UART_IIR_NO_INT)
+	if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)
 		return IRQ_NONE;
 	spin_lock(&up->port.lock);
 	lsr = serial_in(up, UART_LSR);
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index d92a150c8733..4a084161d1d2 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -691,7 +691,8 @@ static irqreturn_t tegra_uart_isr(int irq, void *data)
 	spin_lock_irqsave(&u->lock, flags);
 	while (1) {
 		iir = tegra_uart_read(tup, UART_IIR);
-		if (iir & UART_IIR_NO_INT) {
+		iir &= UART_IIR_MASK;
+		if (iir == UART_IIR_NO_INT) {
 			if (is_rx_int) {
 				tegra_uart_handle_rx_dma(tup);
 				if (tup->rx_in_progress) {
diff --git a/drivers/tty/serial/sunsu.c b/drivers/tty/serial/sunsu.c
index 72df2e1b88af..74e195d4d17e 100644
--- a/drivers/tty/serial/sunsu.c
+++ b/drivers/tty/serial/sunsu.c
@@ -475,7 +475,7 @@ static irqreturn_t sunsu_serial_interrupt(int irq, void *dev_id)
 
 		spin_lock_irqsave(&up->port.lock, flags);
 
-	} while (!(serial_in(up, UART_IIR) & UART_IIR_NO_INT));
+	} while ((serial_in(up, UART_IIR) & UART_IIR_MASK) != UART_IIR_NO_INT);
 
 	spin_unlock_irqrestore(&up->port.lock, flags);
 
diff --git a/drivers/tty/serial/vr41xx_siu.c b/drivers/tty/serial/vr41xx_siu.c
index 439057e8107a..fc3b9bd920e9 100644
--- a/drivers/tty/serial/vr41xx_siu.c
+++ b/drivers/tty/serial/vr41xx_siu.c
@@ -429,7 +429,7 @@ static irqreturn_t siu_interrupt(int irq, void *dev_id)
 	port = (struct uart_port *)dev_id;
 
 	iir = siu_read(port, UART_IIR);
-	if (iir & UART_IIR_NO_INT)
+	if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)
 		return IRQ_NONE;
 
 	lsr = siu_read(port, UART_LSR);
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index 5db76880b4ad..489522389a10 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -31,18 +31,18 @@
 #define UART_IERX_SLEEP		0x10 /* Enable sleep mode */
 
 #define UART_IIR	2	/* In:  Interrupt ID Register */
-#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
-#define UART_IIR_ID		0x0e /* Mask for the interrupt ID */
 #define UART_IIR_MSI		0x00 /* Modem status interrupt */
+#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
 #define UART_IIR_THRI		0x02 /* Transmitter holding register empty */
 #define UART_IIR_RDI		0x04 /* Receiver data interrupt */
 #define UART_IIR_RLSI		0x06 /* Receiver line status interrupt */
-
 #define UART_IIR_BUSY		0x07 /* DesignWare APB Busy Detect */
+#define UART_IIR_RX_TIMEOUT	0x0c /* DesignWare RX Timeout interrupt */
+#define UART_IIR_MASK		0x0f /* DesignWare IIR mask */
 
-#define UART_IIR_RX_TIMEOUT	0x0c /* OMAP RX Timeout interrupt */
 #define UART_IIR_XOFF		0x10 /* OMAP XOFF/Special Character */
 #define UART_IIR_CTS_RTS_DSR	0x20 /* OMAP CTS/RTS/DSR Change */
+#define UART_IIR_EXT_MASK	0x30 /* OMAP extended IIR mask */
 
 #define UART_FCR	2	/* Out: FIFO Control Register */
 #define UART_FCR_ENABLE_FIFO	0x01 /* Enable the FIFO */
-- 
2.11.0

[toc] | [next] | [standalone]

#1612582

Hi,

On Thursday 30 March 2017 12:14 AM, Olliver Schinagl wrote:
> diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
> index 5db76880b4ad..489522389a10 100644
> --- a/include/uapi/linux/serial_reg.h
> +++ b/include/uapi/linux/serial_reg.h
> @@ -31,18 +31,18 @@
>  #define UART_IERX_SLEEP		0x10 /* Enable sleep mode */
>  
>  #define UART_IIR	2	/* In:  Interrupt ID Register */
> -#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
> -#define UART_IIR_ID		0x0e /* Mask for the interrupt ID */
>  #define UART_IIR_MSI		0x00 /* Modem status interrupt */
> +#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
>  #define UART_IIR_THRI		0x02 /* Transmitter holding register empty */
>  #define UART_IIR_RDI		0x04 /* Receiver data interrupt */
>  #define UART_IIR_RLSI		0x06 /* Receiver line status interrupt */
> -
>  #define UART_IIR_BUSY		0x07 /* DesignWare APB Busy Detect */
> +#define UART_IIR_RX_TIMEOUT	0x0c /* DesignWare RX Timeout interrupt */
> +#define UART_IIR_MASK		0x0f /* DesignWare IIR mask */
>  
> -#define UART_IIR_RX_TIMEOUT	0x0c /* OMAP RX Timeout interrupt */

You are removing UART_IIR_RX_TIMEOUT? Is this intended?

>  #define UART_IIR_XOFF		0x10 /* OMAP XOFF/Special Character */
>  #define UART_IIR_CTS_RTS_DSR	0x20 /* OMAP CTS/RTS/DSR Change */
> +#define UART_IIR_EXT_MASK	0x30 /* OMAP extended IIR mask */

-- 
Regards
Vignesh

[toc] | [prev] | [next] | [standalone]

#1612599

On March 30, 2017 8:15:29 AM CEST, Vignesh R <vigneshr@ti.com> wrote:
>Hi,
>
>On Thursday 30 March 2017 12:14 AM, Olliver Schinagl wrote:
>> diff --git a/include/uapi/linux/serial_reg.h
>b/include/uapi/linux/serial_reg.h
>> index 5db76880b4ad..489522389a10 100644
>> --- a/include/uapi/linux/serial_reg.h
>> +++ b/include/uapi/linux/serial_reg.h
>> @@ -31,18 +31,18 @@
>>  #define UART_IERX_SLEEP		0x10 /* Enable sleep mode */
>>  
>>  #define UART_IIR	2	/* In:  Interrupt ID Register */
>> -#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
>> -#define UART_IIR_ID		0x0e /* Mask for the interrupt ID */
>>  #define UART_IIR_MSI		0x00 /* Modem status interrupt */
>> +#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
>>  #define UART_IIR_THRI		0x02 /* Transmitter holding register empty */
>>  #define UART_IIR_RDI		0x04 /* Receiver data interrupt */
>>  #define UART_IIR_RLSI		0x06 /* Receiver line status interrupt */
>> -
>>  #define UART_IIR_BUSY		0x07 /* DesignWare APB Busy Detect */
>> +#define UART_IIR_RX_TIMEOUT	0x0c /* DesignWare RX Timeout interrupt
>*/
It was moved due to sorting. The comment could be changed maybe? I renamed it from omap to dw as i believe the omap also uses the dw ip. But i think it is a defacto standard mapping of the irr register?

>> +#define UART_IIR_MASK		0x0f /* DesignWare IIR mask */
>>  
>> -#define UART_IIR_RX_TIMEOUT	0x0c /* OMAP RX Timeout interrupt */
>
>You are removing UART_IIR_RX_TIMEOUT? Is this intended?
>
>>  #define UART_IIR_XOFF		0x10 /* OMAP XOFF/Special Character */
>>  #define UART_IIR_CTS_RTS_DSR	0x20 /* OMAP CTS/RTS/DSR Change */
>> +#define UART_IIR_EXT_MASK	0x30 /* OMAP extended IIR mask */

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1612698

On Thursday 30 March 2017 12:13 PM, Olliver Schinagl wrote:
> 
> 
> On March 30, 2017 8:15:29 AM CEST, Vignesh R <vigneshr@ti.com> wrote:
>> Hi,
>>
>> On Thursday 30 March 2017 12:14 AM, Olliver Schinagl wrote:
>>> diff --git a/include/uapi/linux/serial_reg.h
>> b/include/uapi/linux/serial_reg.h
>>> index 5db76880b4ad..489522389a10 100644
>>> --- a/include/uapi/linux/serial_reg.h
>>> +++ b/include/uapi/linux/serial_reg.h
>>> @@ -31,18 +31,18 @@
>>>  #define UART_IERX_SLEEP		0x10 /* Enable sleep mode */
>>>  
>>>  #define UART_IIR	2	/* In:  Interrupt ID Register */
>>> -#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
>>> -#define UART_IIR_ID		0x0e /* Mask for the interrupt ID */
>>>  #define UART_IIR_MSI		0x00 /* Modem status interrupt */
>>> +#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
>>>  #define UART_IIR_THRI		0x02 /* Transmitter holding register empty */
>>>  #define UART_IIR_RDI		0x04 /* Receiver data interrupt */
>>>  #define UART_IIR_RLSI		0x06 /* Receiver line status interrupt */
>>> -
>>>  #define UART_IIR_BUSY		0x07 /* DesignWare APB Busy Detect */
>>> +#define UART_IIR_RX_TIMEOUT	0x0c /* DesignWare RX Timeout interrupt
>> */
> It was moved due to sorting. The comment could be changed maybe? I renamed it from omap to dw as i believe the omap also uses the dw ip. But i think it is a defacto standard mapping of the irr register?

AFAIK, OMAP UART does not use DW core, please make these IDs generic to
avoid confusion.

> 
>>> +#define UART_IIR_MASK		0x0f /* DesignWare IIR mask */
>>>  
>>> -#define UART_IIR_RX_TIMEOUT	0x0c /* OMAP RX Timeout interrupt */
>>
>> You are removing UART_IIR_RX_TIMEOUT? Is this intended?
>>
>>>  #define UART_IIR_XOFF		0x10 /* OMAP XOFF/Special Character */
>>>  #define UART_IIR_CTS_RTS_DSR	0x20 /* OMAP CTS/RTS/DSR Change */
>>> +#define UART_IIR_EXT_MASK	0x30 /* OMAP extended IIR mask */
> 

-- 
Regards
Vignesh

[toc] | [prev] | [next] | [standalone]

#1613234

Hey Vignesh,

On March 30, 2017 9:57:19 AM CEST, Vignesh R <vigneshr@ti.com> wrote:
>
>
>On Thursday 30 March 2017 12:13 PM, Olliver Schinagl wrote:
>> 
>> 
>> On March 30, 2017 8:15:29 AM CEST, Vignesh R <vigneshr@ti.com> wrote:
>>> Hi,
>>>
>>> On Thursday 30 March 2017 12:14 AM, Olliver Schinagl wrote:
>>>> diff --git a/include/uapi/linux/serial_reg.h
>>> b/include/uapi/linux/serial_reg.h
>>>> index 5db76880b4ad..489522389a10 100644
>>>> --- a/include/uapi/linux/serial_reg.h
>>>> +++ b/include/uapi/linux/serial_reg.h
>>>> @@ -31,18 +31,18 @@
>>>>  #define UART_IERX_SLEEP		0x10 /* Enable sleep mode */
>>>>  
>>>>  #define UART_IIR	2	/* In:  Interrupt ID Register */
>>>> -#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
>>>> -#define UART_IIR_ID		0x0e /* Mask for the interrupt ID */
>>>>  #define UART_IIR_MSI		0x00 /* Modem status interrupt */
>>>> +#define UART_IIR_NO_INT		0x01 /* No interrupts pending */
>>>>  #define UART_IIR_THRI		0x02 /* Transmitter holding register empty
>*/
>>>>  #define UART_IIR_RDI		0x04 /* Receiver data interrupt */
>>>>  #define UART_IIR_RLSI		0x06 /* Receiver line status interrupt */
>>>> -
>>>>  #define UART_IIR_BUSY		0x07 /* DesignWare APB Busy Detect */
>>>> +#define UART_IIR_RX_TIMEOUT	0x0c /* DesignWare RX Timeout
>interrupt
>>> */
>> It was moved due to sorting. The comment could be changed maybe? I
>renamed it from omap to dw as i believe the omap also uses the dw ip.
>But i think it is a defacto standard mapping of the irr register?
>
>AFAIK, OMAP UART does not use DW core, please make these IDs generic to
>avoid confusion.
The ids are genetic, the comments are not. Ill update the comments to be generic, with the exception of the omap specific extended ones?

Olliver
>
>> 
>>>> +#define UART_IIR_MASK		0x0f /* DesignWare IIR mask */
>>>>  
>>>> -#define UART_IIR_RX_TIMEOUT	0x0c /* OMAP RX Timeout interrupt */
>>>
>>> You are removing UART_IIR_RX_TIMEOUT? Is this intended?
>>>
>>>>  #define UART_IIR_XOFF		0x10 /* OMAP XOFF/Special Character */
>>>>  #define UART_IIR_CTS_RTS_DSR	0x20 /* OMAP CTS/RTS/DSR Change */
>>>> +#define UART_IIR_EXT_MASK	0x30 /* OMAP extended IIR mask */
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1613112

While you're fixing this, there's a bug in samples/vfio-mdev/mtty.c:

		u8 ier = mdev_state->s[index].uart_reg[UART_IER];
		*buf = 0;

		mutex_lock(&mdev_state->rxtx_lock);
		/* Interrupt priority 1: Parity, overrun, framing or break */
		if ((ier & UART_IER_RLSI) && mdev_state->s[index].overrun)
			*buf |= UART_IIR_RLSI;

		/* Interrupt priority 2: Fifo trigger level reached */
		if ((ier & UART_IER_RDI) &&
		    (mdev_state->s[index].rxtx.count ==
		      mdev_state->s[index].intr_trigger_level))
			*buf |= UART_IIR_RDI;

		/* Interrupt priotiry 3: transmitter holding register empty */
		if ((ier & UART_IER_THRI) &&
		    (mdev_state->s[index].rxtx.head ==
				mdev_state->s[index].rxtx.tail))
			*buf |= UART_IIR_THRI;

		/* Interrupt priotiry 4: Modem status: CTS, DSR, RI or DCD  */
		if ((ier & UART_IER_MSI) &&
		    (mdev_state->s[index].uart_reg[UART_MCR] &
				 (UART_MCR_RTS | UART_MCR_DTR)))
			*buf |= UART_IIR_MSI;

		/* bit0: 0=> interrupt pending, 1=> no interrupt is pending */
		if (*buf == 0)
			*buf = UART_IIR_NO_INT;

It's treating the UART_IIR_* fields as a bitmask which is bad enough,
but in the "Interrupt priority 4" case, UART_IIR_MSI is zero, so 
"*buf |= UART_IIR_MSI" is a no-op.   And in the case where the modem
status interrupt is the only thing set, *buf will be 0, and UART_IIR_NO_INT
gets set erroneously.

So this is another example of the bug of trying to treat the
UART_IIR_* fields as a bitmask....

Yes, it's only sample code, but best fix it now before it gets copied
elsewhere and metastisizes.   :-)

							- Ted
							

[toc] | [prev] | [next] | [standalone]

#1613893

Hey Ted,

On 30-03-17 16:11, Theodore Ts'o wrote:
> While you're fixing this, there's a bug in samples/vfio-mdev/mtty.c:
>
> 		u8 ier = mdev_state->s[index].uart_reg[UART_IER];
> 		*buf = 0;
>
> 		mutex_lock(&mdev_state->rxtx_lock);
> 		/* Interrupt priority 1: Parity, overrun, framing or break */
> 		if ((ier & UART_IER_RLSI) && mdev_state->s[index].overrun)
> 			*buf |= UART_IIR_RLSI;
>
> 		/* Interrupt priority 2: Fifo trigger level reached */
> 		if ((ier & UART_IER_RDI) &&
> 		    (mdev_state->s[index].rxtx.count ==
> 		      mdev_state->s[index].intr_trigger_level))
> 			*buf |= UART_IIR_RDI;
>
> 		/* Interrupt priotiry 3: transmitter holding register empty */
> 		if ((ier & UART_IER_THRI) &&
> 		    (mdev_state->s[index].rxtx.head ==
> 				mdev_state->s[index].rxtx.tail))
> 			*buf |= UART_IIR_THRI;
>
> 		/* Interrupt priotiry 4: Modem status: CTS, DSR, RI or DCD  */
> 		if ((ier & UART_IER_MSI) &&
> 		    (mdev_state->s[index].uart_reg[UART_MCR] &
> 				 (UART_MCR_RTS | UART_MCR_DTR)))
> 			*buf |= UART_IIR_MSI;
>
> 		/* bit0: 0=> interrupt pending, 1=> no interrupt is pending */
> 		if (*buf == 0)
> 			*buf = UART_IIR_NO_INT;
>
> It's treating the UART_IIR_* fields as a bitmask which is bad enough,
> but in the "Interrupt priority 4" case, UART_IIR_MSI is zero, so
> "*buf |= UART_IIR_MSI" is a no-op.   And in the case where the modem
> status interrupt is the only thing set, *buf will be 0, and UART_IIR_NO_INT
> gets set erroneously.
>
> So this is another example of the bug of trying to treat the
> UART_IIR_* fields as a bitmask....
>
> Yes, it's only sample code, but best fix it now before it gets copied
> elsewhere and metastisizes.   :-)

Yeah, I notice that a lot of the code I modified in this patch was 
either copy pasted or 'inspired by'. So having bad examples around is 
really bad as you state!

Additionally the friendly build bot reminded me there are other 
subsystems that do this as well, so I'll update the patch to get those 
too and this one too.

Olliver

>
> 							- Ted
> 							
>

[toc] | [prev] | [next] | [standalone]

#1612855

On Wed, 2017-03-29 at 20:44 +0200, Olliver Schinagl wrote:
> It seems that at some point, someone made the assumption that the UART
> Interrupt ID Register was a bitfield and started to check if certain
> bits where set.
> 
> Actually however the register contains interrupt ID's where only the
> MSB
> seems to be used singular and the rest share at least one bit. Thus
> doing bitfield operations is wrong.
> 
> This patch cleans up the serial_reg include file by ordering it and
> replacing the UART_IIR_ID 'mask' with a proper mask for the register.
> The OMAP uart appears to have used the two commonly 'reserved' bits 4
> and 5 and thus get an UART_IIR_EXT_MASK for these two bits.
> 
> This patch then goes over all UART_IIR_* users and changes the code
> from
> bitfield checking, to ID checking instead.


Looking to implementation I would rather go with some helper like

int serial_in_IIR(port, [additional mask])
{
 return port->serial_in(port, UART_IIR) & (_IIR_MASK [| additional
mask]);
}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1614048

Hey Andy,

On 30-03-17 11:56, Andy Shevchenko wrote:
> On Wed, 2017-03-29 at 20:44 +0200, Olliver Schinagl wrote:
>> It seems that at some point, someone made the assumption that the UART
>> Interrupt ID Register was a bitfield and started to check if certain
>> bits where set.
>>
>> Actually however the register contains interrupt ID's where only the
>> MSB
>> seems to be used singular and the rest share at least one bit. Thus
>> doing bitfield operations is wrong.
>>
>> This patch cleans up the serial_reg include file by ordering it and
>> replacing the UART_IIR_ID 'mask' with a proper mask for the register.
>> The OMAP uart appears to have used the two commonly 'reserved' bits 4
>> and 5 and thus get an UART_IIR_EXT_MASK for these two bits.
>>
>> This patch then goes over all UART_IIR_* users and changes the code
>> from
>> bitfield checking, to ID checking instead.
>
>
> Looking to implementation I would rather go with some helper like
>
> int serial_in_IIR(port, [additional mask])
> {
>  return port->serial_in(port, UART_IIR) & (_IIR_MASK [| additional
> mask]);
> }

As I just wrote a simply static inline helper function in serial_core.h, 
I just figured that the helper will only work for some of the calls. All 
interrupt checks in xxx_serial_in() obviously can't rely on this. So do 
you still want this helper function added for the other cases? Or have 
all implementations do the masking manually?

And then, is iir = serial_port_in(up, UART_IIR) & UART_IIR_MASK; 
preferred over splitting it over two lines, like I did?

Finally, why rename it to _IIR_MASK, I assume a typo here?

Olliver

>

[toc] | [prev] | [next] | [standalone]

#1614082

On Fri, Mar 31, 2017 at 4:54 PM, Olliver Schinagl
<o.schinagl@ultimaker.com> wrote:
> On 30-03-17 11:56, Andy Shevchenko wrote:
>> On Wed, 2017-03-29 at 20:44 +0200, Olliver Schinagl wrote:

>> Looking to implementation I would rather go with some helper like
>>
>> int serial_in_IIR(port, [additional mask])
>> {
>>  return port->serial_in(port, UART_IIR) & (_IIR_MASK [| additional
>> mask]);
>> }

> As I just wrote a simply static inline helper function in serial_core.h, I
> just figured that the helper will only work for some of the calls. All
> interrupt checks in xxx_serial_in() obviously can't rely on this. So do you
> still want this helper function added for the other cases? Or have all
> implementations do the masking manually?

You have still few places (3+ IIRC) where it makes sense.

> And then, is iir = serial_port_in(up, UART_IIR) & UART_IIR_MASK; preferred
> over splitting it over two lines, like I did?

With given indentation it might be long enough to uglify the code.

So, I would still go with one / two helpers (do your own choice), but
if you insist that is not beneficial I would not object in-place
masking.

static inline int serial_in_IIR_mask(port, mask)
{
 return ... & mask;
}

static inline int serial_in_IIR(port)
{
return serial_in_IIR_mask(port, ..._IIR_MASK);
}

> Finally, why rename it to _IIR_MASK, I assume a typo here?

I usually do such to minimize characters to type (notice leading _
which means I referred to a suffix) and that's why the work "like" is
used above implying you need to modify to function correctly.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1613002

Hi Olliver,

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on v4.11-rc4 next-20170330]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Olliver-Schinagl/serial-Do-not-treat-the-IIR-register-as-a-bitfield/20170330-165826
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:264:8: sparse: attribute 'no_sanitize_address': unknown attribute
   drivers/staging/media/lirc/lirc_sir.c:285:44: sparse: undefined identifier 'UART_IIR_ID'
   drivers/staging/media/lirc/lirc_sir.c:286:29: sparse: undefined identifier 'UART_IIR_ID'
>> drivers/staging/media/lirc/lirc_sir.c:287:22: sparse: incompatible types for 'case' statement
   drivers/staging/media/lirc/lirc_sir.c:290:22: sparse: incompatible types for 'case' statement
   drivers/staging/media/lirc/lirc_sir.c:293:22: sparse: incompatible types for 'case' statement
   drivers/staging/media/lirc/lirc_sir.c:299:22: sparse: incompatible types for 'case' statement
   drivers/staging/media/lirc/lirc_sir.c: In function 'sir_interrupt':
   drivers/staging/media/lirc/lirc_sir.c:285:37: error: 'UART_IIR_ID' undeclared (first use in this function)
     while ((iir = inb(io + UART_IIR) & UART_IIR_ID)) {
                                        ^~~~~~~~~~~
   drivers/staging/media/lirc/lirc_sir.c:285:37: note: each undeclared identifier is reported only once for each function it appears in

vim +/case +287 drivers/staging/media/lirc/lirc_sir.c

34668350 drivers/staging/media/lirc/lirc_sir.c Ksenija Stanojevic 2015-05-22  279  	ktime_t curr_time;
34668350 drivers/staging/media/lirc/lirc_sir.c Ksenija Stanojevic 2015-05-22  280  	static unsigned long delt;
34668350 drivers/staging/media/lirc/lirc_sir.c Ksenija Stanojevic 2015-05-22  281  	unsigned long deltintr;
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26  282  	unsigned long flags;
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26  283  	int iir, lsr;
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26  284  
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26 @285  	while ((iir = inb(io + UART_IIR) & UART_IIR_ID)) {
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26  286  		switch (iir&UART_IIR_ID) { /* FIXME toto treba preriedit */
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26 @287  		case UART_IIR_MSI:
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26  288  			(void) inb(io + UART_MSR);
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26  289  			break;
404f3e95 drivers/staging/lirc/lirc_sir.c       Jarod Wilson       2010-07-26  290  		case UART_IIR_RLSI:

:::::: The code at line 287 was first introduced by commit
:::::: 404f3e956bc7ab03ac604fabf136e69607315f60 V4L/DVB: staging/lirc: add lirc_sir driver

:::::: TO: Jarod Wilson <jarod@redhat.com>
:::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web