Date: Sun, 8 Apr 2012 06:58:38 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Warner Losh <imp@bsdimp.com> Cc: Ian Lepore <freebsd@damnhippie.dyndns.org>, current@freebsd.org, drivers@freebsd.org Subject: Re: device_attach(9) and driver initialization Message-ID: <20120408035838.GY2358@deviant.kiev.zoral.com.ua> In-Reply-To: <CCBC36A4-CC13-465B-A151-A41DEDF7E55B@bsdimp.com> References: <20120407125056.GS2358@deviant.kiev.zoral.com.ua> <1333810001.1082.36.camel@revolution.hippie.lan> <20120407145728.GV2358@deviant.kiev.zoral.com.ua> <CCBC36A4-CC13-465B-A151-A41DEDF7E55B@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120408035838.GY2358>