Skip site navigation (1)Skip section navigation (2) 
Date:      Sun, 20 Oct 2019 22:01:35 +0000 (UTC)
From:      Kyle Evans <kevans@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org
Subject:   svn commit: r353783 - in stable: 11/sys/kern 11/sys/sys 12/sys/kern 12/sys/sys
Message-ID:  <201910202201.x9KM1Zpi056866@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kevans
Date: Sun Oct 20 22:01:35 2019
New Revision: 353783
URL: https://svnweb.freebsd.org/changeset/base/353783

Log:
  MFC r353128-r353129: fully initialize cloned devices w/ make_dev_args
  
  r353128:
  kern_conf: fully initialize cloned devices with make_dev_args, too
  
  Attempting to initialize si_drv{1,2} with mda_si_drv{1,2} does not work if
  you are operating on cloned devices.
  
  clone_create must be called prior to the make_dev* family to create/return
  the device on the clonelist as needed. This device is later returned early
  in newdev(), prior to si_drv{0,1,2} initialization.
  
  This patch simply breaks out of the loop if we've found a device and
  finishes init.
  
  r353129:
  Remove the remnants of SI_CHEAPCLONE
  
  SI_CHEAPCLONE was introduced in r66067 for use with cloned bpfs. It was
  later also used in tty, tun, tap at points. The rough timeline for being
  removed in each of these is as follows:
  
  - r181690: bpf switched to use cdevpriv API by ed@
  - r181905: ed@ rewrote the TTY later to be mpsafe
  - r204464: kib@ removes it from tun/tap, declaring it unused
  
  I've not yet been able to dig up any other consumers in the intervening 9
  years. It is no longer set on any devices in the tree and leaves an
  interesting situation in make_dev_sv where we're ok with the device already
  being set SI_NAMED.

Modified:
  stable/11/sys/kern/kern_conf.c
  stable/11/sys/sys/conf.h
Directory Properties:
  stable/11/   (props changed)

Changes in other areas also in this revision:
Modified:
  stable/12/sys/kern/kern_conf.c
  stable/12/sys/sys/conf.h
Directory Properties:
  stable/12/   (props changed)

Modified: stable/11/sys/kern/kern_conf.c
==============================================================================
--- stable/11/sys/kern/kern_conf.c	Sun Oct 20 21:06:25 2019	(r353782)
+++ stable/11/sys/kern/kern_conf.c	Sun Oct 20 22:01:35 2019	(r353783)
@@ -164,12 +164,6 @@ dev_rel(struct cdev *dev)
 	dev->si_refcount--;
 	KASSERT(dev->si_refcount >= 0,
 	    ("dev_rel(%s) gave negative count", devtoname(dev)));
-#if 0
-	if (dev->si_usecount == 0 &&
-	    (dev->si_flags & SI_CHEAPCLONE) && (dev->si_flags & SI_NAMED))
-		;
-	else 
-#endif
 	if (dev->si_devsw == NULL && dev->si_refcount == 0) {
 		LIST_REMOVE(dev, si_list);
 		flag = 1;
@@ -573,20 +567,41 @@ newdev(struct make_dev_args *args, struct cdev *si)
 
 	mtx_assert(&devmtx, MA_OWNED);
 	csw = args->mda_devsw;
+	si2 = NULL;
 	if (csw->d_flags & D_NEEDMINOR) {
 		/* We may want to return an existing device */
 		LIST_FOREACH(si2, &csw->d_devs, si_list) {
 			if (dev2unit(si2) == args->mda_unit) {
 				dev_free_devlocked(si);
-				return (si2);
+				si = si2;
+				break;
 			}
 		}
+
+		/*
+		 * If we're returning an existing device, we should make sure
+		 * it isn't already initialized.  This would have been caught
+		 * in consumers anyways, but it's good to catch such a case
+		 * early.  We still need to complete initialization of the
+		 * device, and we'll use whatever make_dev_args were passed in
+		 * to do so.
+		 */
+		KASSERT(si2 == NULL || (si2->si_flags & SI_NAMED) == 0,
+		    ("make_dev() by driver %s on pre-existing device (min=%x, name=%s)",
+		    args->mda_devsw->d_name, dev2unit(si2), devtoname(si2)));
 	}
 	si->si_drv0 = args->mda_unit;
-	si->si_devsw = csw;
 	si->si_drv1 = args->mda_si_drv1;
 	si->si_drv2 = args->mda_si_drv2;
-	LIST_INSERT_HEAD(&csw->d_devs, si, si_list);
+	/* Only push to csw->d_devs if it's not a cloned device. */
+	if (si2 == NULL) {
+		si->si_devsw = csw;
+		LIST_INSERT_HEAD(&csw->d_devs, si, si_list);
+	} else {
+		KASSERT(si->si_devsw == csw,
+		    ("%s: inconsistent devsw between clone_create() and make_dev()",
+		    __func__));
+	}
 	return (si);
 }
 
@@ -796,17 +811,6 @@ make_dev_sv(struct make_dev_args *args1, struct cdev *
 		dev_refl(dev);
 	if ((args.mda_flags & MAKEDEV_ETERNAL) != 0)
 		dev->si_flags |= SI_ETERNAL;
-	if (dev->si_flags & SI_CHEAPCLONE &&
-	    dev->si_flags & SI_NAMED) {
-		/*
-		 * This is allowed as it removes races and generally
-		 * simplifies cloning devices.
-		 * XXX: still ??
-		 */
-		dev_unlock_and_free();
-		*dres = dev;
-		return (0);
-	}
 	KASSERT(!(dev->si_flags & SI_NAMED),
 	    ("make_dev() by driver %s on pre-existing device (min=%x, name=%s)",
 	    args.mda_devsw->d_name, dev2unit(dev), devtoname(dev)));
@@ -1541,7 +1545,6 @@ DB_SHOW_COMMAND(cdev, db_show_cdev)
 	SI_FLAG(SI_ETERNAL);
 	SI_FLAG(SI_ALIAS);
 	SI_FLAG(SI_NAMED);
-	SI_FLAG(SI_CHEAPCLONE);
 	SI_FLAG(SI_CHILD);
 	SI_FLAG(SI_DUMPDEV);
 	SI_FLAG(SI_CLONELIST);

Modified: stable/11/sys/sys/conf.h
==============================================================================
--- stable/11/sys/sys/conf.h	Sun Oct 20 21:06:25 2019	(r353782)
+++ stable/11/sys/sys/conf.h	Sun Oct 20 22:01:35 2019	(r353783)
@@ -57,7 +57,7 @@ struct cdev {
 #define	SI_ETERNAL	0x0001	/* never destroyed */
 #define	SI_ALIAS	0x0002	/* carrier of alias name */
 #define	SI_NAMED	0x0004	/* make_dev{_alias} has been called */
-#define	SI_CHEAPCLONE	0x0008	/* can be removed_dev'ed when vnode reclaims */
+#define	SI_UNUSED1	0x0008	/* unused */
 #define	SI_CHILD	0x0010	/* child of another struct cdev **/
 #define	SI_DUMPDEV	0x0080	/* is kernel dumpdev */
 #define	SI_CLONELIST	0x0200	/* on a clone list */

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201910202201.x9KM1Zpi056866>