Re: CVS commit: src/sys

[Paul Goyette <paul%whooppee.com@localhost>]

(Redirecting to tech-kern, and moving source-changes-d to bcc)

The attached diffs make a first pass at dealing with this mess.  It 
creates two new modules (ld common code, and ld_nvme attach specific), 
and updates the sets lists as appropriate.  It also modifies the nvme 
code to be able to handle "bus rescan" - attaching a modular driver 
requires rescan capability on the parent device.  And finally, the 
changes remove the ld code from the nvme module.

I'm not prepared to commit this yet, since I'm not in a position to do 
any serious testing (for one thing, I don't have a nvme device...)  I 
have successfully built a release with these changes.

There's still more work that eventually needs to be done.  First, the 
other attachments for ld should be modularized, and the parents should 
be taught how to do a rescan.  Secondly, the nvme module itself needs to 
be split into nvme-common and nvme-pci pieces.  But neither of these is 
urgent.

I'd very much appreciate it if someone with the appropriate hardware 
could do some testing.  Just build a kernel with neither ld nor nvme 
devices included, and build the modules.  Put everything in the right 
places, reboot, and try to 'modload ld_nvme'.  You should see other 
modules get autoloaded (turn on sysctl kern.module.verbose for more 
details), and hopefully the nvme device will get attached.  (But I'd be 
very surprised if this works on the first attempt!)




On Sat, 17 Sep 2016, Paul Goyette wrote:

> On Fri, 16 Sep 2016, Paul Goyette wrote:
>
> > > Log Message:
> > > make it possible to load nvme(4) as module to ease testing; currently 
> > > somewhat
> > > non-optimal, since it includes the ld(4) code also and hence requires the
> > > kernel config to have 'no ld'
> >
> > Wouldn't it make more sense to also enable ld(4) as its own module, and 
> > then have nvme "require" ld?
>
> Actually, it looks like we really should have four separate modules here...
>
> * ld        For the common ld code
> * ld_nvme   For the 'ld* at nvme?' attachment code, requires "ld,nvme"
> * nvme      For the common nvme code
> * nvme_pci  For the 'nvme* at pci?' attachment code, requires "nvme,pci"
>
>
>
>
> +------------------+--------------------------+------------------------+
> | Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:      |
> | (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com   |
> | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
> +------------------+--------------------------+------------------------+

+------------------+--------------------------+------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:      |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+------------------+--------------------------+------------------------+

--- /dev/null	2016-09-17 15:14:24.114633403 +0800
+++ sys/modules/ld/Makefile	2016-09-17 15:09:06.018064405 +0800
@@ -0,0 +1,11 @@
+#	$NetBSD$
+
+.include "../Makefile.inc"
+
+.PATH:	${S}/dev
+
+KMOD=	ld
+
+SRCS+=	ld.c
+
+.include <bsd.kmodule.mk>
--- /dev/null	2016-09-17 15:14:24.114633403 +0800
+++ sys/modules/ld_nvme/Makefile	2016-09-17 15:09:17.913260673 +0800
@@ -0,0 +1,12 @@
+#	$NetBSD$
+
+.include "../Makefile.inc"
+
+.PATH:	${S}/dev/ic
+
+KMOD=	ld_nvme
+IOCONF=	ld_nvme.ioconf
+
+SRCS+=	ld_nvme.c
+
+.include <bsd.kmodule.mk>
--- /dev/null	2016-09-17 15:14:24.114633403 +0800
+++ sys/modules/ld_nvme/ld_nvme.ioconf	2016-09-17 15:09:34.530402225 +0800
@@ -0,0 +1,10 @@
+#       $NetBSD$
+
+ioconf ld_nvme
+
+include "conf/files"
+include "dev/pci/files.pci"
+
+pseudo-root nvme*
+
+ld* at nvme? nsid ?
Index: distrib/sets/lists/modules/md.amd64
===================================================================
RCS file: /cvsroot/src/distrib/sets/lists/modules/md.amd64,v
retrieving revision 1.67
diff -u -p -r1.67 md.amd64
--- distrib/sets/lists/modules/md.amd64	17 Sep 2016 02:27:19 -0000	1.67
+++ distrib/sets/lists/modules/md.amd64	17 Sep 2016 07:14:44 -0000
@@ -111,6 +111,8 @@
 ./@MODULEDIR@/i915drmkms/i915drmkms.kmod	base-kernel-modules	kmod
 ./@MODULEDIR@/itesio				base-kernel-modules	kmod
 ./@MODULEDIR@/itesio/itesio.kmod		base-kernel-modules	kmod
+./@MODULEDIR@/ld_nvme				base-kernel-modules	kmod
+./@MODULEDIR@/ld_nvme/ld_nvme.kmod		base-kernel-modules	kmod
 ./@MODULEDIR@/lg3303				base-kernel-modules	kmod
 ./@MODULEDIR@/lg3303/lg3303.kmod		base-kernel-modules	kmod
 ./@MODULEDIR@/lm				base-kernel-modules	kmod
Index: distrib/sets/lists/modules/md.i386
===================================================================
RCS file: /cvsroot/src/distrib/sets/lists/modules/md.i386,v
retrieving revision 1.69
diff -u -p -r1.69 md.i386
--- distrib/sets/lists/modules/md.i386	17 Sep 2016 02:27:19 -0000	1.69
+++ distrib/sets/lists/modules/md.i386	17 Sep 2016 07:14:47 -0000
@@ -105,6 +105,8 @@
 ./@MODULEDIR@/i915drmkms/i915drmkms.kmod	base-kernel-modules	kmod
 ./@MODULEDIR@/itesio				base-kernel-modules	kmod
 ./@MODULEDIR@/itesio/itesio.kmod		base-kernel-modules	kmod
+./@MODULEDIR@/ld_nvme				base-kernel-modules	kmod
+./@MODULEDIR@/ld_nvme/ld_nvme.kmod		base-kernel-modules	kmod
 ./@MODULEDIR@/lg3303				base-kernel-modules	kmod
 ./@MODULEDIR@/lg3303/lg3303.kmod		base-kernel-modules	kmod
 ./@MODULEDIR@/lm				base-kernel-modules	kmod
Index: distrib/sets/lists/modules/mi
===================================================================
RCS file: /cvsroot/src/distrib/sets/lists/modules/mi,v
retrieving revision 1.96
diff -u -p -r1.96 mi
--- distrib/sets/lists/modules/mi	17 Sep 2016 02:27:19 -0000	1.96
+++ distrib/sets/lists/modules/mi	17 Sep 2016 07:14:50 -0000
@@ -168,6 +168,8 @@
 ./@MODULEDIR@/ksem/ksem.kmod			base-obsolete		obsolete
 ./@MODULEDIR@/layerfs				base-kernel-modules	kmod
 ./@MODULEDIR@/layerfs/layerfs.kmod		base-kernel-modules	kmod
+./@MODULEDIR@/ld				base-kernel-modules	kmod
+./@MODULEDIR@/ld/ld.kmod			base-kernel-modules	kmod
 ./@MODULEDIR@/lfs				base-kernel-modules	kmod
 ./@MODULEDIR@/lfs/lfs.kmod			base-kernel-modules	kmod
 ./@MODULEDIR@/lua				base-kernel-modules	kmod
Index: sys/dev/ld.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ld.c,v
retrieving revision 1.95
diff -u -p -r1.95 ld.c
--- sys/dev/ld.c	16 Sep 2016 15:20:50 -0000	1.95
+++ sys/dev/ld.c	17 Sep 2016 07:14:54 -0000
@@ -54,11 +54,10 @@ __KERNEL_RCSID(0, "$NetBSD: ld.c,v 1.95 
 #include <sys/vnode.h>
 #include <sys/syslog.h>
 #include <sys/mutex.h>
+#include <sys/module.h>
 
 #include <dev/ldvar.h>
 
-#include <prop/proplib.h>
-
 static void	ldminphys(struct buf *bp);
 static bool	ld_suspend(device_t, const pmf_qual_t *);
 static bool	ld_shutdown(device_t, int);
@@ -593,3 +592,44 @@ lddiscard(dev_t dev, off_t pos, off_t le
 
 	return dk_discard(dksc, dev, pos, len);
 }
+
+MODULE(MODULE_CLASS_DRIVER, ld, "dk_subr");
+
+#ifdef _MODULE
+
+CFDRIVER_DECL(ld, DV_DISK, NULL);
+
+#endif
+
+static int
+ld_modcmd(modcmd_t cmd, void *opaque)
+{
+#ifdef _MODULE
+	devmajor_t bmajor, cmajor;
+#endif
+	int error = 0;
+
+#ifdef _MODULE
+	switch (cmd) {
+	case MODULE_CMD_INIT:
+		bmajor = cmajor = -1;
+		error = devsw_attach(ld_cd.cd_name, &ld_bdevsw, &bmajor,
+		    &ld_cdevsw, &cmajor);
+		if (error)
+			break;
+		error = config_cfdriver_attach(&ld_cd);
+		break;
+	case MODULE_CMD_FINI:
+		error = config_cfdriver_detach(&ld_cd);
+		if (error)
+			break;
+		devsw_detach(&ld_bdevsw, &ld_cdevsw);
+		break;
+	default:
+		error = ENOTTY;
+		break;
+	}
+#endif
+
+	return error;
+}
Index: sys/dev/ic/ld_nvme.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/ld_nvme.c,v
retrieving revision 1.3
diff -u -p -r1.3 ld_nvme.c
--- sys/dev/ic/ld_nvme.c	16 Sep 2016 15:24:47 -0000	1.3
+++ sys/dev/ic/ld_nvme.c	17 Sep 2016 07:14:57 -0000
@@ -36,11 +36,14 @@ __KERNEL_RCSID(0, "$NetBSD: ld_nvme.c,v 
 #include <sys/bufq.h>
 #include <sys/disk.h>
 #include <sys/kmem.h>
+#include <sys/module.h>
 
 #include <dev/ldvar.h>
 #include <dev/ic/nvmereg.h>
 #include <dev/ic/nvmevar.h>
 
+#include "ioconf.h"
+
 struct ld_nvme_softc {
 	struct ld_softc		sc_ld;
 	struct nvme_softc	*sc_nvme;
@@ -235,3 +238,38 @@ ld_nvme_syncdone(struct nvme_ns_context 
 
 	nvme_ns_put_ctx(ctx);
 }
+
+MODULE(MODULE_CLASS_DRIVER, ld_nvme, "ld,nvme");
+
+#ifdef _MODULE
+#include "ioconf.c"
+#endif
+
+static int
+ld_nvme_modcmd(modcmd_t cmd, void *opaque)
+{
+	int error = 0;
+
+#ifdef _MODULE
+	switch (cmd) {
+	case MODULE_CMD_INIT:
+		/*
+		 * We skip over the first entry in cfdriver[] array
+		 * since the cfdriver is attached by the common
+		 * (non-attachment-specific) code.
+		 */
+		error = config_init_component(&cfdriver_ioconf_ld_nvme[1],
+		    cfattach_ioconf_ld_nvme, cfdata_ioconf_ld_nvme);
+		break;
+	case MODULE_CMD_FINI:
+		error = config_fini_component(&cfdriver_ioconf_ld_nvme[1],
+		    cfattach_ioconf_ld_nvme, cfdata_ioconf_ld_nvme);
+		break;
+	default:
+		error = ENOTTY;
+		break;
+	}
+#endif
+
+	return error;
+}
Index: sys/dev/ic/nvme.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/nvme.c,v
retrieving revision 1.7
diff -u -p -r1.7 nvme.c
--- sys/dev/ic/nvme.c	16 Sep 2016 12:57:26 -0000	1.7
+++ sys/dev/ic/nvme.c	17 Sep 2016 07:15:01 -0000
@@ -310,7 +310,6 @@ nvme_disable(struct nvme_softc *sc)
 int
 nvme_attach(struct nvme_softc *sc)
 {
-	struct nvme_attach_args naa;
 	uint64_t cap;
 	uint32_t reg;
 	u_int dstrd;
@@ -401,14 +400,7 @@ nvme_attach(struct nvme_softc *sc)
 
 	sc->sc_namespaces = kmem_zalloc(sizeof(*sc->sc_namespaces) * sc->sc_nn,
 	    KM_SLEEP);
-	for (i = 0; i < sc->sc_nn; i++) {
-		memset(&naa, 0, sizeof(naa));
-		naa.naa_nsid = i + 1;
-		naa.naa_qentries = ioq_entries;
-		sc->sc_namespaces[i].dev = config_found(sc->sc_dev, &naa,
-		    nvme_print);
-	}
-
+	nvme_rescan(sc->sc_dev, "nvme", &i);
 	return 0;
 
 free_q:
@@ -426,6 +418,25 @@ free_admin_q:
 	return 1;
 }
 
+int
+nvme_rescan(device_t self, const char *attr, const int *flags)
+{
+	int i;
+	struct nvme_softc *sc = device_private(self);
+	struct nvme_attach_args naa;
+
+	for (i = 0; i < sc->sc_nn; i++) {
+		if (sc->sc_namespaces[i].dev)
+			continue;
+		memset(&naa, 0, sizeof(naa));
+		naa.naa_nsid = i + 1;
+		naa.naa_qentries = nvme_ioq_size;
+		sc->sc_namespaces[i].dev = config_found(sc->sc_dev, &naa,
+		    nvme_print);
+	}
+	return 0;
+}
+
 static int
 nvme_print(void *aux, const char *pnp)
 {
Index: sys/dev/ic/nvmevar.h
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/nvmevar.h,v
retrieving revision 1.2
diff -u -p -r1.2 nvmevar.h
--- sys/dev/ic/nvmevar.h	4 Jun 2016 16:11:51 -0000	1.2
+++ sys/dev/ic/nvmevar.h	17 Sep 2016 07:15:04 -0000
@@ -132,6 +132,7 @@ struct nvme_attach_args {
 
 int	nvme_attach(struct nvme_softc *);
 int	nvme_detach(struct nvme_softc *, int flags);
+int	nvme_rescan(device_t, const char *, const int *);
 void	nvme_childdet(device_t, device_t);
 int	nvme_intr(void *);
 int	nvme_mq_msi_intr(void *);
Index: sys/dev/pci/nvme_pci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/nvme_pci.c,v
retrieving revision 1.8
diff -u -p -r1.8 nvme_pci.c
--- sys/dev/pci/nvme_pci.c	17 Sep 2016 03:02:03 -0000	1.8
+++ sys/dev/pci/nvme_pci.c	17 Sep 2016 07:15:07 -0000
@@ -80,9 +80,10 @@ struct nvme_pci_softc {
 static int	nvme_pci_match(device_t, cfdata_t, void *);
 static void	nvme_pci_attach(device_t, device_t, void *);
 static int	nvme_pci_detach(device_t, int);
+static int	nvme_pci_rescan(device_t, const char *, const int *);
 
 CFATTACH_DECL3_NEW(nvme_pci, sizeof(struct nvme_pci_softc),
-    nvme_pci_match, nvme_pci_attach, nvme_pci_detach, NULL, NULL,
+    nvme_pci_match, nvme_pci_attach, nvme_pci_detach, NULL, nvme_pci_rescan,
     nvme_childdet, DVF_DETACH_SHUTDOWN);
 
 static int	nvme_pci_intr_establish(struct nvme_softc *,
@@ -196,6 +197,13 @@ unmap:
 }
 
 static int
+nvme_pci_rescan(device_t self, const char *attr, const int *flags)
+{
+
+	return nvme_rescan(self, attr, flags);
+}
+
+static int
 nvme_pci_detach(device_t self, int flags)
 {
 	struct nvme_pci_softc *psc = device_private(self);
@@ -421,52 +429,26 @@ MODULE(MODULE_CLASS_DRIVER, nvme, "pci,d
 
 #ifdef _MODULE
 #include "ioconf.c"
-
-extern const struct bdevsw ld_bdevsw;
-extern const struct cdevsw ld_cdevsw;
 #endif
 
 static int
 nvme_modcmd(modcmd_t cmd, void *opaque)
 {
-#ifdef _MODULE
-	devmajor_t cmajor, bmajor;
-#endif
 	int error = 0;
 
+#ifdef _MODULE
 	switch (cmd) {
 	case MODULE_CMD_INIT:
-#ifdef _MODULE
-		/* devsw must be done before configuring the pci device,
-		 * otherwise ldattach() fails
-		 */
-		bmajor = cmajor = NODEVMAJOR;
-		error = devsw_attach(ld_cd.cd_name, &ld_bdevsw, &bmajor,
-		    &ld_cdevsw, &cmajor);
-		if (error && error != EEXIST) {
-			aprint_error("%s: unable to register devsw\n",
-			    ld_cd.cd_name);
-			return error;
-		}
-
 		error = config_init_component(cfdriver_ioconf_nvme_pci,
 		    cfattach_ioconf_nvme_pci, cfdata_ioconf_nvme_pci);
-		if (error)
-			return error;
-
-#endif
-		return error;
+		break;
 	case MODULE_CMD_FINI:
-#ifdef _MODULE
 		error = config_fini_component(cfdriver_ioconf_nvme_pci,
 		    cfattach_ioconf_nvme_pci, cfdata_ioconf_nvme_pci);
-		if (error)
-			return error;
-
-		/* devsw not detached, it's static data and fine to stay */
-#endif
-		return error;
+		break;
 	default:
-		return ENOTTY;
+		break;
 	}
+#endif
+	return error;
 }
Index: sys/modules/Makefile
===================================================================
RCS file: /cvsroot/src/sys/modules/Makefile,v
retrieving revision 1.177
diff -u -p -r1.177 Makefile
--- sys/modules/Makefile	16 Sep 2016 11:35:07 -0000	1.177
+++ sys/modules/Makefile	17 Sep 2016 07:15:11 -0000
@@ -67,6 +67,7 @@ SUBDIR+=	if_vlan
 SUBDIR+=	ipl
 SUBDIR+=	kernfs
 SUBDIR+=	layerfs
+SUBDIR+=	ld
 SUBDIR+=	lfs
 SUBDIR+=	lua
 SUBDIR+=	luasystm
@@ -182,6 +183,7 @@ SUBDIR+=	vmt
 .if ${MACHINE_ARCH} == "i386" || \
     ${MACHINE_ARCH} == "x86_64"
 SUBDIR+=	ubsec		# Builds on architectures with PCI bus
+SUBDIR+=	ld_nvme
 SUBDIR+=	nvme
 .endif
 
Index: sys/modules/nvme/Makefile
===================================================================
RCS file: /cvsroot/src/sys/modules/nvme/Makefile,v
retrieving revision 1.1
diff -u -p -r1.1 Makefile
--- sys/modules/nvme/Makefile	16 Sep 2016 11:35:07 -0000	1.1
+++ sys/modules/nvme/Makefile	17 Sep 2016 07:15:15 -0000
@@ -2,13 +2,10 @@
 
 .include "../Makefile.inc"
 
-.PATH:	${S}/dev/pci ${S}/dev/ic ${S}/dev
+.PATH:	${S}/dev/pci ${S}/dev/ic
 
 KMOD=	nvme
 IOCONF=	nvme.ioconf
 SRCS=	nvme.c nvme_pci.c
 
-# move to separate module?
-SRCS+=	ld_nvme.c ld.c
-
 .include <bsd.kmodule.mk>
Index: sys/modules/nvme/nvme.ioconf
===================================================================
RCS file: /cvsroot/src/sys/modules/nvme/nvme.ioconf,v
retrieving revision 1.1
diff -u -p -r1.1 nvme.ioconf
--- sys/modules/nvme/nvme.ioconf	16 Sep 2016 11:35:07 -0000	1.1
+++ sys/modules/nvme/nvme.ioconf	17 Sep 2016 07:15:18 -0000
@@ -8,4 +8,3 @@ include "dev/pci/files.pci"
 pseudo-root pci*
 
 nvme* at pci? dev ? function ?
-ld* at nvme? nsid ?