| Message ID | 20210428143652.1571487-1-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] vfio-ccw: Attempt to clean up all IRQs on error | expand |
On 4/28/21 10:36 AM, Eric Farman 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. > > And regarding those messages, let's change it to a warning instead of > an error, to better reflect their severity. The existing code in both > paths handles everything anyway. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> Based on our prior conversation re: the safety of unconditional calls to vfio_ccw_unregister_irq_notifier, this looks good to me. Acked-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > > Notes: > v1->v2: > - Downgrade unregister IRQ message from error to warning [CH] > > v1: https://lore.kernel.org/qemu-devel/20210427142511.2125733-1-farman@linux.ibm.com/ > > hw/vfio/ccw.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 400bc07fe2..f64dd850db 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -470,7 +470,7 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, > > if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0, > VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { > - error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name); > + warn_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name); > } > > qemu_set_fd_handler(event_notifier_get_fd(notifier), > @@ -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); >
On Wed, 28 Apr 2021 16:36:52 +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. > > And regarding those messages, let's change it to a warning instead of > an error, to better reflect their severity. The existing code in both > paths handles everything anyway. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > > Notes: > v1->v2: > - Downgrade unregister IRQ message from error to warning [CH] > > v1: https://lore.kernel.org/qemu-devel/20210427142511.2125733-1-farman@linux.ibm.com/ > > hw/vfio/ccw.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) Thanks, applied.
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 400bc07fe2..f64dd850db 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -470,7 +470,7 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0, VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) { - error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name); + warn_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name); } qemu_set_fd_handler(event_notifier_get_fd(notifier), @@ -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);
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. And regarding those messages, let's change it to a warning instead of an error, to better reflect their severity. The existing code in both paths handles everything anyway. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- Notes: v1->v2: - Downgrade unregister IRQ message from error to warning [CH] v1: https://lore.kernel.org/qemu-devel/20210427142511.2125733-1-farman@linux.ibm.com/ hw/vfio/ccw.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)