From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55461) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URkRR-00040v-PX for qemu-devel@nongnu.org; Mon, 15 Apr 2013 10:31:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1URkRQ-0005xr-0X for qemu-devel@nongnu.org; Mon, 15 Apr 2013 10:31:33 -0400 Received: from greensocs.com ([87.106.252.221]:50408 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URkRP-0005uG-N9 for qemu-devel@nongnu.org; Mon, 15 Apr 2013 10:31:31 -0400 Message-ID: <516C0F3C.50902@greensocs.com> Date: Mon, 15 Apr 2013 16:31:24 +0200 From: =?ISO-8859-1?Q?KONRAD_Fr=E9d=E9ric?= MIME-Version: 1.0 References: <1365974797-13217-1-git-send-email-fred.konrad@greensocs.com> <1365974797-13217-6-git-send-email-fred.konrad@greensocs.com> <20130415153842.320aac5a@gondolin> In-Reply-To: <20130415153842.320aac5a@gondolin> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Cornelia Huck Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, "Alexander Graf (maintainer:S390)" , "Richard Henderson (maintainer:S390)" On 15/04/2013 15:38, Cornelia Huck wrote: > 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... Yes, I think this is not the right thing to do. Originally, virtio-device was able to be hot-plugged in virtio-bus, that's why I used this callback. So two solutions, * Leave the init function as this. * Modify the callback prototype (returning the error). And probably do the same with PCI and S390. What do you think is the best? Thanks, Fred > >> 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)