From owner-freebsd-current@FreeBSD.ORG Mon Apr 9 19:36:09 2012 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B37C8106566C; Mon, 9 Apr 2012 19:36:09 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 75A368FC0A; Mon, 9 Apr 2012 19:36:09 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id E25FDB95F; Mon, 9 Apr 2012 15:36:08 -0400 (EDT) From: John Baldwin To: Konstantin Belousov Date: Mon, 9 Apr 2012 15:36:08 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p10; KDE/4.5.5; amd64; ; ) References: <20120407125056.GS2358@deviant.kiev.zoral.com.ua> <201204091101.03956.jhb@freebsd.org> <20120409191000.GS2358@deviant.kiev.zoral.com.ua> In-Reply-To: <20120409191000.GS2358@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201204091536.08399.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 09 Apr 2012 15:36:09 -0400 (EDT) Cc: Ian Lepore , freebsd-drivers@freebsd.org, current@freebsd.org Subject: Re: device_attach(9) and driver initialization X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Apr 2012 19:36:09 -0000 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