Date: Mon, 09 Apr 2012 08:07:55 -0600 From: Ian Lepore <freebsd@damnhippie.dyndns.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: current@freebsd.org, drivers@freebsd.org Subject: Re: device_attach(9) and driver initialization Message-ID: <1333980475.1082.47.camel@revolution.hippie.lan> In-Reply-To: <20120407145728.GV2358@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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 2012-04-07 at 17:57 +0300, Konstantin Belousov wrote:
> 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 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 statements,
> > > but drivers that e.g. create devfs node to communicate with consumers
> > > are prone to a race.
> > >
> > > If /dev node is created inside device attach method, then usermode
> > > can start calling cdevsw methods before device fully initialized itself.
> > > 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.
> > >
> > > 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 below.
> > >
> > > 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 {
> > > };
> > >
> > > /**
> > > + * @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 = DS_ATTACHED;
> > > dev->flags &= ~DF_DONENOMATCH;
> > > devadded(dev);
> > > + DEVICE_AFTER_ATTACH(dev);
> > > return (0);
> > > }
> > >
> >
> > 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 initialized
> > 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 without
> > 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.
>
> Singlethreading a driver due to this race is just silly.
>
> And, what do you mean by 'making it harder to MFC' ? How ?
I frequently find myself having to backport driver changes further back
than any currently-supported FreeBSD release, and something like a new
function in newbus can make that pretty hard to do. That often makes me
think of how accomplish something with a minimally-invasive change. (So
it's really more of a selfish personal goal more than a FreeBSD-project
goal.)
-- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1333980475.1082.47.camel>