Re: device_attach(9) and driver initialization

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=
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"