diff mbox

missing vgic_unlock_rank in gic_remove_irq_from_guest

Message ID a09fd672-9ba0-9505-462b-88e724899740@epam.com (mailing list archive)
State New, archived
Headers show

Commit Message

Artem Mygaiev Dec. 9, 2016, 2:12 p.m. UTC 
Hi Stefano

On 09.12.16 02:59, Stefano Stabellini wrote:
> Add missing vgic_unlock_rank on the error path in
> gic_remove_irq_from_guest.
>
> CID: 1381843
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 63c744a..a5348f2 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -205,7 +205,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>           */
>          if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
>               !test_bit(_IRQ_DISABLED, &desc->status) )
> +        {
> +            vgic_unlock_rank(v_target, rank, flags);
>              return -EBUSY;
> +        }
>      }
>  
>      clear_bit(_IRQ_GUEST, &desc->status);
>
>
would it be better to do it in the same way it is done in gic_route_irq_to_guest() just above the patched function?
Something like:

Comments

Stefano Stabellini Dec. 9, 2016, 7:35 p.m. UTC | #1 
On Fri, 9 Dec 2016, Artem Mygaiev wrote:
> Hi Stefano
> 
> On 09.12.16 02:59, Stefano Stabellini wrote:
> > Add missing vgic_unlock_rank on the error path in
> > gic_remove_irq_from_guest.
> >
> > CID: 1381843
> >
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> >
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 63c744a..a5348f2 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -205,7 +205,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> >           */
> >          if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
> >               !test_bit(_IRQ_DISABLED, &desc->status) )
> > +        {
> > +            vgic_unlock_rank(v_target, rank, flags);
> >              return -EBUSY;
> > +        }
> >      }
> >  
> >      clear_bit(_IRQ_GUEST, &desc->status);
> >
> >
> would it be better to do it in the same way it is done in gic_route_irq_to_guest() just above the patched function?

Yes, that is the preferred way of handling error paths, especially when
there are multiple. In this case, there is just one, so it doesn't make
a difference.


> Something like:
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 63c744a..a9bf5d9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -181,6 +181,7 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>      struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>      struct pending_irq *p = irq_to_pending(v_target, virq);
>      unsigned long flags;
> +    int res = -EBUSY;
>  
>      ASSERT(spin_is_locked(&desc->lock));
>      ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> @@ -205,17 +206,19 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>           */
>          if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
>               !test_bit(_IRQ_DISABLED, &desc->status) )
> -            return -EBUSY;
> +            goto out;
>      }
>  
>      clear_bit(_IRQ_GUEST, &desc->status);
>      desc->handler = &no_irq_type;
>  
>      p->desc = NULL;
> +    res = 0;
>  
> +out:
>      vgic_unlock_rank(v_target, rank, flags);
>  
> -    return 0;
> +    return res;
>  }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>
diff mbox

Patch

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 63c744a..a9bf5d9 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -181,6 +181,7 @@  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
     struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
     struct pending_irq *p = irq_to_pending(v_target, virq);
     unsigned long flags;
+    int res = -EBUSY;
 
     ASSERT(spin_is_locked(&desc->lock));
     ASSERT(test_bit(_IRQ_GUEST, &desc->status));
@@ -205,17 +206,19 @@  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
          */
         if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
              !test_bit(_IRQ_DISABLED, &desc->status) )
-            return -EBUSY;
+            goto out;
     }
 
     clear_bit(_IRQ_GUEST, &desc->status);
     desc->handler = &no_irq_type;
 
     p->desc = NULL;
+    res = 0;
 
+out:
     vgic_unlock_rank(v_target, rank, flags);
 
-    return 0;
+    return res;
 }