Date: Mon, 9 Apr 2012 22:10:00 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Ian Lepore <freebsd@damnhippie.dyndns.org>, drivers@freebsd.org, freebsd-drivers@freebsd.org, current@freebsd.org Subject: Re: device_attach(9) and driver initialization Message-ID: <20120409191000.GS2358@deviant.kiev.zoral.com.ua> In-Reply-To: <201204091101.03956.jhb@freebsd.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120409191000.GS2358>