From owner-freebsd-drivers@FreeBSD.ORG Sun Apr 8 03:58:55 2012 Return-Path: Delivered-To: drivers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id F1821106564A; Sun, 8 Apr 2012 03:58:55 +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 66EA48FC08; Sun, 8 Apr 2012 03:58:54 +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 q383wd2B099237; Sun, 8 Apr 2012 06:58:39 +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 q383wcZE092081; Sun, 8 Apr 2012 06:58:38 +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 q383wch2092080; Sun, 8 Apr 2012 06:58:38 +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: Sun, 8 Apr 2012 06:58:38 +0300 From: Konstantin Belousov To: Warner Losh Message-ID: <20120408035838.GY2358@deviant.kiev.zoral.com.ua> References: <20120407125056.GS2358@deviant.kiev.zoral.com.ua> <1333810001.1082.36.camel@revolution.hippie.lan> <20120407145728.GV2358@deviant.kiev.zoral.com.ua> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="6u7EtAStsNhw5cUf" Content-Disposition: inline In-Reply-To: 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: Ian Lepore , current@freebsd.org, drivers@freebsd.org Subject: Re: device_attach(9) and driver initialization X-BeenThere: freebsd-drivers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Writing device drivers for FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 Apr 2012 03:58:56 -0000 --6u7EtAStsNhw5cUf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Apr 07, 2012 at 09:10:55PM -0600, Warner Losh wrote: >=20 > On Apr 7, 2012, at 8:57 AM, Konstantin Belousov wrote: >=20 > > 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 ne= wbus. > >>> 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 itse= lf. > >>> 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 be= low. > >>>=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 initializ= ed > >> 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 witho= ut > >> 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. > >=20 > > Singlethreading a driver due to this race is just silly. > >=20 > > And, what do you mean by 'making it harder to MFC' ? How ? >=20 > driver_attach() > { > ... > softc->flags =3D 0; // redundant, since softc is initialized to 0. > softc->cdev =3D device_create...(); > ... > softc->flags |=3D READY; > } >=20 > driver_open(...) > { > if (!(softc->flags & READY)) > return ENXIO; > ... > } >=20 > What's the big burden here? The burden is that your proposal does not work. As I described above, device_busy() calls from cdevsw method panic if open() is called before DS_ATTACHED is set by newbus. And, DS_ATTACHED is only set after device attach() method returned, so no workarounds from attach() could solve this. --6u7EtAStsNhw5cUf Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAk+BDO4ACgkQC3+MBN1Mb4hckQCgsGf27T3LtF0hlsguyfG8JpPu 1rUAnjvjSYfLZ6uFkgGSKYrXPSokZ3w8 =AgpQ -----END PGP SIGNATURE----- --6u7EtAStsNhw5cUf--