From owner-freebsd-current@FreeBSD.ORG Sat Apr 7 14:57:40 2012 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 6037A1065673; Sat, 7 Apr 2012 14:57:40 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id C77CE8FC08; Sat, 7 Apr 2012 14:57:39 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q37EvTZZ027789; Sat, 7 Apr 2012 17:57:29 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q37EvSfY087985; Sat, 7 Apr 2012 17:57:28 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q37EvSMd087984; Sat, 7 Apr 2012 17:57:28 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 7 Apr 2012 17:57:28 +0300 From: Konstantin Belousov To: Ian Lepore Message-ID: <20120407145728.GV2358@deviant.kiev.zoral.com.ua> References: <20120407125056.GS2358@deviant.kiev.zoral.com.ua> <1333810001.1082.36.camel@revolution.hippie.lan> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="CS4y9zAoaOKmm/jR" Content-Disposition: inline In-Reply-To: <1333810001.1082.36.camel@revolution.hippie.lan> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: current@freebsd.org, drivers@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: Sat, 07 Apr 2012 14:57:40 -0000 --CS4y9zAoaOKmm/jR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Apr 07, 2012 at 08:46:41AM -0600, 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 newb= us. > > 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. > >=20 > > 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. > >=20 > > 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 belo= w. > >=20 > > 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 { > > }; > > =20 > > /** > > + * @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 =3D DS_ATTACHED; > > dev->flags &=3D ~DF_DONENOMATCH; > > devadded(dev); > > + DEVICE_AFTER_ATTACH(dev); > > return (0); > > } > > =20 >=20 > 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. No, 'a mutex' does not solve anything. It only adds enourmous burden on the driver developers, because you cannot sleep under mutex. Changing the mutex to the sleepable lock also does not byy you much, since you need to somehow solve the issues with some cdevsw call waking up thread sleeping into another cdevsw call, just for example. Singlethreading a driver due to this race is just silly. And, what do you mean by 'making it harder to MFC' ? How ? --CS4y9zAoaOKmm/jR Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAk+AVdgACgkQC3+MBN1Mb4jTrACdFHKoYkgbu8XZDxumRGi+XhUK TQUAnR0ERsly/TQuBVeUbjyphXO4DtNO =TyEu -----END PGP SIGNATURE----- --CS4y9zAoaOKmm/jR--