On Mon, Apr 09, 2018 at 11:11:22AM -0600, Theo de Raadt wrote: > I think this approach is wrong, insane, and fragile. DVF_ACTIVE > doesn't work precisely that way.
Yes, it's a hack, but i don't see it as fragile, nor insane, and i agree something better is great, but it does work exactly as i wanted: # gpioctl gpio2 attach gpioow 2 1 gpioow0 at gpio2: DATA[2] onewire0 at gpioow0 # gpioctl gpio2 attach gpioow 4 5 gpioow1 at gpio2: invalid pin mask gpioow1 detached # gpioctl gpio2 attach gpioow 4 1 gpioow1 at gpio2: DATA[4] onewire1 at gpioow1 # gpioctl gpio2 attach gpioow 3 2 gpioow2 at gpio2: can't map pins gpioow2 detached # gpioctl gpio2 attach gpioow 3 1 gpioow2 at gpio2: DATA[3] onewire2 at gpioow2 # gpioctl gpio2 attach gpioow 2 1 gpioow3 at gpio2: can't map pins gpioow3 detached ^ is with the DVF_ACTIVE-hack, which does work because at that point it is 'local' to the config_attach(), and if it does fail it(dv_flags) doesn't exist to cause issues later, as nothing changes for those who don't call config_defer_failure(), they will get their DVF_ACTIVE just like before. my v2.diff below, in case you want to reconsider something like this while waiting for the whole-tree work to appear:) -Artturi diff --git share/man/man9/config_defer.9 share/man/man9/config_defer.9 index ef45867188b..280501a538a 100644 --- share/man/man9/config_defer.9 +++ share/man/man9/config_defer.9 @@ -33,6 +33,7 @@ .Os .Sh NAME .Nm config_defer , +.Nm config_defer_failure , .Nm config_mountroot .Nd deferred device configuration .Sh SYNOPSIS @@ -41,6 +42,8 @@ .Ft "void" .Fn config_defer "struct device *dev" "void (*func)(struct device *)" .Ft "void" +.Fn config_defer_failure "struct device *dev" +.Ft "void" .Fn config_mountroot "struct device *dev" "void (*func)(struct device *)" .Sh DESCRIPTION The @@ -53,6 +56,12 @@ is called with the argument .Fa dev . .Pp The +.Fn config_defer_failure +function is called by the device to mark its configuration as having +failed, so it can be detached by +.Fn config_attach . +.Pp +The .Fn config_mountroot function is called by a device driver to defer the remainder of its configuration until after the root file system is mounted. @@ -60,3 +69,5 @@ At this point, the function .Fa func is called with the argument .Fa dev . +.Sh SEE ALSO +.Xr config_attach 9 diff --git sys/dev/gpio/gpioow.c sys/dev/gpio/gpioow.c index 64d79ab0cb8..912537b7de7 100644 --- sys/dev/gpio/gpioow.c +++ sys/dev/gpio/gpioow.c @@ -101,7 +101,7 @@ gpioow_attach(struct device *parent, struct device *self, void *aux) /* Check that we have enough pins */ if (gpio_npins(ga->ga_mask) != GPIOOW_NPINS) { printf(": invalid pin mask\n"); - return; + goto fail_nomap; } /* Map pins */ @@ -110,7 +110,7 @@ gpioow_attach(struct device *parent, struct device *self, void *aux) if (gpio_pin_map(sc->sc_gpio, ga->ga_offset, ga->ga_mask, &sc->sc_map)) { printf(": can't map pins\n"); - return; + goto fail_nomap; } /* Configure data pin */ @@ -153,6 +153,8 @@ gpioow_attach(struct device *parent, struct device *self, void *aux) fail: gpio_pin_unmap(sc->sc_gpio, &sc->sc_map); +fail_nomap: + config_defer_failure(self); } int diff --git sys/kern/subr_autoconf.c sys/kern/subr_autoconf.c index 3fff153e8ad..e4ff9ee7536 100644 --- sys/kern/subr_autoconf.c +++ sys/kern/subr_autoconf.c @@ -409,6 +409,12 @@ config_attach(struct device *parent, void *match, void *aux, cfprint_t print) if (--autoconf_attdet == 0) wakeup(&autoconf_attdet); mtx_leave(&autoconf_attdet_mtx); + + /* Process deferred failure here by detaching. */ + if (dev->dv_flags & DVF_FAILED) { + if (config_detach(dev, 0) == 0) + dev = NULL; + } return (dev); } @@ -678,6 +684,18 @@ config_defer(struct device *dev, void (*func)(struct device *)) config_pending_incr(); } +/* + * Defer the detachment of the specified device with failure + * until the end of its own configuration, see config_attach(). + * XXX "void2int ca_attach()"-churn workaround + */ +void +config_defer_failure(struct device *dev) +{ + + dev->dv_flags |= DVF_FAILED; +} + /* * Defer the configuration of the specified device until after * root file system is mounted. diff --git sys/sys/device.h sys/sys/device.h index 00a1f6ad2a6..c1d133594ff 100644 --- sys/sys/device.h +++ sys/sys/device.h @@ -82,6 +82,7 @@ struct device { /* dv_flags */ #define DVF_ACTIVE 0x0001 /* device is activated */ +#define DVF_FAILED 0x0002 /* device failed to attach */ TAILQ_HEAD(devicelist, device); @@ -185,6 +186,7 @@ int config_activate_children(struct device *, int); struct device *config_make_softc(struct device *parent, struct cfdata *cf); void config_defer(struct device *, void (*)(struct device *)); +void config_defer_failure(struct device *); void config_pending_incr(void); void config_pending_decr(void); void config_mountroot(struct device *, void (*)(struct device *)); > > A better approach would be a whole-tree change such that attach > functions are non-void. This has been proposed a few times, but > noone ever sat down and did the whole-tree work. > > > could we allow config_attach() to 'fail' without leaving dead device behind? > > ie. with gpioow(4) it's easy to make a load of these while testing stuff, > > which does lead to inconsistent unit numbering unless you manually detach > > the broken ones before attaching another.. > > > > for devices which are found during autoconf, i guess DETACH_QUIET would > > be wanted to keep dmesg pretty, as the error messages printed from any > > ca->ca_attach() should be enough, and would mean no visible change for > > drivers failing out with this:) > > > > for smaller diff i chose to use the DVF_ACTIVE, while i'd actually prefer > > adding "DVF_FAILED" or something. untested morning-coffee-qual.diff below. > > > > comments? > > > > -Artturi > > > > > > diff --git sys/dev/gpio/gpioow.c sys/dev/gpio/gpioow.c > > index 64d79ab0cb8..912537b7de7 100644 > > --- sys/dev/gpio/gpioow.c > > +++ sys/dev/gpio/gpioow.c > > @@ -101,7 +101,7 @@ gpioow_attach(struct device *parent, struct device > > *self, void *aux) > > /* Check that we have enough pins */ > > if (gpio_npins(ga->ga_mask) != GPIOOW_NPINS) { > > printf(": invalid pin mask\n"); > > - return; > > + goto fail_nomap; > > } > > > > /* Map pins */ > > @@ -110,7 +110,7 @@ gpioow_attach(struct device *parent, struct device > > *self, void *aux) > > if (gpio_pin_map(sc->sc_gpio, ga->ga_offset, ga->ga_mask, > > &sc->sc_map)) { > > printf(": can't map pins\n"); > > - return; > > + goto fail_nomap; > > } > > > > /* Configure data pin */ > > @@ -153,6 +153,8 @@ gpioow_attach(struct device *parent, struct device > > *self, void *aux) > > > > fail: > > gpio_pin_unmap(sc->sc_gpio, &sc->sc_map); > > +fail_nomap: > > + config_defer_failure(self); > > } > > > > int > > diff --git sys/kern/subr_autoconf.c sys/kern/subr_autoconf.c > > index 3fff153e8ad..9a797712510 100644 > > --- sys/kern/subr_autoconf.c > > +++ sys/kern/subr_autoconf.c > > @@ -409,6 +409,11 @@ config_attach(struct device *parent, void *match, void > > *aux, cfprint_t print) > > if (--autoconf_attdet == 0) > > wakeup(&autoconf_attdet); > > mtx_leave(&autoconf_attdet_mtx); > > + > > + if ((dev->dv_flags & DVF_ACTIVE) == 0) { > > + if (config_detach(dev, 0) == 0) > > + dev = NULL; > > + } > > return (dev); > > } > > > > @@ -678,6 +683,17 @@ config_defer(struct device *dev, void (*func)(struct > > device *)) > > config_pending_incr(); > > } > > > > +/* > > + * Defer the detachment of the specified device until the end > > + * of its own configuration. > > + */ > > +void > > +config_defer_failure(struct device *dev) > > +{ > > + > > + dev->dv_flags &= ~DVF_ACTIVE; > > +} > > + > > /* > > * Defer the configuration of the specified device until after > > * root file system is mounted. > > diff --git sys/sys/device.h sys/sys/device.h > > index 00a1f6ad2a6..3820cc6e0f5 100644 > > --- sys/sys/device.h > > +++ sys/sys/device.h > > @@ -185,6 +185,7 @@ int config_activate_children(struct device *, int); > > struct device *config_make_softc(struct device *parent, > > struct cfdata *cf); > > void config_defer(struct device *, void (*)(struct device *)); > > +void config_defer_failure(struct device *); > > void config_pending_incr(void); > > void config_pending_decr(void); > > void config_mountroot(struct device *, void (*)(struct device *)); > >