subr_autoconf: allow _attach to fail? w/no void2int churn option

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

subr_autoconf: allow _attach to fail? w/no void2int churn option

Artturi Alm
Hi,

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 *));

Reply | Threaded
Open this post in threaded view
|

Re: subr_autoconf: allow _attach to fail? w/no void2int churn option

Theo de Raadt-2
I think this approach is wrong, insane, and fragile.  DVF_ACTIVE
doesn't work precisely that way.

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 *));
>

Reply | Threaded
Open this post in threaded view
|

Re: subr_autoconf: allow _attach to fail? w/no void2int churn option

Artturi Alm
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 *));
> >

Reply | Threaded
Open this post in threaded view
|

Re: subr_autoconf: allow _attach to fail? w/no void2int churn option

Theo de Raadt-2
In reply to this post by Artturi Alm
>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:

for 1 driver.

Reply | Threaded
Open this post in threaded view
|

Re: subr_autoconf: allow _attach to fail? w/no void2int churn option

Artturi Alm
On Tue, Apr 10, 2018 at 02:40:29PM -0600, Theo de Raadt wrote:
> >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:
>
> for 1 driver.

i offered a horse. if it wasn't good enough, why bother with more work;
as i don't see how it would make the horse any better at this point:)

-Artturi