<p dir="ltr">We have an old PC of mine at the office which is pretty reliable at generating the spurious interrupt. I won't be there this week though.</p>
<p dir="ltr">Is there still a print when one occurs?</p>
<div class="gmail_extra"><br><div class="gmail_quote">On Sep 24, 2016 8:24 PM, "Chris Johns" <<a href="mailto:chrisj@rtems.org">chrisj@rtems.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Pavel,<br>
<br>
Thank you for this. What testing has the patch had?<br>
<br>
It needs to be tested on real hardware and hardware which showed the spurious interrupt issue. The issue appears around the interrupt enable call and my guess is it relates to some type of a race condition in the PIC between the signals and some piece of register timing. I timed the spurious interrupt always appearing usecs after the enable.<br>
<br>
I am currently unable to test this patch and I am not sure when I can. I will do my best next week.<br>
<br>
On 25/09/2016 10:48, Pavel Pisa wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
From: Pavel Pisa <<a href="mailto:pisa@cmp.felk.cvut.cz" target="_blank">pisa@cmp.felk.cvut.cz</a>><br>
<br>
The global state of enabled and disabled interrupts has to hold<br>
interrupts really disabled by drivers and system. If the state is<br>
combined with interrupts temporarily disabled because they are<br>
processed at given time then it is impossible to maintain state<br>
by interrupt handlers in drivers.<br>
---<br>
c/src/lib/libbsp/i386/shared/i<wbr>rq/irq.c | 51 +++++++++++++++++++++---------<wbr>----<br>
1 file changed, 32 insertions(+), 19 deletions(-)<br>
<br>
diff --git a/c/src/lib/libbsp/i386/shared<wbr>/irq/irq.c b/c/src/lib/libbsp/i386/shared<wbr>/irq/irq.c<br>
index 0511257..8316fe6 100644<br>
--- a/c/src/lib/libbsp/i386/shared<wbr>/irq/irq.c<br>
+++ b/c/src/lib/libbsp/i386/shared<wbr>/irq/irq.c<br>
@@ -51,7 +51,22 @@ static enum intr_trigger irq_trigger[BSP_IRQ_LINES_NUMB<wbr>ER];<br>
* while upper bits are interrupt on the slave PIC.<br>
* This cache is initialized in ldseg.s<br>
*/<br>
-static rtems_i8259_masks i8259a_cache = 0xFFFB;<br>
+static rtems_i8259_masks i8259a_imr_cache = 0xFFFB;<br>
+static rtems_i8259_masks i8259a_in_progress = 0;<br>
</blockquote>
<br>
Should these be volatile to be sure?<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+static inline<br>
+void BSP_i8259a_irq_update_master_i<wbr>mr( void )<br>
+{<br>
+ rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;<br>
+ outport_byte( PIC_MASTER_IMR_IO_PORT, mask & 0xff );<br>
+}<br>
+<br>
+static inline<br>
+void BSP_i8259a_irq_update_slave_im<wbr>r( void )<br>
+{<br>
+ rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;<br>
+ outport_byte( PIC_SLAVE_IMR_IO_PORT, ( mask >> 8 ) & 0xff );<br>
+}<br>
<br>
/*<br>
* Print the stats.<br>
@@ -107,7 +122,7 @@ static inline uint8_t BSP_i8259a_irq_in_service_reg(<wbr>uint32_t ioport)<br>
/*----------------------------<wbr>------------------------------<wbr>---------------+<br>
| Function: BSP_irq_disable_at_i8259a<br>
| Description: Mask IRQ line in appropriate PIC chip.<br>
-| Global Variables: i8259a_cache<br>
+| Global Variables: i8259a_imr_cache, i8259a_in_progress<br>
| Arguments: vector_offset - number of IRQ line to mask.<br>
| Returns: 0 is OK.<br>
+-----------------------------<wbr>------------------------------<wbr>---------------*/<br>
@@ -119,15 +134,15 @@ static int BSP_irq_disable_at_i8259a(cons<wbr>t rtems_irq_number irqLine)<br>
rtems_interrupt_disable(level)<wbr>;<br>
<br>
mask = 1 << irqLine;<br>
- i8259a_cache |= mask;<br>
+ i8259a_imr_cache |= mask;<br>
<br>
if (irqLine < 8)<br>
{<br>
- outport_byte(PIC_MASTER_IMR_IO<wbr>_PORT, i8259a_cache & 0xff);<br>
+ BSP_i8259a_irq_update_master_i<wbr>mr();<br>
}<br>
else<br>
{<br>
- outport_byte(PIC_SLAVE_IMR_IO_<wbr>PORT, (i8259a_cache >> 8) & 0xff);<br>
+ BSP_i8259a_irq_update_slave_im<wbr>r();<br>
}<br>
<br>
rtems_interrupt_enable(level);<br>
@@ -138,7 +153,7 @@ static int BSP_irq_disable_at_i8259a(cons<wbr>t rtems_irq_number irqLine)<br>
/*----------------------------<wbr>------------------------------<wbr>---------------+<br>
| Function: BSP_irq_enable_at_i8259a<br>
| Description: Unmask IRQ line in appropriate PIC chip.<br>
-| Global Variables: i8259a_cache<br>
+| Global Variables: i8259a_imr_cache, i8259a_in_progress<br>
| Arguments: irqLine - number of IRQ line to mask.<br>
| Returns: Nothing.<br>
+-----------------------------<wbr>------------------------------<wbr>---------------*/<br>
@@ -152,19 +167,19 @@ static int BSP_irq_enable_at_i8259a(const rtems_irq_number irqLine)<br>
rtems_interrupt_disable(level)<wbr>;<br>
<br>
mask = 1 << irqLine;<br>
- i8259a_cache &= ~mask;<br>
+ i8259a_imr_cache &= ~mask;<br>
<br>
if (irqLine < 8)<br>
{<br>
isr = BSP_i8259a_irq_in_service_reg(<wbr>PIC_MASTER_COMMAND_IO_PORT);<br>
irr = BSP_i8259a_irq_int_request_reg<wbr>(PIC_MASTER_COMMAND_IO_PORT);<br>
- outport_byte(PIC_MASTER_IMR_IO<wbr>_PORT, i8259a_cache & 0xff);<br>
+ BSP_i8259a_irq_update_master_i<wbr>mr();<br>
}<br>
else<br>
{<br>
isr = BSP_i8259a_irq_in_service_reg(<wbr>PIC_SLAVE_COMMAND_IO_PORT);<br>
irr = BSP_i8259a_irq_int_request_reg<wbr>(PIC_SLAVE_COMMAND_IO_PORT);<br>
- outport_byte(PIC_SLAVE_IMR_IO_<wbr>PORT, (i8259a_cache >> 8) & 0xff);<br>
+ BSP_i8259a_irq_update_slave_im<wbr>r();<br>
}<br>
<br>
if (((isr ^ irr) & mask) != 0)<br>
@@ -298,7 +313,7 @@ void BSP_dispatch_isr(int vector);<br>
<br>
void BSP_dispatch_isr(int vector)<br>
{<br>
- uint16_t old_imr = 0;<br>
+ rtems_i8259_masks in_progress_save = 0;<br>
<br>
if (vector < BSP_IRQ_VECTOR_NUMBER) {<br>
/*<br>
@@ -329,10 +344,10 @@ void BSP_dispatch_isr(int vector)<br>
* vector clear.<br>
*/<br>
if (vector <= BSP_IRQ_MAX_ON_i8259A) {<br>
- old_imr = i8259a_cache;<br>
- i8259a_cache |= irq_mask_or_tbl[vector];<br>
- outport_byte(PIC_MASTER_IMR_IO<wbr>_PORT, i8259a_cache & 0xff);<br>
- outport_byte(PIC_SLAVE_IMR_IO_<wbr>PORT, (i8259a_cache >> 8) & 0xff);<br>
+ in_progress_save = i8259a_in_progress;<br>
+ i8259a_in_progress |= irq_mask_or_tbl[vector];<br>
+ BSP_i8259a_irq_update_master_i<wbr>mr();<br>
+ BSP_i8259a_irq_update_slave_im<wbr>r();<br>
}<br>
<br>
/*<br>
@@ -365,11 +380,9 @@ void BSP_dispatch_isr(int vector)<br>
* again.<br>
*/<br>
if (vector <= BSP_IRQ_MAX_ON_i8259A) {<br>
- if (irq_trigger[vector] == INTR_TRIGGER_LEVEL)<br>
- old_imr |= 1 << vector;<br>
</blockquote>
<br>
Looking at the interrupt server I see a disable in the trigger call which means it should be ok to remove this code. I am wondering if this piece of code is an artefact of the comparison of the code in FreeBSD I did which always uses an interrupt server and may assume the interrupt is masked.<br>
<br>
Chris<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- i8259a_cache = old_imr;<br>
- outport_byte(PIC_MASTER_IMR_IO<wbr>_PORT, i8259a_cache & 0xff);<br>
- outport_byte(PIC_SLAVE_IMR_IO_<wbr>PORT, (i8259a_cache >> 8) & 0xff);<br>
+ i8259a_in_progress = in_progress_save;<br>
+ BSP_i8259a_irq_update_master_i<wbr>mr();<br>
+ BSP_i8259a_irq_update_slave_im<wbr>r();<br>
}<br>
}<br>
}<br>
<br>
</blockquote>
______________________________<wbr>_________________<br>
devel mailing list<br>
<a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
<a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman<wbr>/listinfo/devel</a><br>
</blockquote></div></div>