virtio: Handle possible failure of virtio_finalize_features()

Try to standardize how drivers negotiate feature and the
function names

Reviewed by: grehan (mentor)
Differential Revision: https://reviews.freebsd.org/D27930

7 files changed, 149 insertions, 65 deletions

diff --git a/sys/dev/virtio/balloon/virtio_balloon.c b/sys/dev/virtio/balloon/virtio_balloon.c
index 0973528887c5..848dd4e9a7f5 100644
--- a/sys/dev/virtio/balloon/virtio_balloon.c
+++ b/sys/dev/virtio/balloon/virtio_balloon.c
@@ -90,7 +90,8 @@ static int	vtballoon_attach(device_t);
 static int	vtballoon_detach(device_t);
 static int	vtballoon_config_change(device_t);
 
-static void	vtballoon_negotiate_features(struct vtballoon_softc *);
+static int	vtballoon_negotiate_features(struct vtballoon_softc *);
+static int	vtballoon_setup_features(struct vtballoon_softc *);
 static int	vtballoon_alloc_virtqueues(struct vtballoon_softc *);
 
 static void	vtballoon_vq_intr(void *);
@@ -110,7 +111,7 @@ static void	vtballoon_free_page(struct vtballoon_softc *, vm_page_t);
 
 static int	vtballoon_sleep(struct vtballoon_softc *);
 static void	vtballoon_thread(void *);
-static void	vtballoon_add_sysctl(struct vtballoon_softc *);
+static void	vtballoon_setup_sysctl(struct vtballoon_softc *);
 
 #define vtballoon_modern(_sc) \
     (((_sc)->vtballoon_features & VIRTIO_F_VERSION_1) != 0)
@@ -183,14 +184,18 @@ vtballoon_attach(device_t dev)
 
 	sc = device_get_softc(dev);
 	sc->vtballoon_dev = dev;
+	virtio_set_feature_desc(dev, vtballoon_feature_desc);
 
 	VTBALLOON_LOCK_INIT(sc, device_get_nameunit(dev));
 	TAILQ_INIT(&sc->vtballoon_pages);
 
-	vtballoon_add_sysctl(sc);
+	vtballoon_setup_sysctl(sc);
 
-	virtio_set_feature_desc(dev, vtballoon_feature_desc);
-	vtballoon_negotiate_features(sc);
+	error = vtballoon_setup_features(sc);
+	if (error) {
+		device_printf(dev, "cannot setup features\n");
+		goto fail;
+	}
 
 	sc->vtballoon_page_frames = malloc(VTBALLOON_PAGES_PER_REQUEST *
 	    sizeof(uint32_t), M_DEVBUF, M_NOWAIT | M_ZERO);
@@ -276,7 +281,7 @@ vtballoon_config_change(device_t dev)
 	return (1);
 }
 
-static void
+static int
 vtballoon_negotiate_features(struct vtballoon_softc *sc)
 {
 	device_t dev;
@@ -286,7 +291,19 @@ vtballoon_negotiate_features(struct vtballoon_softc *sc)
 	features = VTBALLOON_FEATURES;
 
 	sc->vtballoon_features = virtio_negotiate_features(dev, features);
-	virtio_finalize_features(dev);
+	return (virtio_finalize_features(dev));
+}
+
+static int
+vtballoon_setup_features(struct vtballoon_softc *sc)
+{
+	int error;
+
+	error = vtballoon_negotiate_features(sc);
+	if (error)
+		return (error);
+
+	return (0);
 }
 
 static int
@@ -559,7 +576,7 @@ vtballoon_thread(void *xsc)
 }
 
 static void
