[PATCH] vfio-ccw: Attempt to clean up all IRQs on error

[PATCH] vfio-ccw: Attempt to clean up all IRQs on error

[Eric Farman]

The vfio_ccw_unrealize() routine makes an unconditional attempt to
unregister every IRQ notifier, though they may not have been registered
in the first place (when running on an older kernel, for example).

Let's mirror this behavior in the error cleanups in vfio_ccw_realize()
so that if/when new IRQs are added, it is less confusing to recognize
the necessary procedures. The worst case scenario would be some extra
messages about an undefined IRQ, but since this is an error exit that
won't be the only thing to worry about.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/vfio/ccw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 400bc07fe2..169438c3b8 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -690,7 +690,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
     if (vcdev->crw_region) {
         vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err);
         if (err) {
-            goto out_crw_notifier_err;
+            goto out_irq_notifier_err;
         }
     }
 
@@ -705,7 +705,9 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
 
     return;
 
-out_crw_notifier_err:
+out_irq_notifier_err:
+    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX);
+    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX);
     vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
 out_io_notifier_err:
     vfio_ccw_put_region(vcdev);
-- 
2.25.1

Re: [PATCH] vfio-ccw: Attempt to clean up all IRQs on error

[Cornelia Huck]

On Tue, 27 Apr 2021 16:25:11 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> The vfio_ccw_unrealize() routine makes an unconditional attempt to
> unregister every IRQ notifier, though they may not have been registered
> in the first place (when running on an older kernel, for example).
> 
> Let's mirror this behavior in the error cleanups in vfio_ccw_realize()
> so that if/when new IRQs are added, it is less confusing to recognize
> the necessary procedures. The worst case scenario would be some extra
> messages about an undefined IRQ, but since this is an error exit that
> won't be the only thing to worry about.

I'm wondering whether we should downgrade the moan during unregister
from error to warn; the code deals with it gracefully, and we're not
losing any functionality.

Patch looks good.

> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/vfio/ccw.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 400bc07fe2..169438c3b8 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -690,7 +690,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>      if (vcdev->crw_region) {
>          vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err);
>          if (err) {
> -            goto out_crw_notifier_err;
> +            goto out_irq_notifier_err;
>          }
>      }
>  
> @@ -705,7 +705,9 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>  
>      return;
>  
> -out_crw_notifier_err:
> +out_irq_notifier_err:
> +    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX);
> +    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX);
>      vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
>  out_io_notifier_err:
>      vfio_ccw_put_region(vcdev);

Re: [PATCH] vfio-ccw: Attempt to clean up all IRQs on error

[Eric Farman]

On Wed, 2021-04-28 at 12:37 +0200, Cornelia Huck wrote:
> On Tue, 27 Apr 2021 16:25:11 +0200
> Eric Farman <farman@linux.ibm.com> wrote:
> 
> > The vfio_ccw_unrealize() routine makes an unconditional attempt to
> > unregister every IRQ notifier, though they may not have been
> > registered
> > in the first place (when running on an older kernel, for example).
> > 
> > Let's mirror this behavior in the error cleanups in
> > vfio_ccw_realize()
> > so that if/when new IRQs are added, it is less confusing to
> > recognize
> > the necessary procedures. The worst case scenario would be some
> > extra
> > messages about an undefined IRQ, but since this is an error exit
> > that
> > won't be the only thing to worry about.
> 
> I'm wondering whether we should downgrade the moan during unregister
> from error to warn; the code deals with it gracefully, and we're not
> losing any functionality.

Ha, I thought it already was a warning. I think that's a good idea, let
me spin a quick v2.

> 
> Patch looks good.

Thanks,
Eric

> 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  hw/vfio/ccw.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index 400bc07fe2..169438c3b8 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -690,7 +690,7 @@ static void vfio_ccw_realize(DeviceState *dev,
> > Error **errp)
> >      if (vcdev->crw_region) {
> >          vfio_ccw_register_irq_notifier(vcdev,
> > VFIO_CCW_CRW_IRQ_INDEX, &err);
> >          if (err) {
> > -            goto out_crw_notifier_err;
> > +            goto out_irq_notifier_err;
> >          }
> >      }
> >  
> > @@ -705,7 +705,9 @@ static void vfio_ccw_realize(DeviceState *dev,
> > Error **errp)
> >  
> >      return;
> >  
> > -out_crw_notifier_err:
> > +out_irq_notifier_err:
> > +    vfio_ccw_unregister_irq_notifier(vcdev,
> > VFIO_CCW_REQ_IRQ_INDEX);
> > +    vfio_ccw_unregister_irq_notifier(vcdev,
> > VFIO_CCW_CRW_IRQ_INDEX);
> >      vfio_ccw_unregister_irq_notifier(vcdev,
> > VFIO_CCW_IO_IRQ_INDEX);
> >  out_io_notifier_err:
> >      vfio_ccw_put_region(vcdev);