Message ID | 20180507155130.21085-3-cohuck@redhat.com |
---|---|
State | New |
Headers | show |
Series | s390x: reset handling for ccw devices | expand |
On 07.05.2018 17:51, Cornelia Huck wrote: > Thomas reported that the subchannel for a 3270 device that ended up > in a broken state (status pending even though not enabled) did not > get out of that state even after a reboot (which involves a subsytem > reset). The reason for this is that the 3270 device did not define > a reset handler. > > Let's fix this by introducing a base reset handler (set up for all > ccw devices) that resets the subchannel and have virtio-ccw call > its virtio-specific reset procedure in addition to that. > > Reported-by: Thomas Huth <thuth@redhat.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > hw/s390x/ccw-device.c | 8 ++++++++ > hw/s390x/virtio-ccw.c | 9 ++++++--- > hw/s390x/virtio-ccw.h | 1 + > 3 files changed, 15 insertions(+), 3 deletions(-) Looks good! Reviewed-by: Thomas Huth <thuth@redhat.com> I also checked that the broken 3270 device is now operational after a reboot again with your two patches, so: Tested-by: Thomas Huth <thuth@redhat.com>
ACK. cc stable? On 05/07/2018 05:51 PM, Cornelia Huck wrote: > Thomas reported that the subchannel for a 3270 device that ended up > in a broken state (status pending even though not enabled) did not > get out of that state even after a reboot (which involves a subsytem > reset). The reason for this is that the 3270 device did not define > a reset handler. > > Let's fix this by introducing a base reset handler (set up for all > ccw devices) that resets the subchannel and have virtio-ccw call > its virtio-specific reset procedure in addition to that. > > Reported-by: Thomas Huth <thuth@redhat.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > hw/s390x/ccw-device.c | 8 ++++++++ > hw/s390x/virtio-ccw.c | 9 ++++++--- > hw/s390x/virtio-ccw.h | 1 + > 3 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c > index f9bfa154d6..7cd73df4aa 100644 > --- a/hw/s390x/ccw-device.c > +++ b/hw/s390x/ccw-device.c > @@ -40,6 +40,13 @@ static Property ccw_device_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static void ccw_device_reset(DeviceState *d) > +{ > + CcwDevice *ccw_dev = CCW_DEVICE(d); > + > + css_reset_sch(ccw_dev->sch); > +} > + > static void ccw_device_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -48,6 +55,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data) > k->realize = ccw_device_realize; > k->refill_ids = ccw_device_refill_ids; > dc->props = ccw_device_properties; > + dc->reset = ccw_device_reset; > } > > const VMStateDescription vmstate_ccw_dev = { > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 40a33302a7..22df33b509 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1058,10 +1058,12 @@ static void virtio_ccw_reset(DeviceState *d) > { > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > - CcwDevice *ccw_dev = CCW_DEVICE(d); > + VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev); > > virtio_ccw_reset_virtio(dev, vdev); > - css_reset_sch(ccw_dev->sch); > + if (vdc->parent_reset) { > + vdc->parent_reset(d); > + } > } > > static void virtio_ccw_vmstate_change(DeviceState *d, bool running) > @@ -1715,12 +1717,13 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > CCWDeviceClass *k = CCW_DEVICE_CLASS(dc); > + VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_CLASS(klass); > > k->unplug = virtio_ccw_busdev_unplug; > dc->realize = virtio_ccw_busdev_realize; > dc->unrealize = virtio_ccw_busdev_unrealize; > dc->bus_type = TYPE_VIRTUAL_CSS_BUS; > - dc->reset = virtio_ccw_reset; > + device_class_set_parent_reset(dc, virtio_ccw_reset, &vdc->parent_reset); > } > > static const TypeInfo virtio_ccw_device_info = { > diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h > index 2fc513001e..3453aa1f98 100644 > --- a/hw/s390x/virtio-ccw.h > +++ b/hw/s390x/virtio-ccw.h > @@ -77,6 +77,7 @@ typedef struct VirtIOCCWDeviceClass { > CCWDeviceClass parent_class; > void (*realize)(VirtioCcwDevice *dev, Error **errp); > void (*unrealize)(VirtioCcwDevice *dev, Error **errp); > + void (*parent_reset)(DeviceState *dev); > } VirtIOCCWDeviceClass; > > /* Performance improves when virtqueue kick processing is decoupled from the >
On Tue, 8 May 2018 09:17:08 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > ACK. > > cc stable? Probably. We'd also need patch 1 for that. > > > > On 05/07/2018 05:51 PM, Cornelia Huck wrote: > > Thomas reported that the subchannel for a 3270 device that ended up > > in a broken state (status pending even though not enabled) did not > > get out of that state even after a reboot (which involves a subsytem > > reset). The reason for this is that the 3270 device did not define > > a reset handler. > > > > Let's fix this by introducing a base reset handler (set up for all > > ccw devices) that resets the subchannel and have virtio-ccw call > > its virtio-specific reset procedure in addition to that. > > > > Reported-by: Thomas Huth <thuth@redhat.com> > > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > hw/s390x/ccw-device.c | 8 ++++++++ > > hw/s390x/virtio-ccw.c | 9 ++++++--- > > hw/s390x/virtio-ccw.h | 1 + > > 3 files changed, 15 insertions(+), 3 deletions(-)
On 05/07/2018 05:51 PM, Cornelia Huck wrote: > Thomas reported that the subchannel for a 3270 device that ended up > in a broken state (status pending even though not enabled) did not > get out of that state even after a reboot (which involves a subsytem > reset). The reason for this is that the 3270 device did not define > a reset handler. > > Let's fix this by introducing a base reset handler (set up for all > ccw devices) that resets the subchannel and have virtio-ccw call > its virtio-specific reset procedure in addition to that. > > Reported-by: Thomas Huth <thuth@redhat.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Cornelia Huck <cohuck@redhat.com> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> The 'all' from the subject is actually 'all except maybe vfio-ccw' AFAIU. [..]
On Tue, 8 May 2018 15:42:17 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On 05/07/2018 05:51 PM, Cornelia Huck wrote: > > Thomas reported that the subchannel for a 3270 device that ended up > > in a broken state (status pending even though not enabled) did not > > get out of that state even after a reboot (which involves a subsytem > > reset). The reason for this is that the 3270 device did not define > > a reset handler. > > > > Let's fix this by introducing a base reset handler (set up for all > > ccw devices) that resets the subchannel and have virtio-ccw call > > its virtio-specific reset procedure in addition to that. > > > > Reported-by: Thomas Huth <thuth@redhat.com> > > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > > The 'all' from the subject is actually 'all except maybe vfio-ccw' > AFAIU. Yeah, but as it overwrites the default handler, it's on its own :) Thanks for your review!
diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c index f9bfa154d6..7cd73df4aa 100644 --- a/hw/s390x/ccw-device.c +++ b/hw/s390x/ccw-device.c @@ -40,6 +40,13 @@ static Property ccw_device_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static void ccw_device_reset(DeviceState *d) +{ + CcwDevice *ccw_dev = CCW_DEVICE(d); + + css_reset_sch(ccw_dev->sch); +} + static void ccw_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -48,6 +55,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data) k->realize = ccw_device_realize; k->refill_ids = ccw_device_refill_ids; dc->props = ccw_device_properties; + dc->reset = ccw_device_reset; } const VMStateDescription vmstate_ccw_dev = { diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 40a33302a7..22df33b509 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1058,10 +1058,12 @@ static void virtio_ccw_reset(DeviceState *d) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); - CcwDevice *ccw_dev = CCW_DEVICE(d); + VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev); virtio_ccw_reset_virtio(dev, vdev); - css_reset_sch(ccw_dev->sch); + if (vdc->parent_reset) { + vdc->parent_reset(d); + } } static void virtio_ccw_vmstate_change(DeviceState *d, bool running) @@ -1715,12 +1717,13 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); CCWDeviceClass *k = CCW_DEVICE_CLASS(dc); + VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_CLASS(klass); k->unplug = virtio_ccw_busdev_unplug; dc->realize = virtio_ccw_busdev_realize; dc->unrealize = virtio_ccw_busdev_unrealize; dc->bus_type = TYPE_VIRTUAL_CSS_BUS; - dc->reset = virtio_ccw_reset; + device_class_set_parent_reset(dc, virtio_ccw_reset, &vdc->parent_reset); } static const TypeInfo virtio_ccw_device_info = { diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h index 2fc513001e..3453aa1f98 100644 --- a/hw/s390x/virtio-ccw.h +++ b/hw/s390x/virtio-ccw.h @@ -77,6 +77,7 @@ typedef struct VirtIOCCWDeviceClass { CCWDeviceClass parent_class; void (*realize)(VirtioCcwDevice *dev, Error **errp); void (*unrealize)(VirtioCcwDevice *dev, Error **errp); + void (*parent_reset)(DeviceState *dev); } VirtIOCCWDeviceClass; /* Performance improves when virtqueue kick processing is decoupled from the
Thomas reported that the subchannel for a 3270 device that ended up in a broken state (status pending even though not enabled) did not get out of that state even after a reboot (which involves a subsytem reset). The reason for this is that the 3270 device did not define a reset handler. Let's fix this by introducing a base reset handler (set up for all ccw devices) that resets the subchannel and have virtio-ccw call its virtio-specific reset procedure in addition to that. Reported-by: Thomas Huth <thuth@redhat.com> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- hw/s390x/ccw-device.c | 8 ++++++++ hw/s390x/virtio-ccw.c | 9 ++++++--- hw/s390x/virtio-ccw.h | 1 + 3 files changed, 15 insertions(+), 3 deletions(-)