-vtballoon_add_sysctl(struct vtballoon_softc *sc)
+vtballoon_setup_sysctl(struct vtballoon_softc *sc)
 {
 	device_t dev;
 	struct sysctl_ctx_list *ctx;
diff --git a/sys/dev/virtio/block/virtio_blk.c b/sys/dev/virtio/block/virtio_blk.c
index 08df77d6de5b..6056771e3735 100644
--- a/sys/dev/virtio/block/virtio_blk.c
+++ b/sys/dev/virtio/block/virtio_blk.c
@@ -135,8 +135,8 @@ static int	vtblk_ioctl(struct disk *, u_long, void *, int,
 static int	vtblk_dump(void *, void *, vm_offset_t, off_t, size_t);
 static void	vtblk_strategy(struct bio *);
 
-static void	vtblk_negotiate_features(struct vtblk_softc *);
-static void	vtblk_setup_features(struct vtblk_softc *);
+static int	vtblk_negotiate_features(struct vtblk_softc *);
+static int	vtblk_setup_features(struct vtblk_softc *);
 static int	vtblk_maximum_segments(struct vtblk_softc *,
 		    struct virtio_blk_config *);
 static int	vtblk_alloc_virtqueue(struct vtblk_softc *);
@@ -312,10 +312,10 @@ vtblk_attach(device_t dev)
 	struct virtio_blk_config blkcfg;
 	int error;
 
-	virtio_set_feature_desc(dev, vtblk_feature_desc);
-
 	sc = device_get_softc(dev);
 	sc->vtblk_dev = dev;
+	virtio_set_feature_desc(dev, vtblk_feature_desc);
+
 	VTBLK_LOCK_INIT(sc, device_get_nameunit(dev));
 	bioq_init(&sc->vtblk_bioq);
 	TAILQ_INIT(&sc->vtblk_dump_queue);
@@ -323,7 +323,12 @@ vtblk_attach(device_t dev)
 	TAILQ_INIT(&sc->vtblk_req_ready);
 
 	vtblk_setup_sysctl(sc);
-	vtblk_setup_features(sc);
+
+	error = vtblk_setup_features(sc);
+	if (error) {
+		device_printf(dev, "cannot setup features\n");
+		goto fail;
+	}
 
 	vtblk_read_config(sc, &blkcfg);
 
@@ -572,7 +577,7 @@ vtblk_strategy(struct bio *bp)
 	VTBLK_UNLOCK(sc);
 }
 
-static void
+static int
 vtblk_negotiate_features(struct vtblk_softc *sc)
 {
 	device_t dev;
@@ -583,17 +588,20 @@ vtblk_negotiate_features(struct vtblk_softc *sc)
 	    VTBLK_LEGACY_FEATURES;
 
 	sc->vtblk_features = virtio_negotiate_features(dev, features);
-	virtio_finalize_features(dev);
+	return (virtio_finalize_features(dev));
 }
 
-static void
+static int
 vtblk_setup_features(struct vtblk_softc *sc)
 {
 	device_t dev;
+	int error;
 
 	dev = sc->vtblk_dev;
 
-	vtblk_negotiate_features(sc);
+	error = vtblk_negotiate_features(sc);
+	if (error)
+		return (error);
 
 	if (virtio_with_feature(dev, VIRTIO_RING_F_INDIRECT_DESC))
 		sc->vtblk_flags |= VTBLK_FLAG_INDIRECT;
@@ -603,6 +611,8 @@ vtblk_setup_features(struct vtblk_softc *sc)
 	/* Legacy. */
 	if (virtio_with_feature(dev, VIRTIO_BLK_F_BARRIER))
 		sc->vtblk_flags |= VTBLK_FLAG_BARRIER;
+
+	return (0);
 }
 
 static int
diff --git a/sys/dev/virtio/console/virtio_console.c b/sys/dev/virtio/console/virtio_console.c
index 0bd7c982e3f5..315eb59716b4 100644
--- a/sys/dev/virtio/console/virtio_console.c
+++ b/sys/dev/virtio/console/virtio_console.c
@@ -157,8 +157,8 @@ static int	 vtcon_attach(device_t);
 static int	 vtcon_detach(device_t);
 static int	 vtcon_config_change(device_t);
 
-static void	 vtcon_setup_features(struct vtcon_softc *);
-static void	 vtcon_negotiate_features(struct vtcon_softc *);
+static int	 vtcon_setup_features(struct vtcon_softc *);
+static int	 vtcon_negotiate_features(struct vtcon_softc *);
 static int	 vtcon_alloc_scports(struct vtcon_softc *);
 static int	 vtcon_alloc_virtqueues(struct vtcon_softc *);
 static void	 vtcon_read_config(struct vtcon_softc *,
@@ -331,12 +331,16 @@ vtcon_attach(device_t dev)
 
 	sc = device_get_softc(dev);
 	sc->vtcon_dev = dev;
+	virtio_set_feature_desc(dev, vtcon_feature_desc);
 
 	mtx_init(&sc->vtcon_mtx, "vtconmtx", NULL, MTX_DEF);
 	mtx_init(&sc->vtcon_ctrl_tx_mtx, "vtconctrlmtx", NULL, MTX_DEF);
 
-	virtio_set_feature_desc(dev, vtcon_feature_desc);
-	vtcon_setup_features(sc);
+	error = vtcon_setup_features(sc);
+	if (error) {
+		device_printf(dev, "cannot setup features\n");
+		goto fail;
+	}
 
 	vtcon_read_config(sc, &concfg);
 	vtcon_determine_max_ports(sc, &concfg);
@@ -428,7 +432,7 @@ vtcon_config_change(device_t dev)
 	return (0);
 }
 
-static void
+static int
 vtcon_negotiate_features(struct vtcon_softc *sc)
 {
 	device_t dev;
@@ -438,22 +442,27 @@ vtcon_negotiate_features(struct vtcon_softc *sc)
 	features = VTCON_FEATURES;
 
 	sc->vtcon_features = virtio_negotiate_features(dev, features);
-	virtio_finalize_features(dev);
+	return (virtio_finalize_features(dev));
 }
 
-static void
+static int
 vtcon_setup_features(struct vtcon_softc *sc)
 {
 	device_t dev;
+	int error;
 
 	dev = sc->vtcon_dev;
 
-	vtcon_negotiate_features(sc);
+	error = vtcon_negotiate_features(sc);
+	if (error)
+		return (error);
 
 	if (virtio_with_feature(dev, VIRTIO_CONSOLE_F_SIZE))
 		sc->vtcon_flags |= VTCON_FLAG_SIZE;
 	if (virtio_with_feature(dev, VIRTIO_CONSOLE_F_MULTIPORT))
 		sc->vtcon_flags |= VTCON_FLAG_MULTIPORT;
+
+	return (0);
 }
 
 #define VTCON_GET_CONFIG(_dev, _feature, _field, _cfg)			\
diff --git a/sys/dev/virtio/network/if_vtnet.c b/sys/dev/virtio/network/if_vtnet.c
index ed3065b61283..8d0770f5ac2d 100644
--- a/sys/dev/virtio/network/if_vtnet.c
+++ b/sys/dev/virtio/network/if_vtnet.c
@@ -102,8 +102,8 @@ static int	vtnet_shutdown(device_t);
 static int	vtnet_attach_completed(device_t);
 static int	vtnet_config_change(device_t);
 
-static void	vtnet_negotiate_features(struct vtnet_softc *);
-static void	vtnet_setup_features(struct vtnet_softc *);
+static int	vtnet_negotiate_features(struct vtnet_softc *);
+static int	vtnet_setup_features(struct vtnet_softc *);
 static int	vtnet_init_rxq(struct vtnet_softc *, int);
 static int	vtnet_init_txq(struct vtnet_softc *, int);
 static int	vtnet_alloc_rxtx_queues(struct vtnet_softc *);
@@ -434,7 +434,6 @@ vtnet_attach(device_t dev)
 
 	sc = device_get_softc(dev);
 	sc->vtnet_dev = dev;
-
 	virtio_set_feature_desc(dev, vtnet_feature_desc);
 
 	VTNET_CORE_LOCK_INIT(sc);
@@ -448,7 +447,12 @@ vtnet_attach(device_t dev)
 	}
 
 	vtnet_setup_sysctl(sc);
