Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set

[Alistair Francis]

On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <address@hidden> wrote:
> On 5 April 2017 at 00:40, Alistair Francis <address@hidden> wrote:
>> Only trigger multi-queue GEM interrupts if the interrupt status register
>> is set. This logic was already used for non multi-queue interrupts but
>> it also applies to multi-queue interrupts.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>>
>>  hw/net/cadence_gem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 3e37665..b9eaed4 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
>>      }
>>
>>      for (i = 0; i < s->num_priority_queues; ++i) {
>> -        if (s->regs[GEM_INT_Q1_STATUS + i]) {
>> +        if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
>>              DB_PRINT("asserting int. (q=%d)\n", i);
>>              qemu_set_irq(s->irq[i], 1);
>>          }
>
> With this change, if s->regs[GEM_ISR] is zero then the entire
> function never does anything, so you could hoist that up to
> the start, rather than testing it inside the loop and in the
> previous conditional.

Yep, I have moved it to the top.

>
> Also the comment says "raise or lower interrupt based on current
> status", but the code will only ever do qemu_set_irq(..., 1),
> never 0. Which is right?

This is a little confusing. The interrupts are lowered when the ISR is
read, so the assumption was that we never need to clear them in the
gem_update_int_status(). Although then if we perform a reset nothing
will clear the interrupts until the ISR is read from. So that is a
bug. I'll update this patch to fix this up in V2.

Thanks,

Alistair

>
> thanks
> -- PMM