Re: device_attach(9) and driver initialization

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
> _at__at_ -43,6 +43,10 _at__at_ 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;
> _at__at_ -199,6 +203,21 _at__at_ METHOD int attach {
>  };
>  
>  /**
> + * _at_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.
> + *
> + * _at_param dev		the device to probe
> + *
> + * _at_see DEVICE_ATTACH()
> + */
> +METHOD void after_attach {
> +	device_t dev;
> +} DEFAULT null_after_attach;
> +
> +/**
>   * _at_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
> _at__at_ -2743,6 +2743,7 _at__at_ 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.

-- Ian