-	vtnet_setup_features(sc);
+
+	error = vtnet_setup_features(sc);
+	if (error) {
+		device_printf(dev, "cannot setup features\n");
+		goto fail;
+	}
 
 	error = vtnet_alloc_rx_filters(sc);
 	if (error) {
@@ -619,7 +623,7 @@ vtnet_config_change(device_t dev)
 	return (0);
 }
 
-static void
+static int
 vtnet_negotiate_features(struct vtnet_softc *sc)
 {
 	device_t dev;
@@ -705,17 +709,20 @@ vtnet_negotiate_features(struct vtnet_softc *sc)
 	sc->vtnet_features = negotiated_features;
 	sc->vtnet_negotiated_features = negotiated_features;
 
-	virtio_finalize_features(dev);
+	return (virtio_finalize_features(dev));
 }
 
-static void
+static int
 vtnet_setup_features(struct vtnet_softc *sc)
 {
 	device_t dev;
+	int error;
 
 	dev = sc->vtnet_dev;
 
-	vtnet_negotiate_features(sc);
+	error = vtnet_negotiate_features(sc);
+	if (error)
+		return (error);
 
 	if (virtio_with_feature(dev, VIRTIO_F_VERSION_1))
 		sc->vtnet_flags |= VTNET_FLAG_MODERN;
@@ -807,6 +814,8 @@ vtnet_setup_features(struct vtnet_softc *sc)
 			sc->vtnet_flags |= VTNET_FLAG_MQ;
 		}
 	}
+
+	return (0);
 }
 
 static int
