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 <alistair.francis@xilinx.com>
---

 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);
         }
-- 
2.9.3

On 04/04/2017 08:40 PM, Alistair Francis 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 <alistair.francis@xilinx.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>
>  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);
>          }
>

On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
> ---
>
>  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.

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?

thanks
-- PMM

On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>> ---
>>
>>  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

On 10 April 2017 at 23:23, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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.

On QEMU reset the other end will be reset anyway so it will
update its idea of whether the irq is asserted (and calling
qemu_set_irq in a reset function is generally not a good idea).
If the device has guest-programmable reset of some kind you'd
need to clear the irq lines then, though.

thanks
-- PMM