Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 7 Apr 2012 21:08:41 -0600
From:      Warner Losh <imp@bsdimp.com>
To:        Ian Lepore <freebsd@damnhippie.dyndns.org>
Cc:        Konstantin Belousov <kostikbel@gmail.com>, current@FreeBSD.org, drivers@FreeBSD.org
Subject:   Re: device_attach(9) and driver initialization
Message-ID:  <7C5089DC-AB9B-458F-A606-B432505BD961@bsdimp.com>
In-Reply-To: <1333810001.1082.36.camel@revolution.hippie.lan>
References:  <20120407125056.GS2358@deviant.kiev.zoral.com.ua> <1333810001.1082.36.camel@revolution.hippie.lan>

next in thread | previous in thread | raw e-mail | index | archive | help

On Apr 7, 2012, at 8:46 AM, 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.
>>=20
>> 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.
>>=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 =
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 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 =
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.

Also, more generally, don't create the dev nodes before you are ready to =
deal with requests.  Why do we need to uglify everything here?  If you =
can't do that, you can check a bit in the softc and return EBUSY or =
ENXIO on open if that bit says that your driver isn't ready to accept =
requests.

Warner=



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7C5089DC-AB9B-458F-A606-B432505BD961>