diff --git a/sys/dev/virtio/pci/virtio_pci_modern.c b/sys/dev/virtio/pci/virtio_pci_modern.c
index 09ac0a1232e7..7029d2ff76ce 100644
--- a/sys/dev/virtio/pci/virtio_pci_modern.c
+++ b/sys/dev/virtio/pci/virtio_pci_modern.c
@@ -462,8 +462,6 @@ vtpci_modern_finalize_features(device_t dev)
 	/*
 	 * Must re-read the status after setting it to verify the negotiated
 	 * features were accepted by the device.
-	 *
-	 * BMV: TODO Drivers need to handle possible failure of this method!
 	 */
 	vtpci_modern_set_status(sc, VIRTIO_CONFIG_S_FEATURES_OK);
 
diff --git a/sys/dev/virtio/random/virtio_random.c b/sys/dev/virtio/random/virtio_random.c
index 8c01b1cf6ae3..ee3a24bb5513 100644
--- a/sys/dev/virtio/random/virtio_random.c
+++ b/sys/dev/virtio/random/virtio_random.c
@@ -36,7 +36,6 @@ __FBSDID("$FreeBSD$");
 #include <sys/malloc.h>
 #include <sys/module.h>
 #include <sys/sglist.h>
-#include <sys/callout.h>
 #include <sys/random.h>
 #include <sys/stdatomic.h>
 
@@ -50,6 +49,7 @@ __FBSDID("$FreeBSD$");
 #include <dev/virtio/virtqueue.h>
 
 struct vtrnd_softc {
+	device_t		 vtrnd_dev;
 	uint64_t		 vtrnd_features;
 	struct virtqueue	*vtrnd_vq;
 };
@@ -60,8 +60,9 @@ static int	vtrnd_probe(device_t);
 static int	vtrnd_attach(device_t);
 static int	vtrnd_detach(device_t);
 
-static void	vtrnd_negotiate_features(device_t);
-static int	vtrnd_alloc_virtqueue(device_t);
+static int	vtrnd_negotiate_features(struct vtrnd_softc *);
+static int	vtrnd_setup_features(struct vtrnd_softc *);
+static int	vtrnd_alloc_virtqueue(struct vtrnd_softc *);
 static int	vtrnd_harvest(struct vtrnd_softc *, void *, size_t *);
 static unsigned	vtrnd_read(void *, unsigned);
 
@@ -142,11 +143,16 @@ vtrnd_attach(device_t dev)
 	int error;
 
 	sc = device_get_softc(dev);
-
+	sc->vtrnd_dev = dev;
 	virtio_set_feature_desc(dev, vtrnd_feature_desc);
-	vtrnd_negotiate_features(dev);
 
-	error = vtrnd_alloc_virtqueue(dev);
+	error = vtrnd_setup_features(sc);
+	if (error) {
+		device_printf(dev, "cannot setup features\n");
+		goto fail;
+	}
+
+	error = vtrnd_alloc_virtqueue(sc);
 	if (error) {
 		device_printf(dev, "cannot allocate virtqueue\n");
 		goto fail;
@@ -182,23 +188,38 @@ vtrnd_detach(device_t dev)
 	return (0);
 }
 
-static void
-vtrnd_negotiate_features(device_t dev)
+static int
+vtrnd_negotiate_features(struct vtrnd_softc *sc)
 {
-	struct vtrnd_softc *sc;
+	device_t dev;
+	uint64_t features;
 
-	sc = device_get_softc(dev);
-	sc->vtrnd_features = virtio_negotiate_features(dev, VTRND_FEATURES);
-	virtio_finalize_features(dev);
+	dev = sc->vtrnd_dev;
+	features = VTRND_FEATURES;
+
+	sc->vtrnd_features = virtio_negotiate_features(dev, features);
+	return (virtio_finalize_features(dev));
 }
 
 static int
