Re: [Qemu-devel] [PATCH v2 3/5] cadence_gem: Correct the interupt logic

[Alistair Francis <alistair23@gmail.com>]

On Mon, Apr 10, 2017 at 4:43 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch fixes two mistakes in the interrupt logic.
>
> First we only trigger single-queue or multi-queue interrupts if the status
> register is set. This logic was already used for non multi-queue interrupts
> but it also applies to multi-queue interrupts.
>
> Secondly we need to lower the interrupts if the ISR isn't set. As part
> of this we can remove the other interrupt lowering logic and consolidate
> it inside gem_update_int_status().
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>

This patch can be cleaned up more, I'll send a V3.

Thanks,

Alistair

> ---
>
>  hw/net/cadence_gem.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index a66a9cc..fc3a184 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -509,7 +509,18 @@ static void gem_update_int_status(CadenceGEMState *s)
>  {
>      int i;
>
> -    if ((s->num_priority_queues == 1) && s->regs[GEM_ISR]) {
> +    if (!s->regs[GEM_ISR]) {
> +        /* ISR isn't set, clear all the interrupts */
> +        for (i = 0; i < s->num_priority_queues; ++i) {
> +            qemu_set_irq(s->irq[i], 0);
> +        }
> +        return;
> +    }
> +
> +    /* If we get here we know s->regs[GEM_ISR] is set, so we don't need to
> +     * check it again.
> +     */
> +    if (s->num_priority_queues == 1) {
>          /* No priority queues, just trigger the interrupt */
>          DB_PRINT("asserting int.\n");
>          qemu_set_irq(s->irq[0], 1);
> @@ -1274,7 +1285,6 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      CadenceGEMState *s;
>      uint32_t retval;
> -    int i;
>      s = (CadenceGEMState *)opaque;
>
>      offset >>= 2;
> @@ -1285,9 +1295,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>      switch (offset) {
>      case GEM_ISR:
>          DB_PRINT("lowering irqs on ISR read\n");
> -        for (i = 0; i < s->num_priority_queues; ++i) {
> -            qemu_set_irq(s->irq[i], 0);
> -        }
> +        gem_update_int_status(s);
>          break;
>      case GEM_PHYMNTNC:
>          if (retval & GEM_PHYMNTNC_OP_R) {
> --
> 2.9.3
>