svn commit: r293349 - head/sys/kern

[Bruce Evans]

On Thu, 7 Jan 2016, Konstantin Belousov wrote:

> Log:
>  Convert tty common code to use make_dev_s().
>
>  Tty.c was untypical in that it handled the si_drv1 issue consistently
>  and correctly, by always checking for si_drv1 being non-NULL and
>  sleeping if NULL.  The removed code also illustrated unneeded
>  complications in drivers which are eliminated by the use of new KPI.

Actually, the handling was consistently incorrect.  In the case where
si_drv1 is actually NULL, it usually returned EWOULDBLOCK instead of 0
for successful opens.  This is a rare case and I haven't seen it so
I'm not sure how to recover from it.  revoke() of the device should
work.  Even open() followed by a "last" close() would probably work.

>  Reviewed by:	hps, jhb
>  Discussed with:	bde

There seem to further old and unfixed problems.  Sorry I didn't look
at your patch very closely.  I will reply privately the complicated
details of this.

> Modified: head/sys/kern/tty.c
> ==============================================================================
> --- head/sys/kern/tty.c	Thu Jan  7 20:15:05 2016	(r293348)
> +++ head/sys/kern/tty.c	Thu Jan  7 20:15:09 2016	(r293349)
> @@ -237,14 +237,10 @@ static int
> ttydev_open(struct cdev *dev, int oflags, int devtype, struct thread *td)
> {
> 	struct tty *tp;
> -	int error = 0;

This code used to be just 2 style bugs:
- initialization in declaration
- use of the initialization much later.  It is to get the variable
   initialized for code starting about 50 lines later, in case that
   code falls through an if ladder to th return another 40 lines later.

> -
> -	while ((tp = dev->si_drv1) == NULL) {
> -		error = tsleep(&dev->si_drv1, PCATCH, "ttdrv1", 1);
> -		if (error != EWOULDBLOCK)
> -			return (error);
> -	}

The initialization became not just a style bug when the si_drv1 handling
corrupted it.

Urk, the handling was almost completely incorrect:
- the timeout shouldn't be needed.  There is a wakeup when si_drv1 is
   initialized.
- if the wakeup actually works, then its handling is very broken.  Then
   tsleep() returns 0.  This is treated as an error, and the function
   returns 0, which means success, but the devie has not been opened.
- PCATCH shouldn't be needed either, but is not harmful like the buggy
   check for the unnecessary timeout.

> +	int error;
>
> +	tp = dev->si_drv1;
> +	error = 0;

The initialization is now correct and only 40 lines before it is needed.

> 	tty_lock(tp);
> 	if (tty_gone(tp)) {
> 		/* Device is already gone. */

In my version, 'error' is initialized by more complicated checks on entry
that naturally set it to 0 when they succeed.  After returning when
error != 0 early, it would be unnatural to set it to 0 again later.

> @@ -1221,71 +1215,72 @@ tty_makedevf(struct tty *tp, struct ucre
> 	flags |= MAKEDEV_CHECKNAME;
>
> 	/* Master call-in device. */
> -	error = make_dev_p(flags, &dev, &ttydev_cdevsw, cred, uid, gid, mode,
> -	    "%s%s", prefix, name);
> -	if (error)
> +	make_dev_args_init(&args);
> +	args.mda_flags = flags;
> +	args.mda_devsw = &ttydev_cdevsw;
> +	args.mda_cr = cred;
> +	args.mda_uid = uid;
> +	args.mda_gid = gid;
> +	args.mda_mode = mode;
> +	args.mda_si_drv1 = tp;
> +	error = make_dev_s(&args, &dev, "%s%s", prefix, name);
> +	if (error != 0)
> 		return (error);
> -	dev->si_drv1 = tp;
> -	wakeup(&dev->si_drv1);

Wakeups like this should have made the timeout unnecessary.  However, it
would be better to not have them.  This wakeup seems to give an instant
race, while the timeout will often be delayed long enough for the
initialization to complete.

> 	tp->t_dev = dev;

The new version presumably fixes the initialiation order for dev->si_drv1,
but it doesn't change this.  This is delicate.  The details are too large
for this reply.

Bruce