AHCI: Masking of IRQs actually masks them

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 <agraf@suse.de>
---
 hw/ide/ahci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

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 <agraf@suse.de>
> ---
>  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?

> @@ -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)

Kevin

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 <agraf@suse.de>
>> ---
>> 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