[RFC][PATCH 1/3] Move parts of init_dev() into new functions

[RFC][PATCH 1/3] Move parts of init_dev() into new functions

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

init_dev() is doing too many things all in one function, making it
harder to understand/extend. This and following two patches attempt
to simplify/cleanup the interface a bit.

Only touch tested at this point. Appreciate any comments :-)

---

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

Move the 'find-tty' and 'fast-track-open' parts of init_dev() to
separate functions.
---
 drivers/char/tty_io.c |  140 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 89 insertions(+), 51 deletions(-)

Index: linux-next/drivers/char/tty_io.c
===================================================================
--- linux-next.orig/drivers/char/tty_io.c	2008-08-25 11:57:15.000000000 -0700
+++ linux-next/drivers/char/tty_io.c	2008-08-25 12:31:15.000000000 -0700
@@ -1213,6 +1213,82 @@ static void tty_line_name(struct tty_dri
 	sprintf(p, "%s%d", driver->name, index + driver->name_base);
 }
 
+/*
+ * 	find_tty() - find an existing tty, if any
+ * 	@driver: the driver for the tty
+ * 	@idx:	 the minor number
+ *
+ * 	Return the tty, if found or ERR_PTR() otherwise.
+ *
+ * 	Locking: tty_mutex must be held. If tty is found, the mutex must
+ * 		 be held until the 'fast-open' is also done.
+ */
+struct tty_struct *find_tty(struct tty_driver *driver, int idx)
+{
+	struct tty_struct *tty;
+
+	/* check whether we're reopening an existing tty */
+	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
+		tty = devpts_get_tty(idx);
+		/*
+		 * If we don't have a tty here on a slave open, it's because
+		 * the master already started the close process and there's
+		 * no relation between devpts file and tty anymore.
+		 */
+		if (!tty && driver->subtype == PTY_TYPE_SLAVE)
+			return ERR_PTR(-EIO);
+
+		/*
+		 * tty is safe on because we are called with tty_mutex held
+		 * and release_dev() won't change tty->count or tty->flags
+		 * without grabbing tty_mutex.
+		 */
+		if (tty && driver->subtype == PTY_TYPE_MASTER)
+			tty = tty->link;
+	} else {
+		tty = driver->ttys[idx];
+	}
+
+	return tty;
+}
+
+/*
+ * 	fast_tty_open()	- fast re-open of an open tty
+ * 	@tty	- the tty to open
+ *
+ * 	Return 0 on success, -errno on error.
+ *
+ * 	Locking: tty_mutex must be held from the time the tty was found
+ * 		 till this open completes.
+ */
+static int fast_tty_open(struct tty_struct *tty)
+{
+	struct tty_driver *driver = tty->driver;
+
+	if (test_bit(TTY_CLOSING, &tty->flags))
+		return -EIO;
+
+	if (driver->type == TTY_DRIVER_TYPE_PTY &&
+	    driver->subtype == PTY_TYPE_MASTER) {
+		/*
+		 * special case for PTY masters: only one open permitted,
+		 * and the slave side open count is incremented as well.
+		 */
+		if (tty->count)
+			return -EIO;
+
+		tty->link->count++;
+	}
+	tty->count++;
+	tty->driver = driver; /* N.B. why do this every time?? */
+
+	/* FIXME */
+	if (!test_bit(TTY_LDISC, &tty->flags))
+		printk(KERN_ERR "fast_tty_open: no ldisc\n");
+
+	return 0;
+}
+
 /**
  *	init_dev		-	initialise a tty device
  *	@driver: tty driver we are opening a device on
@@ -1246,30 +1322,21 @@ static int init_dev(struct tty_driver *d
 	struct ktermios *ltp, **ltp_loc, *o_ltp, **o_ltp_loc;
 	int retval = 0;
 
-	/* check whether we're reopening an existing tty */
-	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
-		tty = devpts_get_tty(idx);
-		/*
-		 * If we don't have a tty here on a slave open, it's because
-		 * the master already started the close process and there's
-		 * no relation between devpts file and tty anymore.
-		 */
-		if (!tty && driver->subtype == PTY_TYPE_SLAVE) {
-			retval = -EIO;
-			goto end_init;
-		}
-		/*
-		 * It's safe from now on because init_dev() is called with
-		 * tty_mutex held and release_dev() won't change tty->count
-		 * or tty->flags without having to grab tty_mutex
-		 */
-		if (tty && driver->subtype == PTY_TYPE_MASTER)
-			tty = tty->link;
-	} else {
-		tty = driver->ttys[idx];
+	tty = find_tty(driver, idx);
+	if (IS_ERR(tty)) {
+		retval = PTR_ERR(tty);
+		goto end_init;
 	}
-	if (tty) goto fast_track;
 
+	if (tty) {
+		retval = fast_tty_open(tty);
+		if (retval)
+			return retval;
+		*ret_tty = tty;
+		return 0;
+	}
+
+	/* Check if pty master is being opened multiple times */
 	if (driver->subtype == PTY_TYPE_MASTER &&
 		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) {
 		retval = -EIO;
@@ -1411,35 +1478,6 @@ static int init_dev(struct tty_driver *d
 		goto release_mem_out;
 	goto success;
 
-	/*
-	 * This fast open can be used if the tty is already open.
-	 * No memory is allocated, and the only failures are from
-	 * attempting to open a closing tty or attempting multiple
-	 * opens on a pty master.
-	 */
-fast_track:
-	if (test_bit(TTY_CLOSING, &tty->flags)) {
-		retval = -EIO;
-		goto end_init;
-	}
-	if (driver->type == TTY_DRIVER_TYPE_PTY &&
-	    driver->subtype == PTY_TYPE_MASTER) {
-		/*
-		 * special case for PTY masters: only one open permitted,
-		 * and the slave side open count is incremented as well.
-		 */
-		if (tty->count) {
-			retval = -EIO;
-			goto end_init;
-		}
-		tty->link->count++;
-	}
-	tty->count++;
-	tty->driver = driver; /* N.B. why do this every time?? */
-
-	/* FIXME */
-	if (!test_bit(TTY_LDISC, &tty->flags))
-		printk(KERN_ERR "init_dev but no ldisc\n");
 success:
 	*ret_tty = tty;

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[Alan Cox]

On Mon, 25 Aug 2008 13:11:10 -0700
sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:

> 
> init_dev() is doing too many things all in one function, making it
> harder to understand/extend. This and following two patches attempt
> to simplify/cleanup the interface a bit.
> 
> Only touch tested at this point. Appreciate any comments :-)

Well this code is very much in flux so I really don't want to get further
changes mixed up into the plan at the wrong moment. That said:

> +/*
> + * 	find_tty() - find an existing tty, if any
> + * 	@driver: the driver for the tty
> + * 	@idx:	 the minor number

This fits very well with where I want to end up - which is


	tty = driver->ops->something(driver, idx);

and thus punts all the pty crap from the tty lookup into the pty driver.

So I think this is looking good as a direction, and once the pty crap is
in the pty code it is much easier to localise all the devpts magic.

Alan

[RFC][PATCH 2/3] Move some init_dev() code to callers

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 2/3] Move some init_dev() code to callers

init_dev() tries to find a tty and if it finds an existing tty, does
a 'fast' open. If its not an existing tty, init_dev does a slower
first time open requiring allocation and complex initialization.

All these seem to make the code more complex. When opening /dev/tty,
the caller already has the tty so there is no need to find it. Further
the fast and slow opens in init_dev() don't really share much code
and could be in separate functions.

With only two callers, init_dev() does not really need to be that
generalized and some of the pieces can be moved into the callers.

---
 drivers/char/tty_io.c |   71 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 47 insertions(+), 24 deletions(-)

Index: linux-next/drivers/char/tty_io.c
===================================================================
--- linux-next.orig/drivers/char/tty_io.c	2008-08-25 12:31:15.000000000 -0700
+++ linux-next/drivers/char/tty_io.c	2008-08-25 12:54:35.000000000 -0700
@@ -1322,20 +1322,6 @@ static int init_dev(struct tty_driver *d
 	struct ktermios *ltp, **ltp_loc, *o_ltp, **o_ltp_loc;
 	int retval = 0;
 
-	tty = find_tty(driver, idx);
-	if (IS_ERR(tty)) {
-		retval = PTR_ERR(tty);
-		goto end_init;
-	}
-
-	if (tty) {
-		retval = fast_tty_open(tty);
-		if (retval)
-			return retval;
-		*ret_tty = tty;
-		return 0;
-	}
-
 	/* Check if pty master is being opened multiple times */
 	if (driver->subtype == PTY_TYPE_MASTER &&
 		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) {
@@ -1476,9 +1462,7 @@ static int init_dev(struct tty_driver *d
 
 	if (retval)
 		goto release_mem_out;
-	goto success;
 
-success:
 	*ret_tty = tty;
 
 	/* All paths come through here to release the mutex */
@@ -1853,14 +1837,15 @@ static void release_dev(struct file *fil
  *	The termios state of a pty is reset on first open so that
  *	settings don't persist across reuse.
  *
- *	Locking: tty_mutex protects tty, get_tty_driver and init_dev work.
+ *	Locking: tty_mutex protects tty, get_tty_driver, find_tty,
+ *		fast_tty_open and init_dev work.
  *		 tty->count should protect the rest.
  *		 ->siglock protects ->signal/->sighand
  */
 
 static int __tty_open(struct inode *inode, struct file *filp)
 {
-	struct tty_struct *tty;
+	struct tty_struct *tty = NULL;
 	int noctty, retval;
 	struct tty_driver *driver;
 	int index;
@@ -1917,8 +1902,19 @@ retry_open:
 		return -ENODEV;
 	}
 got_driver:
-	retval = init_dev(driver, index, &tty, 0);
+	if (!tty) {
+		tty = find_tty(driver, index);
+		if (IS_ERR(tty))
+			return PTR_ERR(tty);
+	}
+
+	if (tty)
+		retval = fast_tty_open(tty);
+	else
+		retval = init_dev(driver, index, &tty, 0);
+
 	mutex_unlock(&tty_mutex);
+
 	if (retval)
 		return retval;
 
@@ -1986,8 +1982,24 @@ static int tty_open(struct inode *inode,
 }
 
 
-
 #ifdef CONFIG_UNIX98_PTYS
+
+static struct tty_struct *find_open_tty(struct tty_driver *driver, int index)
+{
+	int retval;
+	struct tty_struct *tty;
+
+	tty = find_tty(driver, index);
+	if (!tty || IS_ERR(tty))
+		return tty;
+
+	retval = fast_tty_open(tty);
+	if (retval)
+		tty = ERR_PTR(retval);
+
+	return tty;
+}
+
 /**
  *	ptmx_open		-	open a unix 98 pty master
  *	@inode: inode of device file
@@ -1995,15 +2007,15 @@ static int tty_open(struct inode *inode,
  *
  *	Allocate a unix98 pty master device from the ptmx driver.
  *
- *	Locking: tty_mutex protects theinit_dev work. tty->count should
- * 		protect the rest.
+ *	Locking: tty_mutex protects the find_open_tty and init_dev work.
+ *		tty->count should protect the rest.
  *		allocated_ptys_lock handles the list of free pty numbers
  */
 
 static int __ptmx_open(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty;
-	int retval;
+	int retval = 0;
 	int index;
 
 	nonseekable_open(inode, filp);
@@ -2014,7 +2026,18 @@ static int __ptmx_open(struct inode *ino
 		return index;
 
 	mutex_lock(&tty_mutex);
-	retval = init_dev(ptm_driver, index, &tty, 1);
+
+	/*
+	 * TODO: We just allocated the index, will find_open_tty() ever
+	 *	 find a tty ? Keep the find for now for compatiblity with
+	 *	 old init_dev().
+	 */
+	tty = find_open_tty(ptm_driver, index);
+	if (IS_ERR(tty))
+		retval = PTR_ERR(tty);
+	else if (!tty)
+		retval = init_dev(ptm_driver, index, &tty, 1);
+
 	mutex_unlock(&tty_mutex);
 
 	if (retval)

[RFC][PATCH 3/3] Rename and change init_dev() prototype

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 3/3] Rename and change init_dev() prototype

init_dev() takes an additional '*ret_tty' parameter so it can either return
a valid 'tty_struct *' (on success) or an integer error code on failure.

Drop the '*ret_tty' and return either a tty_struct * or ERR_PTR().

Also since init_dev() now only handles the (slow) open of a _new_ tty,
rename to alloc_init_dev().

---
 drivers/char/tty_io.c |   56 ++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

Index: linux-next/drivers/char/tty_io.c
===================================================================
--- linux-next.orig/drivers/char/tty_io.c	2008-08-25 12:54:35.000000000 -0700
+++ linux-next/drivers/char/tty_io.c	2008-08-25 12:56:30.000000000 -0700
@@ -1290,7 +1290,7 @@ static int fast_tty_open(struct tty_stru
 }
 
 /**
- *	init_dev		-	initialise a tty device
+ *	alloc_init_dev		-	alloc/initialise a new tty device
  *	@driver: tty driver we are opening a device on
  *	@idx: device index
  *	@ret_tty: returned tty structure
@@ -1314,20 +1314,19 @@ static int fast_tty_open(struct tty_stru
  * relaxed for the (most common) case of reopening a tty.
  */
 
-static int init_dev(struct tty_driver *driver, int idx,
-	struct tty_struct **ret_tty, int first_ok)
+static struct tty_struct *alloc_init_dev(struct tty_driver *driver, int idx,
+	int first_ok)
 {
-	struct tty_struct *tty, *o_tty;
+	struct tty_struct *tty, *o_tty, *ret_tty;
 	struct ktermios *tp, **tp_loc, *o_tp, **o_tp_loc;
 	struct ktermios *ltp, **ltp_loc, *o_ltp, **o_ltp_loc;
-	int retval = 0;
+	int err;
 
 	/* Check if pty master is being opened multiple times */
+	ret_tty = ERR_PTR(-EIO);
 	if (driver->subtype == PTY_TYPE_MASTER &&
-		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) {
-		retval = -EIO;
+		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok)
 		goto end_init;
-	}
 	/*
 	 * First time open is complex, especially for PTY devices.
 	 * This code guarantees that either everything succeeds and the
@@ -1336,10 +1335,9 @@ static int init_dev(struct tty_driver *d
 	 * and locked termios may be retained.)
 	 */
 
-	if (!try_module_get(driver->owner)) {
-		retval = -ENODEV;
+	ret_tty = ERR_PTR(-ENODEV);
+	if (!try_module_get(driver->owner))
 		goto end_init;
-	}
 
 	o_tty = NULL;
 	tp = o_tp = NULL;
@@ -1348,6 +1346,7 @@ static int init_dev(struct tty_driver *d
 	tty = alloc_tty_struct();
 	if (!tty)
 		goto fail_no_mem;
+
 	initialize_tty_struct(tty);
 	tty->driver = driver;
 	tty->ops = driver->ops;
@@ -1458,16 +1457,15 @@ static int init_dev(struct tty_driver *d
 	 * to decrement the use counts, as release_tty doesn't care.
 	 */
 
-	retval = tty_ldisc_setup(tty, o_tty);
-
-	if (retval)
+	err = tty_ldisc_setup(tty, o_tty);
+	if (err)
 		goto release_mem_out;
 
-	*ret_tty = tty;
+	ret_tty = tty;
 
 	/* All paths come through here to release the mutex */
 end_init:
-	return retval;
+	return ret_tty;
 
 	/* Release locally allocated memory ... nothing placed in slots */
 free_mem_out:
@@ -1482,13 +1480,13 @@ free_mem_out:
 
 fail_no_mem:
 	module_put(driver->owner);
-	retval = -ENOMEM;
+	ret_tty = ERR_PTR(-ENOMEM);
 	goto end_init;
 
 	/* call the tty release_tty routine to clean out this slot */
 release_mem_out:
 	if (printk_ratelimit())
-		printk(KERN_INFO "init_dev: ldisc open failed, "
+		printk(KERN_INFO "alloc_init_dev: ldisc open failed, "
 				 "clearing slot %d\n", idx);
 	release_tty(tty, idx);
 	goto end_init;
@@ -1910,8 +1908,11 @@ got_driver:
 
 	if (tty)
 		retval = fast_tty_open(tty);
-	else
-		retval = init_dev(driver, index, &tty, 0);
+	else {
+		tty = alloc_init_dev(driver, index, 0);
+		if (IS_ERR(tty))
+			retval = PTR_ERR(tty);
+	}
 
 	mutex_unlock(&tty_mutex);
 
@@ -2007,7 +2008,7 @@ static struct tty_struct *find_open_tty(
  *
  *	Allocate a unix98 pty master device from the ptmx driver.
  *
- *	Locking: tty_mutex protects the find_open_tty and init_dev work.
+ *	Locking: tty_mutex protects the find_open_tty and alloc_init_dev work.
  *		tty->count should protect the rest.
  *		allocated_ptys_lock handles the list of free pty numbers
  */
@@ -2033,15 +2034,18 @@ static int __ptmx_open(struct inode *ino
 	 *	 old init_dev().
 	 */
 	tty = find_open_tty(ptm_driver, index);
-	if (IS_ERR(tty))
-		retval = PTR_ERR(tty);
-	else if (!tty)
-		retval = init_dev(ptm_driver, index, &tty, 1);
+
+	if (!tty)
+		tty = alloc_init_dev(ptm_driver, index, 1);
 
 	mutex_unlock(&tty_mutex);
 
-	if (retval)
+	BUG_ON(tty == NULL);	/* TODO: remove */
+
+	if (IS_ERR(tty)) {
+		retval = PTR_ERR(tty);
 		goto out;
+	}
 
 	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
 	filp->private_data = tty;

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

Alan Cox [alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org] wrote:
| On Mon, 25 Aug 2008 13:11:10 -0700
| sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
| 
| > 
| > init_dev() is doing too many things all in one function, making it
| > harder to understand/extend. This and following two patches attempt
| > to simplify/cleanup the interface a bit.
| > 
| > Only touch tested at this point. Appreciate any comments :-)
| 
| Well this code is very much in flux so I really don't want to get further
| changes mixed up into the plan at the wrong moment. That said:

Ok. Do let me know if there are pieces I can help with.

| 
| > +/*
| > + * 	find_tty() - find an existing tty, if any
| > + * 	@driver: the driver for the tty
| > + * 	@idx:	 the minor number
| 
| This fits very well with where I want to end up - which is
| 
| 
| 	tty = driver->ops->something(driver, idx);

That looks like a good idea. With multiple pts instances, we would also
need an 'instance' parameter to uniquely identify the pty. Other tty
devices could set their 'instance' to NULL.

It could be a void *, although internally if it is the pts inode, we
could optimize devpts (as Peter Anvin pointed out).

By extension, maybe the tty layer would need another interface to determine
the instance:

	instance =  driver->ops->get_instance(driver, inode, other_stuff) 

using this we find the tty

	tty = driver->ops->something(driver, instance, idx);

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[Alan Cox]

> This seems more than a bit redundant.  The "instance", IMO, *is* the tty 
> structure; so the interface should be:

Only for a re-open - which is very different to an initial open,
and /dev/tty is deep magic in this situation.

> Not "index", but "inode".  If, as a courtesy to the generic driver, we 
> want to precalculate the index number we can do that, but otherwise that 
> is of course available as:

Thats a much bigger step and raises problems later on with consoles. We
might want to end up there - but not in one leap.

Alan

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[H. Peter Anvin]

sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> 
> By extension, maybe the tty layer would need another interface to determine
> the instance:
> 
> 	instance =  driver->ops->get_instance(driver, inode, other_stuff) 
> 
> using this we find the tty
> 
> 	tty = driver->ops->something(driver, instance, idx);

This seems more than a bit redundant.  The "instance", IMO, *is* the tty 
structure; so the interface should be:

	tty = driver->ops->get_tty(driver, inode [, other_stuff?]);

Not "index", but "inode".  If, as a courtesy to the generic driver, we 
want to precalculate the index number we can do that, but otherwise that 
is of course available as:

index = inode->i_rdev - MK_DEV(device->major, device->minor_start);

... and if we replaced device->{major,minor_start} with device->base and 
made the drivers use MK_DEV() in the initializers, it would be even simpler.

	-hpa

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

H. Peter Anvin [hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org] wrote:
> sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
>> By extension, maybe the tty layer would need another interface to 
>> determine
>> the instance:
>> 	instance =  driver->ops->get_instance(driver, inode, other_stuff) using 
>> this we find the tty
>> 	tty = driver->ops->something(driver, instance, idx);
>
> This seems more than a bit redundant.  The "instance", IMO, *is* the tty 
> structure; so the interface should be:
>
> 	tty = driver->ops->get_tty(driver, inode [, other_stuff?]);


Yes, if we have the tty inode, we don't need the index or instance
parameters.  I was generalizing 

For instance in ptmx_open() we initially have only an index and the
ptmx_inode which is somewhat distinct from the tty's inode.

IOW, we use the ptmx_inode to find the 'instance' and use that
instance to build the tty inode. Of course we don't need ->get_tty()
there.

Can the inode be used to identify the driver too ?  (but inode to driver
mapping is not trivial atm).

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[H. Peter Anvin]

Alan Cox wrote:
>> This seems more than a bit redundant.  The "instance", IMO, *is* the tty 
>> structure; so the interface should be:
> 
> Only for a re-open - which is very different to an initial open,
> and /dev/tty is deep magic in this situation.

I guess I fail to understand something here, perhaps because I haven't 
looked at the code in very much details for several years.  How is there 
not a 1:1 mapping between tty structures and instances, even in the 
presence of /dev/tty?  (/dev/tty, of course, points to a real tty.)

>> Not "index", but "inode".  If, as a courtesy to the generic driver, we 
>> want to precalculate the index number we can do that, but otherwise that 
>> is of course available as:
> 
> Thats a much bigger step and raises problems later on with consoles. We
> might want to end up there - but not in one leap.

*Nod.*  It may mean that for consoles we have to provide transient 
inodes in rootfs.

	-hpa

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[H. Peter Anvin]

sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
>>
>> 	tty = driver->ops->get_tty(driver, inode [, other_stuff?]);
> 
> Can the inode be used to identify the driver too ?  (but inode to driver
> mapping is not trivial atm).

It can, but it's an O(n) operation in the number of registered drivers. 
  However, we can only call the above if we know the driver in the first 
place so such a lookup is rather pointless.

	-hpa

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

H. Peter Anvin [hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org] wrote:
> sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
>>>
>>> 	tty = driver->ops->get_tty(driver, inode [, other_stuff?]);
>> Can the inode be used to identify the driver too ?  (but inode to driver
>> mapping is not trivial atm).
>
> It can, but it's an O(n) operation in the number of registered drivers.  
> However, we can only call the above if we know the driver in the first 
> place so such a lookup is rather pointless.

Yes, we know the driver, but do we need to pass it into ->get_tty() ?

Passing it in (or having the operation compute from inode) has advantage
of allowing drivers to share code if necessary.

	common_get_tty(driver, inode)
	{
		if (is_ptmx_driver(driver))
			something;
		else // pts driver
			something_else;
	}

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[H. Peter Anvin]

sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> 
> Yes, we know the driver, but do we need to pass it into ->get_tty() ?
> 
> Passing it in (or having the operation compute from inode) has advantage
> of allowing drivers to share code if necessary.
> 

Yes, and it gets access to its own data.  It's how you implement an 
object-oriented method call in C.

	-hpa

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[Alan Cox]

> > Only for a re-open - which is very different to an initial open,
> > and /dev/tty is deep magic in this situation.
> 
> I guess I fail to understand something here, perhaps because I haven't 
> looked at the code in very much details for several years.  How is there 
> not a 1:1 mapping between tty structures and instances, even in the 
> presence of /dev/tty?  (/dev/tty, of course, points to a real tty.)

In the case of the initial open you don't yet know the tty pointer and
may be creating it. SO the tty isn't a reference because it doesn't exist.

> *Nod.*  It may mean that for consoles we have to provide transient 
> inodes in rootfs.

consoles for printk need to exist extremely early if we are going to be
able to kick most of the console special case code into the bitbucket.

Alan

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[Alan Cox]

On Mon, 25 Aug 2008 13:11:10 -0700
sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:

> 
> init_dev() is doing too many things all in one function, making it
> harder to understand/extend. This and following two patches attempt
> to simplify/cleanup the interface a bit.
> 
> Only touch tested at this point. Appreciate any comments :-)

Seems to work - merged this first chunk plus the tty->driver changes so
can you check the ttydev tree/linux-next this evening and it should be
in. If it looks ok it needs a signed off by from yourself.

Alan

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[H. Peter Anvin]

Alan Cox wrote:
> 
> In the case of the initial open you don't yet know the tty pointer and
> may be creating it. SO the tty isn't a reference because it doesn't exist.
> 

Got it.  I was under the (apparently mistaken) notion that only pty tty 
structures were created dynamically.

	-hpa

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[Alan Cox]

On Tue, 26 Aug 2008 09:40:20 -0700
"H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:

> Alan Cox wrote:
> > 
> > In the case of the initial open you don't yet know the tty pointer and
> > may be creating it. SO the tty isn't a reference because it doesn't exist.
> > 
> 
> Got it.  I was under the (apparently mistaken) notion that only pty tty 
> structures were created dynamically.

tty is dynamically created and attached to the file handle. The port side
structure is currently port specific and does last. Thats what the
tty_port stuff is intended to slowly standardise but won't help ptys as
they don't have a physical port anyway.

Alan

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[H. Peter Anvin]

Alan Cox wrote:
> On Tue, 26 Aug 2008 09:40:20 -0700
> "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Alan Cox wrote:
>>> In the case of the initial open you don't yet know the tty pointer and
>>> may be creating it. SO the tty isn't a reference because it doesn't exist.
>>>
>> Got it.  I was under the (apparently mistaken) notion that only pty tty 
>> structures were created dynamically.
> 
> tty is dynamically created and attached to the file handle. The port side
> structure is currently port specific and does last. Thats what the
> tty_port stuff is intended to slowly standardise but won't help ptys as
> they don't have a physical port anyway.
> 

Got it, sorry for the confusion.

	-hpa

Re: [RFC][PATCH 2/3] Move some init_dev() code to callers

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org [sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| 
| From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| Subject: [RFC][PATCH 2/3] Move some init_dev() code to callers
| 
| init_dev() tries to find a tty and if it finds an existing tty, does
| a 'fast' open. If its not an existing tty, init_dev does a slower
| first time open requiring allocation and complex initialization.
| 
| All these seem to make the code more complex. When opening /dev/tty,
| the caller already has the tty so there is no need to find it. Further
| the fast and slow opens in init_dev() don't really share much code
| and could be in separate functions.
| 
| With only two callers, init_dev() does not really need to be that
| generalized and some of the pieces can be moved into the callers.

Alan, should I rebase this patch on the current ttydev tree -
(i.e move tty_driver_lookup() and tty_reopen() to callers) or do you
have other plans ?

ptmx_open() may not even need call tty_driver_lookup() or reopen()
since it just allocated a new index. I did quick touch test on it.

tty_open() can skip the lookup if opening /dev/tty.

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

Alan,

Resending patch with sign-off. We may also want to update the patch
description of (in tty-init-dev-rework) in the ttydev tree.

Thanks,

---

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH] Move parts of init_dev() into new functions

Move the 'find-tty' and 'fast-track-open' parts of init_dev() to
separate functions.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 drivers/char/tty_io.c |  140 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 89 insertions(+), 51 deletions(-)

Index: linux-next/drivers/char/tty_io.c
===================================================================
--- linux-next.orig/drivers/char/tty_io.c	2008-08-25 11:57:15.000000000 -0700
+++ linux-next/drivers/char/tty_io.c	2008-08-25 12:31:15.000000000 -0700
@@ -1213,6 +1213,82 @@ static void tty_line_name(struct tty_dri
 	sprintf(p, "%s%d", driver->name, index + driver->name_base);
 }

+/*
+ * 	find_tty() - find an existing tty, if any
+ * 	@driver: the driver for the tty
+ * 	@idx:	 the minor number
+ *
+ * 	Return the tty, if found or ERR_PTR() otherwise.
+ *
+ * 	Locking: tty_mutex must be held. If tty is found, the mutex must
+ * 		 be held until the 'fast-open' is also done.
+ */
+struct tty_struct *find_tty(struct tty_driver *driver, int idx)
+{
+	struct tty_struct *tty;
+
+	/* check whether we're reopening an existing tty */
+	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
+		tty = devpts_get_tty(idx);
+		/*
+		 * If we don't have a tty here on a slave open, it's because
+		 * the master already started the close process and there's
+		 * no relation between devpts file and tty anymore.
+		 */
+		if (!tty && driver->subtype == PTY_TYPE_SLAVE)
+			return ERR_PTR(-EIO);
+
+		/*
+		 * tty is safe on because we are called with tty_mutex held
+		 * and release_dev() won't change tty->count or tty->flags
+		 * without grabbing tty_mutex.
+		 */
+		if (tty && driver->subtype == PTY_TYPE_MASTER)
+			tty = tty->link;
+	} else {
+		tty = driver->ttys[idx];
+	}
+
+	return tty;
+}
+
+/*
+ * 	fast_tty_open()	- fast re-open of an open tty
+ * 	@tty	- the tty to open
+ *
+ * 	Return 0 on success, -errno on error.
+ *
+ * 	Locking: tty_mutex must be held from the time the tty was found
+ * 		 till this open completes.
+ */
+static int fast_tty_open(struct tty_struct *tty)
+{
+	struct tty_driver *driver = tty->driver;
+
+	if (test_bit(TTY_CLOSING, &tty->flags))
+		return -EIO;
+
+	if (driver->type == TTY_DRIVER_TYPE_PTY &&
+	    driver->subtype == PTY_TYPE_MASTER) {
+		/*
+		 * special case for PTY masters: only one open permitted,
+		 * and the slave side open count is incremented as well.
+		 */
+		if (tty->count)
+			return -EIO;
+
+		tty->link->count++;
+	}
+	tty->count++;
+	tty->driver = driver; /* N.B. why do this every time?? */
+
+	/* FIXME */
+	if (!test_bit(TTY_LDISC, &tty->flags))
+		printk(KERN_ERR "fast_tty_open: no ldisc\n");
+
+	return 0;
+}
+
 /**
  *	init_dev		-	initialise a tty device
  *	@driver: tty driver we are opening a device on
@@ -1246,30 +1322,21 @@ static int init_dev(struct tty_driver *d
 	struct ktermios *ltp, **ltp_loc, *o_ltp, **o_ltp_loc;
 	int retval = 0;

-	/* check whether we're reopening an existing tty */
-	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
-		tty = devpts_get_tty(idx);
-		/*
-		 * If we don't have a tty here on a slave open, it's because
-		 * the master already started the close process and there's
-		 * no relation between devpts file and tty anymore.
-		 */
-		if (!tty && driver->subtype == PTY_TYPE_SLAVE) {
-			retval = -EIO;
-			goto end_init;
-		}
-		/*
-		 * It's safe from now on because init_dev() is called with
-		 * tty_mutex held and release_dev() won't change tty->count
-		 * or tty->flags without having to grab tty_mutex
-		 */
-		if (tty && driver->subtype == PTY_TYPE_MASTER)
-			tty = tty->link;
-	} else {
-		tty = driver->ttys[idx];
+	tty = find_tty(driver, idx);
+	if (IS_ERR(tty)) {
+		retval = PTR_ERR(tty);
+		goto end_init;
 	}
-	if (tty) goto fast_track;

+	if (tty) {
+		retval = fast_tty_open(tty);
+		if (retval)
+			return retval;
+		*ret_tty = tty;
+		return 0;
+	}
+
+	/* Check if pty master is being opened multiple times */
 	if (driver->subtype == PTY_TYPE_MASTER &&
 		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) {
 		retval = -EIO;
@@ -1411,35 +1478,6 @@ static int init_dev(struct tty_driver *d
 		goto release_mem_out;
 	goto success;

-	/*
-	 * This fast open can be used if the tty is already open.
-	 * No memory is allocated, and the only failures are from
-	 * attempting to open a closing tty or attempting multiple
-	 * opens on a pty master.
-	 */
-fast_track:
-	if (test_bit(TTY_CLOSING, &tty->flags)) {
-		retval = -EIO;
-		goto end_init;
-	}
-	if (driver->type == TTY_DRIVER_TYPE_PTY &&
-	    driver->subtype == PTY_TYPE_MASTER) {
-		/*
-		 * special case for PTY masters: only one open permitted,
-		 * and the slave side open count is incremented as well.
-		 */
-		if (tty->count) {
-			retval = -EIO;
-			goto end_init;
-		}
-		tty->link->count++;
-	}
-	tty->count++;
-	tty->driver = driver; /* N.B. why do this every time?? */
-
-	/* FIXME */
-	if (!test_bit(TTY_LDISC, &tty->flags))
-		printk(KERN_ERR "init_dev but no ldisc\n");
 success:
 	*ret_tty = tty;

Re: [RFC][PATCH 1/3] Move parts of init_dev() into new functions

[Alan Cox]

On Thu, 28 Aug 2008 13:25:20 -0700
sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:

> Alan,
> 
> Resending patch with sign-off. We may also want to update the patch
> description of (in tty-init-dev-rework) in the ttydev tree.

Thanks - will do that tomorrow.

I've pushed some further bits to try and get all of the pty special cases
out of the tty_io code - it isn't quite there yet but it's looking a lot
cleaner now.

Alan