From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36281) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URkzE-0007vC-42 for qemu-devel@nongnu.org; Mon, 15 Apr 2013 11:06:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1URkz9-0002Aq-Nr for qemu-devel@nongnu.org; Mon, 15 Apr 2013 11:06:28 -0400 Received: from e06smtp11.uk.ibm.com ([195.75.94.107]:36082) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URkz9-00028j-Ba for qemu-devel@nongnu.org; Mon, 15 Apr 2013 11:06:23 -0400 Received: from /spool/local by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 15 Apr 2013 16:02:52 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 58A8117D8019 for ; Mon, 15 Apr 2013 16:07:13 +0100 (BST) Received: from d06av10.portsmouth.uk.ibm.com (d06av10.portsmouth.uk.ibm.com [9.149.37.251]) by b06cxnps3075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3FF693Z54788210 for ; Mon, 15 Apr 2013 15:06:09 GMT Received: from d06av10.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av10.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3FDf456010802 for ; Mon, 15 Apr 2013 09:41:04 -0400 Date: Mon, 15 Apr 2013 17:06:16 +0200 From: Cornelia Huck Message-ID: <20130415170616.6de98375@gondolin> In-Reply-To: <516C0F3C.50902@greensocs.com> References: <1365974797-13217-1-git-send-email-fred.konrad@greensocs.com> <1365974797-13217-6-git-send-email-fred.konrad@greensocs.com> <20130415153842.320aac5a@gondolin> <516C0F3C.50902@greensocs.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 5/8] virtio-ccw: cleanup. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: KONRAD =?UTF-8?B?RnLDqWTDqXJpYw==?= 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 Mon, 15 Apr 2013 16:31:24 +0200 KONRAD Fr=C3=A9d=C3=A9ric wrote: > 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. >=20 > Originally, virtio-device was able to be hot-plugged in virtio-bus,=20 > that's why I > used this callback. >=20 > So two solutions, > * Leave the init function as this. > * Modify the callback prototype (returning the error). >=20 > And probably do the same with PCI and S390. >=20 > What do you think is the best? For virtio-ccw, I want to be able to fail initialization if the user specified an invalid devno or if the channel subsystem is full. Moreover, the exit function removes the subchannel from the configuration, and I'd like that to be symmetrical to the init function adding the subchannel. So I think I'd prefer keeping the init function. It might be a good idea to move generating the crws to the plugging function, though. >=20 > Thanks, > Fred >=20 > > > >> 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 =3D VIRTIO_CCW_DEVICE(d); > >> unsigned int cssid =3D 0; > >> unsigned int ssid =3D 0; > >> unsigned int schid; > >> @@ -401,7 +403,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice = *dev, VirtIODevice *vdev) > >> bool have_devno =3D false; > >> bool found =3D false; > >> SubchDev *sch; > >> - int ret; > >> int num; > >> DeviceState *parent =3D DEVICE(dev); > >> > >> @@ -410,7 +411,7 @@ static int virtio_ccw_device_init(VirtioCcwDevice = *dev, VirtIODevice *vdev) > >> sch->driver_data =3D dev; > >> dev->sch =3D sch; > >> > >> - dev->vdev =3D vdev; > >> + dev->vdev =3D dev->bus.vdev; > >> dev->indicators =3D 0; > >> > >> /* Initialize subchannel structure. */ > >> @@ -425,19 +426,16 @@ static int virtio_ccw_device_init(VirtioCcwDevic= e *dev, VirtIODevice *vdev) > >> num =3D sscanf(dev->bus_id, "%x.%x.%04x", &cssid, &ssid, &de= vno); > >> if (num =3D=3D 3) { > >> if ((cssid > MAX_CSSID) || (ssid > MAX_SSID)) { > >> - ret =3D -EINVAL; > >> error_report("Invalid cssid or ssid: cssid %x, ssid = %x", > >> cssid, ssid); > >> goto out_err; > >> } > >> /* Enforce use of virtual cssid. */ > >> if (cssid !=3D VIRTUAL_CSSID) { > >> - ret =3D -EINVAL; > >> error_report("cssid %x not valid for virtio devices"= , cssid); > >> goto out_err; > >> } > >> if (css_devno_used(cssid, ssid, devno)) { > >> - ret =3D -EEXIST; > >> error_report("Device %x.%x.%04x already exists", css= id, ssid, > >> devno); > >> goto out_err; > >> @@ -447,7 +445,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice = *dev, VirtIODevice *vdev) > >> sch->devno =3D devno; > >> have_devno =3D true; > >> } else { > >> - ret =3D -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 =3D -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 =3D=3D MAX_SCHID) { > >> devno =3D 0; > >> } else if (devno =3D=3D schid - 1) { > >> - ret =3D -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 =3D -ENODEV; > >> error_report("Virtual channel subsystem is full!"); > >> goto out_err; > >> } > >> @@ -525,20 +519,19 @@ static int virtio_ccw_device_init(VirtioCcwDevic= e *dev, VirtIODevice *vdev) > >> sch->id.cu_type =3D VIRTIO_CCW_CU_TYPE; > >> sch->id.cu_model =3D 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] =3D vdev->get_features(vdev, dev->host_feat= ures[0]); > >> + dev->host_features[0] =3D dev->vdev->get_features(dev->vdev, > >> + dev->host_feature= s[0]); > >> dev->host_features[0] |=3D 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; > >> dev->host_features[0] |=3D 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 =3D 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) >=20