On Monday, April 09, 2012 3:10:00 pm Konstantin Belousov wrote:
> On Mon, Apr 09, 2012 at 11:01:03AM -0400, John Baldwin wrote:
> > On Saturday, April 07, 2012 11:08:41 pm Warner Losh wrote:
> > >
> > > On Apr 7, 2012, at 8:46 AM, Ian Lepore wrote:
> > >
> > > > On Sat, 2012-04-07 at 15:50 +0300, Konstantin Belousov wrote:
> > > >> Hello,
> > > >> there seems to be a problem with device attach sequence offered by
> > newbus.
> > > >> Basically, when device attach method is executing, device is not fully
> > > >> initialized yet. Also the device state in the newbus part of the world
> > > >> is DS_ALIVE. There is definitely no shattering news in the statements,
> > > >> but drivers that e.g. create devfs node to communicate with consumers
> > > >> are prone to a race.
> > > >>
> > > >> If /dev node is created inside device attach method, then usermode
> > > >> can start calling cdevsw methods before device fully initialized
> > > >> itself.
> > > >> Even more, if device tries to use newbus helpers in cdevsw methods,
> > > >> like device_busy(9), then panic occurs "called for unatteched device".
> > > >> I get reports from users about this issues, to it is not something
> > > >> that only could happen.
> > > >>
> > > >> I propose to add DEVICE_AFTER_ATTACH() driver method, to be called
> > > >> from newbus right after device attach finished and newbus considers
> > > >> the device fully initialized. Driver then could create devfs node
> > > >> in the after_attach method instead of attach. Please see the patch
> > > >> below.
> > > >>
> > > >> diff --git a/sys/kern/device_if.m b/sys/kern/device_if.m
> > > >> index eb720eb..9db74e2 100644
> > > >> --- a/sys/kern/device_if.m
> > > >> +++ b/sys/kern/device_if.m
> > > >> @@ -43,6 +43,10 @@ INTERFACE device;
> > > >> # Default implementations of some methods.
> > > >> #
> > > >> CODE {
> > > >> + static void null_after_attach(device_t dev)
> > > >> + {
> > > >> + }
> > > >> +
> > > >> static int null_shutdown(device_t dev)
> > > >> {
> > > >> return 0;
> > > >> @@ -199,6 +203,21 @@ METHOD int attach {
> > > >> };
> > > >>
> > > >> /**
> > > >> + * @brief Notify the driver that device is in attached state
> > > >> + *
> > > >> + * Called after driver is successfully attached to the device and
> > > >> + * corresponding device_t is fully operational. Driver now may expose
> > > >> + * the device to the consumers, e.g. create devfs nodes.
> > > >> + *
> > > >> + * @param dev the device to probe
> > > >> + *
> > > >> + * @see DEVICE_ATTACH()
> > > >> + */
> > > >> +METHOD void after_attach {
> > > >> + device_t dev;
> > > >> +} DEFAULT null_after_attach;
> > > >> +
> > > >> +/**
> > > >> * @brief Detach a driver from a device.
> > > >> *
> > > >> * This can be called if the user is replacing the
> > > >> diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c
> > > >> index d485b9f..6d849cb 100644
> > > >> --- a/sys/kern/subr_bus.c
> > > >> +++ b/sys/kern/subr_bus.c
> > > >> @@ -2743,6 +2743,7 @@ device_attach(device_t dev)
> > > >> dev->state = DS_ATTACHED;
> > > >> dev->flags &= ~DF_DONENOMATCH;
> > > >> devadded(dev);
> > > >> + DEVICE_AFTER_ATTACH(dev);
> > > >> return (0);
> > > >> }
> > > >>
> > > >
> > > > Does device_get_softc() work before attach is completed? (I don't have
> > > > time to go look in the code right now). If so, then a mutex initialized
> > > > and acquired early in the driver's attach routine, and also acquired in
> > > > the driver's cdev implementation routines before using any newbus
> > > > functions other than device_get_softc(), would solve the problem without
> > > > a driver api change that would make it harder to backport/MFC driver
> > > > changes.
> > >
> > > Also, more generally, don't create the dev nodes before you are ready to
> > deal with requests. Why do we need to uglify everything here? If you
> > can't
> > do that, you can check a bit in the softc and return EBUSY or ENXIO on open
> > if
> > that bit says that your driver isn't ready to accept requests.
> >
> > I agree, this dosen't actually fix anything as the decision for what to put
> > in your foo_attach() method rather than foo_after_attach() is non-obvious
> > and
> > very arbitrary.
> >
> > The actual bug appears to only be with using 'device_busy()'. I think
> > this should be fixed by making device_busy() better, not by adding
> > this type of obfuscation to attach. It should be trivial to make
> > device_busy() safe to use on a device that is currently being attached
> > which will not require any changes to drivers.
>
> Could you, please, elaborate your proposal ? How do you think device_busy()
> can be enchanced ?
>
> Obvious idea to sleep inside device_busy() until dev->state becomes !=
> DS_ATTACHED is no go, IMO. The issue is that this causes immediate deadlocks
> if device_attach() method needs to call destroy_dev() to rollback.
I think you could have a DS_ATTACHING state and allow device_busy() to work
for DS_ATTACHING. The idea being that it is a bug for a driver to invoke
device_busy() if it is going to fail attach. You may then need to do a fixup
in device_attach() to promote the state from DS_ATTACHED to DS_BUSY when it
returns if there is a non-zero busy count.
Something like this:
Index: kern/subr_bus.c
===================================================================
--- kern/subr_bus.c (revision 234057)
+++ kern/subr_bus.c (working copy)
@@ -2472,12 +2472,13 @@
void
device_busy(device_t dev)
{
- if (dev->state < DS_ATTACHED)
+ if (dev->state < DS_ATTACHING)
panic("device_busy: called for unattached device");
if (dev->busy == 0 && dev->parent)
device_busy(dev->parent);
dev->busy++;
- dev->state = DS_BUSY;
+ if (dev->state == DS_ATTACHED)
+ dev->state = DS_BUSY;
}
/**
@@ -2486,14 +2487,16 @@
void
device_unbusy(device_t dev)
{
- if (dev->state != DS_BUSY)
+ if (dev->busy != 0 && dev->state != DS_BUSY &&
+ dev->state != DS_ATTACHING)
panic("device_unbusy: called for non-busy device %s",
device_get_nameunit(dev));
dev->busy--;
if (dev->busy == 0) {
if (dev->parent)
device_unbusy(dev->parent);
- dev->state = DS_ATTACHED;
+ if (dev->state == DS_BUSY)
+ dev->state = DS_ATTACHED;
}
}
@@ -2729,6 +2732,7 @@
device_sysctl_init(dev);
if (!device_is_quiet(dev))
device_print_child(dev->parent, dev);
+ dev->state = DS_ATTACHING;
if ((error = DEVICE_ATTACH(dev)) != 0) {
printf("device_attach: %s%d attach returned %d\n",
dev->driver->name, dev->unit, error);
@@ -2736,11 +2740,15 @@
devclass_delete_device(dev->devclass, dev);
(void)device_set_driver(dev, NULL);
device_sysctl_fini(dev);
+ KASSERT(dev->busy == 0, ("attach failed but busy"));
dev->state = DS_NOTPRESENT;
return (error);
}
device_sysctl_update(dev);
- dev->state = DS_ATTACHED;
+ if (dev->busy)
+ dev->state = DS_BUSY;
+ else
+ dev->state = DS_ATTACHED;
dev->flags &= ~DF_DONENOMATCH;
devadded(dev);
return (0);
Index: sys/bus.h
===================================================================
--- sys/bus.h (revision 234057)
+++ sys/bus.h (working copy)
@@ -52,6 +52,7 @@
typedef enum device_state {
DS_NOTPRESENT = 10, /**< @brief not probed or probe failed
*/
DS_ALIVE = 20, /**< @brief probe succeeded */
+ DS_ATTACHING = 25, /**< @brief currently attaching */
DS_ATTACHED = 30, /**< @brief attach method called */
DS_BUSY = 40 /**< @brief device is open */
} device_state_t;
--
John Baldwin
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
