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 <[email protected]>
---
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
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
On March 30, 2017 8:15:29 AM CEST, Vignesh R <[email protected]> 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.
On Thursday 30 March 2017 12:13 PM, Olliver Schinagl wrote:
>
>
> On March 30, 2017 8:15:29 AM CEST, Vignesh R <[email protected]> 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
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 <[email protected]>
Intel Finland Oy
Hi Olliver,
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.11-rc4 next-20170329]
[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
config: i386-randconfig-s0-201713 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/irda/ali-ircc.c: In function 'ali_ircc_sir_interrupt':
>> drivers/net/irda/ali-ircc.c:816:31: error: 'UART_IIR_ID' undeclared (first use in this function)
iir = inb(iobase+UART_IIR) & UART_IIR_ID;
^~~~~~~~~~~
drivers/net/irda/ali-ircc.c:816:31: note: each undeclared identifier is reported only once for each function it appears in
vim +/UART_IIR_ID +816 drivers/net/irda/ali-ircc.c
^1da177e Linus Torvalds 2005-04-16 810 int iobase;
^1da177e Linus Torvalds 2005-04-16 811 int iir, lsr;
^1da177e Linus Torvalds 2005-04-16 812
^1da177e Linus Torvalds 2005-04-16 813
^1da177e Linus Torvalds 2005-04-16 814 iobase = self->io.sir_base;
^1da177e Linus Torvalds 2005-04-16 815
^1da177e Linus Torvalds 2005-04-16 @816 iir = inb(iobase+UART_IIR) & UART_IIR_ID;
^1da177e Linus Torvalds 2005-04-16 817 if (iir) {
^1da177e Linus Torvalds 2005-04-16 818 /* Clear interrupt */
^1da177e Linus Torvalds 2005-04-16 819 lsr = inb(iobase+UART_LSR);
:::::: The code at line 816 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:::::: TO: Linus Torvalds <[email protected]>
:::::: CC: Linus Torvalds <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Olliver,
[auto build test ERROR on tty/tty-testing]
[also build test ERROR 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
config: x86_64-randconfig-i0-201713 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/net//irda/smsc-ircc2.c: In function 'smsc_ircc_interrupt_sir':
>> drivers/net//irda/smsc-ircc2.c:1582:33: error: 'UART_IIR_ID' undeclared (first use in this function)
iir = inb(iobase + UART_IIR) & UART_IIR_ID;
^
drivers/net//irda/smsc-ircc2.c:1582:33: note: each undeclared identifier is reported only once for each function it appears in
vim +/UART_IIR_ID +1582 drivers/net//irda/smsc-ircc2.c
^1da177e Linus Torvalds 2005-04-16 1576
25985edc Lucas De Marchi 2011-03-30 1577 /* Already locked coming here in smsc_ircc_interrupt() */
^1da177e Linus Torvalds 2005-04-16 1578 /*spin_lock(&self->lock);*/
^1da177e Linus Torvalds 2005-04-16 1579
^1da177e Linus Torvalds 2005-04-16 1580 iobase = self->io.sir_base;
^1da177e Linus Torvalds 2005-04-16 1581
^1da177e Linus Torvalds 2005-04-16 @1582 iir = inb(iobase + UART_IIR) & UART_IIR_ID;
^1da177e Linus Torvalds 2005-04-16 1583 if (iir == 0)
^1da177e Linus Torvalds 2005-04-16 1584 return IRQ_NONE;
^1da177e Linus Torvalds 2005-04-16 1585 while (iir) {
:::::: The code at line 1582 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:::::: TO: Linus Torvalds <[email protected]>
:::::: CC: Linus Torvalds <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, 2017-03-30 at 18:22 +0800, kbuild test robot wrote:
> drivers/net//irda/smsc-ircc2.c:
> > > drivers/net//irda/smsc-ircc2.c:
Just out of my curiosity, why do we have // in some reports?
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Thu, Mar 30, 2017 at 01:32:51PM +0300, Andy Shevchenko wrote:
>On Thu, 2017-03-30 at 18:22 +0800, kbuild test robot wrote:
>
>> drivers/net//irda/smsc-ircc2.c:
>
>> > > drivers/net//irda/smsc-ircc2.c:
>
>Just out of my curiosity, why do we have // in some reports?
Log shows the bisect runs
make M=drivers/net/
That should explain the // in error messages.
Thanks,
Fengguang
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 <[email protected]>
:::::: CC: Mauro Carvalho Chehab <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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
Hey Vignesh,
On March 30, 2017 9:57:19 AM CEST, Vignesh R <[email protected]> wrote:
>
>
>On Thursday 30 March 2017 12:13 PM, Olliver Schinagl wrote:
>>
>>
>> On March 30, 2017 8:15:29 AM CEST, Vignesh R <[email protected]> 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.
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
>
>
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
>
On Fri, Mar 31, 2017 at 4:54 PM, Olliver Schinagl
<[email protected]> 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