From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37665) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URjcW-0003qK-Oy for qemu-devel@nongnu.org; Mon, 15 Apr 2013 09:38:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1URjcR-0001Ao-PA for qemu-devel@nongnu.org; Mon, 15 Apr 2013 09:38:56 -0400 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:44106) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URjcR-0001AM-DT for qemu-devel@nongnu.org; Mon, 15 Apr 2013 09:38:51 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 15 Apr 2013 14:35:34 +0100 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 0CCDD17D8026 for ; Mon, 15 Apr 2013 14:39:39 +0100 (BST) Received: from d06av05.portsmouth.uk.ibm.com (d06av05.portsmouth.uk.ibm.com [9.149.37.229]) by b06cxnps4076.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3FDcZk050135166 for ; Mon, 15 Apr 2013 13:38:35 GMT Received: from d06av05.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av05.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3FDciLg001760 for ; Mon, 15 Apr 2013 07:38:44 -0600 Date: Mon, 15 Apr 2013 15:38:42 +0200 From: Cornelia Huck Message-ID: <20130415153842.320aac5a@gondolin> In-Reply-To: <1365974797-13217-6-git-send-email-fred.konrad@greensocs.com> References: <1365974797-13217-1-git-send-email-fred.konrad@greensocs.com> <1365974797-13217-6-git-send-email-fred.konrad@greensocs.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 5/8] virtio-ccw: cleanup. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: fred.konrad@greensocs.com Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, Alexander Graf , Richard Henderson On Sun, 14 Apr 2013 23:26:34 +0200 fred.konrad@greensocs.com wrote: > From: KONRAD Frederic > > This is a cleanup for virtio-ccw. > The init function is replaced by the device_plugged callback from > virtio-bus. Hm, so you replaced a callback that might return an error by another one that can't... > > Signed-off-by: KONRAD Frederic > --- > hw/s390x/virtio-ccw.c | 34 ++++++++++++++-------------------- > 1 file changed, 14 insertions(+), 20 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 5d62606..4857f97 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -392,8 +392,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > return ret; > } > > -static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > +/* This is called by virtio-bus just after the device is plugged. */ > +static void virtio_ccw_device_plugged(DeviceState *d) > { > + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > unsigned int cssid = 0; > unsigned int ssid = 0; > unsigned int schid; > @@ -401,7 +403,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > bool have_devno = false; > bool found = false; > SubchDev *sch; > - int ret; > int num; > DeviceState *parent = DEVICE(dev); > > @@ -410,7 +411,7 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > sch->driver_data = dev; > dev->sch = sch; > > - dev->vdev = vdev; > + dev->vdev = dev->bus.vdev; > dev->indicators = 0; > > /* Initialize subchannel structure. */ > @@ -425,19 +426,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > num = sscanf(dev->bus_id, "%x.%x.%04x", &cssid, &ssid, &devno); > if (num == 3) { > if ((cssid > MAX_CSSID) || (ssid > MAX_SSID)) { > - ret = -EINVAL; > error_report("Invalid cssid or ssid: cssid %x, ssid %x", > cssid, ssid); > goto out_err; > } > /* Enforce use of virtual cssid. */ > if (cssid != VIRTUAL_CSSID) { > - ret = -EINVAL; > error_report("cssid %x not valid for virtio devices", cssid); > goto out_err; > } > if (css_devno_used(cssid, ssid, devno)) { > - ret = -EEXIST; > error_report("Device %x.%x.%04x already exists", cssid, ssid, > devno); > goto out_err; > @@ -447,7 +445,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > sch->devno = devno; > have_devno = true; > } else { > - ret = -EINVAL; > error_report("Malformed devno parameter '%s'", dev->bus_id); > goto out_err; > } > @@ -464,7 +461,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > } > } > if (!found) { > - ret = -ENODEV; > error_report("No free subchannel found for %x.%x.%04x", cssid, ssid, > devno); > goto out_err; > @@ -488,7 +484,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > if (devno == MAX_SCHID) { > devno = 0; > } else if (devno == schid - 1) { > - ret = -ENODEV; > error_report("No free devno found"); > goto out_err; > } else { > @@ -506,7 +501,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > } > } > if (!found) { > - ret = -ENODEV; > error_report("Virtual channel subsystem is full!"); > goto out_err; > } > @@ -525,20 +519,19 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev) > sch->id.cu_type = VIRTIO_CCW_CU_TYPE; > sch->id.cu_model = dev->vdev->device_id; > > - virtio_bind_device(vdev, &virtio_ccw_bindings, DEVICE(dev)); > /* Only the first 32 feature bits are used. */ > - dev->host_features[0] = vdev->get_features(vdev, dev->host_features[0]); > + dev->host_features[0] = dev->vdev->get_features(dev->vdev, > + dev->host_features[0]); > dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; > dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE; > > css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, > parent->hotplugged, 1); > - return 0; > + return; > > out_err: > dev->sch = NULL; > g_free(sch); > - return ret; What happens here? We now have a VirtioCcwDevice without an associated subchannel device (i. e. no way to do any I/O). With the old code, we'd just have failed initializing the virtio device, but now we have a useless device laying around. > } > > static int virtio_ccw_exit(VirtioCcwDevice *dev)