Patchwork AHCI: Masking of IRQs actually masks them

login
register
mail settings
Submitter Alexander Graf
Date Jan. 30, 2012, 10:29 p.m.
Message ID <1327962588-5230-3-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/138664/
State New
Headers show

Comments

Alexander Graf - Jan. 30, 2012, 10:29 p.m.
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(-)
Kevin Wolf - Feb. 9, 2012, 2:41 p.m.
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
Alexander Graf - Feb. 9, 2012, 2:52 p.m.
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

Patch

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