Date: Sat, 7 Apr 2012 21:08:41 -0600 From: Warner Losh <imp@bsdimp.com> To: Ian Lepore <freebsd@damnhippie.dyndns.org> Cc: 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>