[PATCH] bsps/i386: Separate variable for i8259 IRQs disable due to in progress state.

Joel Sherrill joel at rtems.org
Sun Sep 25 01:28:56 UTC 2016


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.

Is there still a print when one occurs?

On Sep 24, 2016 8:24 PM, "Chris Johns" <chrisj at rtems.org> wrote:

> Hi Pavel,
>
> Thank you for this. What testing has the patch had?
>
> 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.
>
> I am currently unable to test this patch and I am not sure when I can. I
> will do my best next week.
>
> On 25/09/2016 10:48, Pavel Pisa wrote:
>
>> From: Pavel Pisa <pisa at cmp.felk.cvut.cz>
>>
>> The global state of enabled and disabled interrupts has to hold
>> interrupts really disabled by drivers and system. If the state is
>> combined with interrupts temporarily disabled because they are
>> processed at given time then it is impossible to maintain state
>> by interrupt handlers in drivers.
>> ---
>>   c/src/lib/libbsp/i386/shared/irq/irq.c | 51
>> +++++++++++++++++++++-------------
>>   1 file changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/c/src/lib/libbsp/i386/shared/irq/irq.c
>> b/c/src/lib/libbsp/i386/shared/irq/irq.c
>> index 0511257..8316fe6 100644
>> --- a/c/src/lib/libbsp/i386/shared/irq/irq.c
>> +++ b/c/src/lib/libbsp/i386/shared/irq/irq.c
>> @@ -51,7 +51,22 @@ static enum intr_trigger irq_trigger[BSP_IRQ_LINES_NUMB
>> ER];
>>    * while upper bits are interrupt on the slave PIC.
>>    * This cache is initialized in ldseg.s
>>    */
>> -static rtems_i8259_masks i8259a_cache = 0xFFFB;
>> +static rtems_i8259_masks i8259a_imr_cache = 0xFFFB;
>> +static rtems_i8259_masks i8259a_in_progress = 0;
>>
>
> Should these be volatile to be sure?
>
> +
>> +static inline
>> +void BSP_i8259a_irq_update_master_imr( void )
>> +{
>> +  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
>> +  outport_byte( PIC_MASTER_IMR_IO_PORT, mask & 0xff );
>> +}
>> +
>> +static inline
>> +void BSP_i8259a_irq_update_slave_imr( void )
>> +{
>> +  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
>> +  outport_byte( PIC_SLAVE_IMR_IO_PORT, ( mask >> 8 ) & 0xff );
>> +}
>>
>>   /*
>>    * Print the stats.
>> @@ -107,7 +122,7 @@ static inline uint8_t BSP_i8259a_irq_in_service_reg(uint32_t
>> ioport)
>>   /*----------------------------------------------------------
>> ---------------+
>>   |         Function:  BSP_irq_disable_at_i8259a
>>   |      Description: Mask IRQ line in appropriate PIC chip.
>> -| Global Variables: i8259a_cache
>> +| Global Variables: i8259a_imr_cache, i8259a_in_progress
>>   |        Arguments: vector_offset - number of IRQ line to mask.
>>   |          Returns: 0 is OK.
>>   +-----------------------------------------------------------
>> ---------------*/
>> @@ -119,15 +134,15 @@ static int BSP_irq_disable_at_i8259a(const
>> rtems_irq_number irqLine)
>>     rtems_interrupt_disable(level);
>>
>>     mask = 1 << irqLine;
>> -  i8259a_cache |= mask;
>> +  i8259a_imr_cache |= mask;
>>
>>     if (irqLine < 8)
>>     {
>> -    outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
>> +    BSP_i8259a_irq_update_master_imr();
>>     }
>>     else
>>     {
>> -    outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
>> +    BSP_i8259a_irq_update_slave_imr();
>>     }
>>
>>     rtems_interrupt_enable(level);
>> @@ -138,7 +153,7 @@ static int BSP_irq_disable_at_i8259a(const
>> rtems_irq_number irqLine)
>>   /*----------------------------------------------------------
>> ---------------+
>>   |         Function:  BSP_irq_enable_at_i8259a
>>   |      Description: Unmask IRQ line in appropriate PIC chip.
>> -| Global Variables: i8259a_cache
>> +| Global Variables: i8259a_imr_cache, i8259a_in_progress
>>   |        Arguments: irqLine - number of IRQ line to mask.
>>   |          Returns: Nothing.
>>   +-----------------------------------------------------------
>> ---------------*/
>> @@ -152,19 +167,19 @@ static int BSP_irq_enable_at_i8259a(const
>> rtems_irq_number irqLine)
>>     rtems_interrupt_disable(level);
>>
>>     mask = 1 << irqLine;
>> -  i8259a_cache &= ~mask;
>> +  i8259a_imr_cache &= ~mask;
>>
>>     if (irqLine < 8)
>>     {
>>       isr = BSP_i8259a_irq_in_service_reg(PIC_MASTER_COMMAND_IO_PORT);
>>       irr = BSP_i8259a_irq_int_request_reg(PIC_MASTER_COMMAND_IO_PORT);
>> -    outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
>> +    BSP_i8259a_irq_update_master_imr();
>>     }
>>     else
>>     {
>>       isr = BSP_i8259a_irq_in_service_reg(PIC_SLAVE_COMMAND_IO_PORT);
>>       irr = BSP_i8259a_irq_int_request_reg(PIC_SLAVE_COMMAND_IO_PORT);
>> -    outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
>> +    BSP_i8259a_irq_update_slave_imr();
>>     }
>>
>>     if (((isr ^ irr) & mask) != 0)
>> @@ -298,7 +313,7 @@ void BSP_dispatch_isr(int vector);
>>
>>   void BSP_dispatch_isr(int vector)
>>   {
>> -  uint16_t old_imr = 0;
>> +  rtems_i8259_masks in_progress_save = 0;
>>
>>     if (vector < BSP_IRQ_VECTOR_NUMBER) {
>>       /*
>> @@ -329,10 +344,10 @@ void BSP_dispatch_isr(int vector)
>>          * vector clear.
>>          */
>>         if (vector <= BSP_IRQ_MAX_ON_i8259A) {
>> -        old_imr = i8259a_cache;
>> -        i8259a_cache |= irq_mask_or_tbl[vector];
>> -        outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
>> -        outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
>> +        in_progress_save = i8259a_in_progress;
>> +        i8259a_in_progress |= irq_mask_or_tbl[vector];
>> +        BSP_i8259a_irq_update_master_imr();
>> +        BSP_i8259a_irq_update_slave_imr();
>>         }
>>
>>         /*
>> @@ -365,11 +380,9 @@ void BSP_dispatch_isr(int vector)
>>          * again.
>>          */
>>         if (vector <= BSP_IRQ_MAX_ON_i8259A) {
>> -        if (irq_trigger[vector] == INTR_TRIGGER_LEVEL)
>> -          old_imr |= 1 << vector;
>>
>
> 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.
>
> Chris
>
> -        i8259a_cache = old_imr;
>> -        outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
>> -        outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
>> +        i8259a_in_progress = in_progress_save;
>> +        BSP_i8259a_irq_update_master_imr();
>> +        BSP_i8259a_irq_update_slave_imr();
>>         }
>>       }
>>     }
>>
>> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20160924/5bd707a0/attachment-0002.html>


More information about the devel mailing list