Date: Sat, 7 Apr 2012 21:10:55 -0600 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Ian Lepore <freebsd@damnhippie.dyndns.org>, current@FreeBSD.org, drivers@FreeBSD.org Subject: Re: device_attach(9) and driver initialization Message-ID: <CCBC36A4-CC13-465B-A151-A41DEDF7E55B@bsdimp.com> 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 Apr 7, 2012, at 8:57 AM, 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.
>>>=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.
> 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 ?
driver_attach()
{
...
softc->flags =3D 0; // redundant, since softc is initialized to =
0.
softc->cdev =3D device_create...();
...
softc->flags |=3D READY;
}
driver_open(...)
{
if (!(softc->flags & READY))
return ENXIO;
...
}
What's the big burden here?
Warner=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CCBC36A4-CC13-465B-A151-A41DEDF7E55B>