-vtrnd_alloc_virtqueue(device_t dev)
+vtrnd_setup_features(struct vtrnd_softc *sc)
 {
-	struct vtrnd_softc *sc;
+	int error;
+
+	error = vtrnd_negotiate_features(sc);
+	if (error)
+		return (error);
+
+	return (0);
+}
+
+static int
+vtrnd_alloc_virtqueue(struct vtrnd_softc *sc)
+{
+	device_t dev;
 	struct vq_alloc_info vq_info;
 
-	sc = device_get_softc(dev);
+	dev = sc->vtrnd_dev;
 
 	VQ_ALLOC_INFO_INIT(&vq_info, 0, NULL, sc, &sc->vtrnd_vq,
 	    "%s request", device_get_nameunit(dev));
diff --git a/sys/dev/virtio/scsi/virtio_scsi.c b/sys/dev/virtio/scsi/virtio_scsi.c
index f4c716af3725..737b6d0a7a42 100644
--- a/sys/dev/virtio/scsi/virtio_scsi.c
+++ b/sys/dev/virtio/scsi/virtio_scsi.c
@@ -76,7 +76,8 @@ static int	vtscsi_detach(device_t);
 static int	vtscsi_suspend(device_t);
 static int	vtscsi_resume(device_t);
 
-static void	vtscsi_negotiate_features(struct vtscsi_softc *);
+static int	vtscsi_negotiate_features(struct vtscsi_softc *);
+static int	vtscsi_setup_features(struct vtscsi_softc *);
 static void	vtscsi_read_config(struct vtscsi_softc *,
 		    struct virtio_scsi_config *);
 static int	vtscsi_maximum_segments(struct vtscsi_softc *, int);
@@ -184,7 +185,7 @@ static void	vtscsi_disable_vqs_intr(struct vtscsi_softc *);
 static void	vtscsi_enable_vqs_intr(struct vtscsi_softc *);
 
 static void	vtscsi_get_tunables(struct vtscsi_softc *);
-static void	vtscsi_add_sysctl(struct vtscsi_softc *);
+static void	vtscsi_setup_sysctl(struct vtscsi_softc *);
 
 static void	vtscsi_printf_req(struct vtscsi_request *, const char *,
 		    const char *, ...);
@@ -285,22 +286,19 @@ vtscsi_attach(device_t dev)
 
 	sc = device_get_softc(dev);
 	sc->vtscsi_dev = dev;
+	virtio_set_feature_desc(dev, vtscsi_feature_desc);
 
 	VTSCSI_LOCK_INIT(sc, device_get_nameunit(dev));
 	TAILQ_INIT(&sc->vtscsi_req_free);
 
 	vtscsi_get_tunables(sc);
-	vtscsi_add_sysctl(sc);
-
-	virtio_set_feature_desc(dev, vtscsi_feature_desc);
-	vtscsi_negotiate_features(sc);
+	vtscsi_setup_sysctl(sc);
 
-	if (virtio_with_feature(dev, VIRTIO_RING_F_INDIRECT_DESC))
-		sc->vtscsi_flags |= VTSCSI_FLAG_INDIRECT;
-	if (virtio_with_feature(dev, VIRTIO_SCSI_F_INOUT))
-		sc->vtscsi_flags |= VTSCSI_FLAG_BIDIRECTIONAL;
-	if (virtio_with_feature(dev, VIRTIO_SCSI_F_HOTPLUG))
-		sc->vtscsi_flags |= VTSCSI_FLAG_HOTPLUG;
+	error = vtscsi_setup_features(sc);
+	if (error) {
+		device_printf(dev, "cannot setup features\n");
+		goto fail;
+	}
 
 	vtscsi_read_config(sc, &scsicfg);
 
@@ -413,7 +411,7 @@ vtscsi_resume(device_t dev)
 	return (0);
 }
 
-static void
+static int
 vtscsi_negotiate_features(struct vtscsi_softc *sc)
 {
 	device_t dev;
@@ -423,7 +421,29 @@ vtscsi_negotiate_features(struct vtscsi_softc *sc)
 	features = VTSCSI_FEATURES;
 
 	sc->vtscsi_features = virtio_negotiate_features(dev, features);
-	virtio_finalize_features(dev);
+	return (virtio_finalize_features(dev));
+}
+
+static int
+vtscsi_setup_features(struct vtscsi_softc *sc)
+{
+	device_t dev;
+	int error;
+
+	dev = sc->vtscsi_dev;
+
+	error = vtscsi_negotiate_features(sc);
+	if (error)
+		return (error);
+
+	if (virtio_with_feature(dev, VIRTIO_RING_F_INDIRECT_DESC))
+		sc->vtscsi_flags |= VTSCSI_FLAG_INDIRECT;
+	if (virtio_with_feature(dev, VIRTIO_SCSI_F_INOUT))
+		sc->vtscsi_flags |= VTSCSI_FLAG_BIDIRECTIONAL;
+	if (virtio_with_feature(dev, VIRTIO_SCSI_F_HOTPLUG))
+		sc->vtscsi_flags |= VTSCSI_FLAG_HOTPLUG;
+
+	return (0);
 }
 
 #define VTSCSI_GET_CONFIG(_dev, _field, _cfg)			\
@@ -2287,7 +2307,7 @@ vtscsi_get_tunables(struct vtscsi_softc *sc)
 }
 
 static void
-vtscsi_add_sysctl(struct vtscsi_softc *sc)
+vtscsi_setup_sysctl(struct vtscsi_softc *sc)
 {
 	device_t dev;
 	struct vtscsi_statistics *stats;