Re: [Qemu-devel] [PATCH] AHCI: Masking of IRQs actually masks them

[Alexander Graf]

On 09.02.2012, at 15:41, Kevin Wolf wrote:

> Am 30.01.2012 23:29, schrieb Alexander Graf:
>> When masking IRQ lines, we should actually mask them out and not declare
>> them active anymore. Once we mask them in again, they are allowed to trigger
>> again.
>> 
>> Signed-off-by: Alexander Graf <address@hidden>
>> ---
>> hw/ide/ahci.c |    2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>> 
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index c2c168d..f8e9eb4 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -146,6 +146,7 @@ static void ahci_check_irq(AHCIState *s)
>> 
>>     DPRINTF(-1, "check irq %#x\n", s->control_regs.irqstatus);
>> 
>> +    s->control_regs.irqstatus = 0;
>>     for (i = 0; i < s->ports; i++) {
>>         AHCIPortRegs *pr = &s->dev[i].port_regs;
>>         if (pr->irq_stat & pr->irq_mask) {
> 
> Is this an independent bug fix?

No, it's the same thing. Without this we only OR new interrupts into irqstatus. 
This way we also allow clearing of them when the producer vanishes / gets 
masked.

> 
>> @@ -216,6 +217,7 @@ static void  ahci_port_write(AHCIState *s, int port, int 
>> offset, uint32_t val)
>>             break;
>>         case PORT_IRQ_STAT:
>>             pr->irq_stat &= ~val;
>> +            ahci_check_irq(s);
>>             break;
>>         case PORT_IRQ_MASK:
>>             pr->irq_mask = val & 0xfdc000ff;
> 
> Makes some sense, but isn't really about masking interrupts either?
> (From the commit message I would have expected that you touch PORT_IRQ_MASK)

There are multiple layers of IRQ lines, wired to each other, with each layer 
being able to mask specific bits. We didn't catch the part where irq_stat was 
losing a bit, either by masking or by explicit unsetting.


Alex