From owner-freebsd-drivers@FreeBSD.ORG Mon Apr 9 19:10:14 2012 Return-Path: Delivered-To: drivers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id CEE6E106564A; Mon, 9 Apr 2012 19:10:14 +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 0EC598FC08; Mon, 9 Apr 2012 19:10:13 +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 q39JA2Zj015198; Mon, 9 Apr 2012 22:10:02 +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 q39JA1SR051583; Mon, 9 Apr 2012 22:10:01 +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 q39JA0mH051582; Mon, 9 Apr 2012 22:10:01 +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: Mon, 9 Apr 2012 22:10:00 +0300 From: Konstantin Belousov To: John Baldwin Message-ID: <20120409191000.GS2358@deviant.kiev.zoral.com.ua> References: <20120407125056.GS2358@deviant.kiev.zoral.com.ua> <1333810001.1082.36.camel@revolution.hippie.lan> <7C5089DC-AB9B-458F-A606-B432505BD961@bsdimp.com> <201204091101.03956.jhb@freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="pC/tHwOGf8AW2ISZ" Content-Disposition: inline In-Reply-To: <201204091101.03956.jhb@freebsd.org> 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 , drivers@freebsd.org, freebsd-drivers@freebsd.org, current@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: Mon, 09 Apr 2012 19:10:14 -0000 --pC/tHwOGf8AW2ISZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: > >=20 > > On Apr 7, 2012, at 8:46 AM, Ian Lepore wrote: > >=20 > > > 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= =20 > newbus. > > >> Basically, when device attach method is executing, device is not ful= ly > > >> initialized yet. Also the device state in the newbus part of the wor= ld > > >> is DS_ALIVE. There is definitely no shattering news in the statement= s, > > >> 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 its= elf. > > >> 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 b= elow. > > >>=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 expo= se > > >> + * 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 ha= ve > > > time to go look in the code right now). If so, then a mutex initiali= zed > > > 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 with= out > > > a driver api change that would make it harder to backport/MFC driver > > > changes. > >=20 > > Also, more generally, don't create the dev nodes before you are ready t= o=20 > deal with requests. Why do we need to uglify everything here? If you ca= n't=20 > do that, you can check a bit in the softc and return EBUSY or ENXIO on op= en if=20 > that bit says that your driver isn't ready to accept requests. >=20 > I agree, this dosen't actually fix anything as the decision for what to p= ut > in your foo_attach() method rather than foo_after_attach() is non-obvious= and=20 > very arbitrary. >=20 > 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 !=3D 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. Pointing driver authors to destroy_dev_sched() for this purpose is overkill, since average driver author, me included, could not use destroy_dev_sched() properly, at least without lot of works. --pC/tHwOGf8AW2ISZ Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAk+DNAgACgkQC3+MBN1Mb4iFywCg7rSjfbyQaBeVRxgyn5c23OJf tSQAn1RNLlJnVXxklIrOxfJrXFaiSyk7 =I1pD -----END PGP SIGNATURE----- --pC/tHwOGf8AW2ISZ--