Date: Mon, 9 Apr 2012 23:05:29 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Ian Lepore <freebsd@damnhippie.dyndns.org>, freebsd-drivers@freebsd.org, current@freebsd.org Subject: Re: device_attach(9) and driver initialization Message-ID: <20120409200529.GT2358@deviant.kiev.zoral.com.ua> In-Reply-To: <201204091536.08399.jhb@freebsd.org> References: <20120407125056.GS2358@deviant.kiev.zoral.com.ua> <201204091101.03956.jhb@freebsd.org> <20120409191000.GS2358@deviant.kiev.zoral.com.ua> <201204091536.08399.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--3e+tHtSlmys++yUp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Mon, Apr 09, 2012 at 03:36:08PM -0400, John Baldwin wrote:
> On Monday, April 09, 2012 3:10:00 pm Konstantin Belousov wrote:
> > 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=
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 state=
ments,
> > > > >> but drivers that e.g. create devfs node to communicate with cons=
umers
> > > > >> are prone to a race.
> > > > >>=20
> > > > >> If /dev node is created inside device attach method, then usermo=
de
> > > > >> can start calling cdevsw methods before device fully initialized=
itself.
> > > > >> Even more, if device tries to use newbus helpers in cdevsw metho=
ds,
> > > > >> like device_busy(9), then panic occurs "called for unatteched de=
vice".
> > > > >> I get reports from users about this issues, to it is not somethi=
ng
> > > > >> that only could happen.
> > > > >>=20
> > > > >> I propose to add DEVICE_AFTER_ATTACH() driver method, to be call=
ed
> > > > >> from newbus right after device attach finished and newbus consid=
ers
> > > > >> the device fully initialized. Driver then could create devfs node
> > > > >> in the after_attach method instead of attach. Please see the pat=
ch below.
> > > > >>=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 a=
nd
> > > > >> + * 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 init=
ialized
> > > > > and acquired early in the driver's attach routine, and also acqui=
red 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 dri=
ver
> > > > > changes.
> > > >=20
> > > > Also, more generally, don't create the dev nodes before you are rea=
dy to=20
> > > deal with requests. Why do we need to uglify everything here? If yo=
u can't=20
> > > do that, you can check a bit in the softc and return EBUSY or ENXIO o=
n open 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 put
> > > in your foo_attach() method rather than foo_after_attach() is non-obv=
ious 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.
> >=20
> > Could you, please, elaborate your proposal ? How do you think device_bu=
sy()
> > can be enchanced ?
> >=20
> > 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 dead=
locks
> > if device_attach() method needs to call destroy_dev() to rollback.
>=20
> I think you could have a DS_ATTACHING state and allow device_busy() to wo=
rk
> for DS_ATTACHING. The idea being that it is a bug for a driver to invoke
> device_busy() if it is going to fail attach. You may then need to do a f=
ixup
> in device_attach() to promote the state from DS_ATTACHED to DS_BUSY when =
it
> returns if there is a non-zero busy count.
This is quite good idea, but it still adds burden to device author,
although I agree that this is manageable. A scenario I have in mind now
is the following:
assume that driver needs to create two devfs nodes, lets name them
dri/card0 and dri/forcewake0. Driver would perform two make_dev_p(9)
calls, and while creation of dri/card0 succeed, consequent creation
of dri/forcewake0 could fail for numerous reasons.
Now, the driver needs to ensure that cdesvw->d_open() on dri/card0
would return ENXIO until dri/forcewake0 is created. This can be implemented
with flag, indeed. But still somewhat muddy, and probably leads to
user-visible errors (I mostly worry about graphical login managers).
But for single-node drivers it is indeed a nice solution.
>=20
> Something like this:
>=20
> Index: kern/subr_bus.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- kern/subr_bus.c (revision 234057)
> +++ kern/subr_bus.c (working copy)
> @@ -2472,12 +2472,13 @@
> void
> device_busy(device_t dev)
> {
> - if (dev->state < DS_ATTACHED)
> + if (dev->state < DS_ATTACHING)
> panic("device_busy: called for unattached device");
> if (dev->busy =3D=3D 0 && dev->parent)
> device_busy(dev->parent);
> dev->busy++;
> - dev->state =3D DS_BUSY;
> + if (dev->state =3D=3D DS_ATTACHED)
> + dev->state =3D DS_BUSY;
> }
> =20
> /**
> @@ -2486,14 +2487,16 @@
> void
> device_unbusy(device_t dev)
> {
> - if (dev->state !=3D DS_BUSY)
> + if (dev->busy !=3D 0 && dev->state !=3D DS_BUSY &&
> + dev->state !=3D DS_ATTACHING)
> panic("device_unbusy: called for non-busy device %s",
> device_get_nameunit(dev));
> dev->busy--;
> if (dev->busy =3D=3D 0) {
> if (dev->parent)
> device_unbusy(dev->parent);
> - dev->state =3D DS_ATTACHED;
> + if (dev->state =3D=3D DS_BUSY)
> + dev->state =3D DS_ATTACHED;
> }
> }
> =20
> @@ -2729,6 +2732,7 @@
> device_sysctl_init(dev);
> if (!device_is_quiet(dev))
> device_print_child(dev->parent, dev);
> + dev->state =3D DS_ATTACHING;
> if ((error =3D DEVICE_ATTACH(dev)) !=3D 0) {
> printf("device_attach: %s%d attach returned %d\n",
> dev->driver->name, dev->unit, error);
> @@ -2736,11 +2740,15 @@
> devclass_delete_device(dev->devclass, dev);
> (void)device_set_driver(dev, NULL);
> device_sysctl_fini(dev);
> + KASSERT(dev->busy =3D=3D 0, ("attach failed but busy"));
> dev->state =3D DS_NOTPRESENT;
> return (error);
> }
> device_sysctl_update(dev);
> - dev->state =3D DS_ATTACHED;
> + if (dev->busy)
> + dev->state =3D DS_BUSY;
> + else
> + dev->state =3D DS_ATTACHED;
> dev->flags &=3D ~DF_DONENOMATCH;
> devadded(dev);
> return (0);
> Index: sys/bus.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- sys/bus.h (revision 234057)
> +++ sys/bus.h (working copy)
> @@ -52,6 +52,7 @@
> typedef enum device_state {
> DS_NOTPRESENT =3D 10, /**< @brief not probed or probe failed */
> DS_ALIVE =3D 20, /**< @brief probe succeeded */
> + DS_ATTACHING =3D 25, /**< @brief currently attaching */
> DS_ATTACHED =3D 30, /**< @brief attach method called */
> DS_BUSY =3D 40 /**< @brief device is open */
> } device_state_t;
>=20
> --=20
> John Baldwin
--3e+tHtSlmys++yUp
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (FreeBSD)
iEYEARECAAYFAk+DQQkACgkQC3+MBN1Mb4gq+wCgmVOauphI9rfoDe0vN+/yPiK1
XH4An2S95rMdXjh59SBwTNiu64r1fvQO
=Sjwn
-----END PGP SIGNATURE-----
--3e+tHtSlmys++yUp--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120409200529.GT2358>