[2/2] s390x/ccw: make sure all ccw devices are properly reset

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 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!