[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