* [PATCH 2/6] TTY: unify pty_install fail path handling
2011-03-23 9:48 [PATCH 1/6] TTY: unify tty_init_dev fail path handling Jiri Slaby
@ 2011-03-23 9:48 ` Jiri Slaby
2011-03-23 9:48 ` [PATCH 3/6] TTY: unify pty_unix98_install " Jiri Slaby
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-03-23 9:48 UTC (permalink / raw)
To: gregkh; +Cc: jirislaby, linux-kernel, Jiri Slaby, Alan Cox, Julian Anastasov
Change it so that we call the deinit functions at one place at the end
of the function (by gotos). And while at it use some sane label names.
This is a preparation for the deinitialization of tty in the next
patch.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Julian Anastasov <ja@ssi.bg>
---
drivers/tty/pty.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 2107747..f511918 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -295,8 +295,8 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
return -ENOMEM;
if (!try_module_get(driver->other->owner)) {
/* This cannot in fact currently happen */
- free_tty_struct(o_tty);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto err_free_tty;
}
initialize_tty_struct(o_tty, driver->other, idx);
@@ -304,13 +304,11 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
the easy way .. */
retval = tty_init_termios(tty);
if (retval)
- goto free_mem_out;
+ goto err_module_put;
retval = tty_init_termios(o_tty);
- if (retval) {
- tty_free_termios(tty);
- goto free_mem_out;
- }
+ if (retval)
+ goto err_free_termios;
/*
* Everything allocated ... set up the o_tty structure.
@@ -327,10 +325,13 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
tty->count++;
driver->ttys[idx] = tty;
return 0;
-free_mem_out:
+err_free_termios:
+ tty_free_termios(tty);
+err_module_put:
module_put(o_tty->driver->owner);
+err_free_tty:
free_tty_struct(o_tty);
- return -ENOMEM;
+ return retval;
}
static int pty_bsd_ioctl(struct tty_struct *tty,
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/6] TTY: unify pty_unix98_install fail path handling
2011-03-23 9:48 [PATCH 1/6] TTY: unify tty_init_dev fail path handling Jiri Slaby
2011-03-23 9:48 ` [PATCH 2/6] TTY: unify pty_install " Jiri Slaby
@ 2011-03-23 9:48 ` Jiri Slaby
2011-03-23 9:48 ` [PATCH 4/6] TTY: introduce deinit helpers for proper ldisc shutdown Jiri Slaby
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-03-23 9:48 UTC (permalink / raw)
To: gregkh; +Cc: jirislaby, linux-kernel, Jiri Slaby, Alan Cox, Julian Anastasov
Change it so that we call the deinit functions at one place at the end
of the function (by gotos). And while at it use some sane label names.
This is a preparation for the deinitialization of tty in the next
patch.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Julian Anastasov <ja@ssi.bg>
---
drivers/tty/pty.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index f511918..b25d6c4 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -560,20 +560,19 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
return -ENOMEM;
if (!try_module_get(driver->other->owner)) {
/* This cannot in fact currently happen */
- free_tty_struct(o_tty);
- return -ENOMEM;
+ goto err_free_tty;
}
initialize_tty_struct(o_tty, driver->other, idx);
tty->termios = kzalloc(sizeof(struct ktermios[2]), GFP_KERNEL);
if (tty->termios == NULL)
- goto free_mem_out;
+ goto err_free_mem;
*tty->termios = driver->init_termios;
tty->termios_locked = tty->termios + 1;
o_tty->termios = kzalloc(sizeof(struct ktermios[2]), GFP_KERNEL);
if (o_tty->termios == NULL)
- goto free_mem_out;
+ goto err_free_mem;
*o_tty->termios = driver->other->init_termios;
o_tty->termios_locked = o_tty->termios + 1;
@@ -592,11 +591,12 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
tty->count++;
pty_count++;
return 0;
-free_mem_out:
+err_free_mem:
kfree(o_tty->termios);
+ kfree(tty->termios);
module_put(o_tty->driver->owner);
+err_free_tty:
free_tty_struct(o_tty);
- kfree(tty->termios);
return -ENOMEM;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/6] TTY: introduce deinit helpers for proper ldisc shutdown
2011-03-23 9:48 [PATCH 1/6] TTY: unify tty_init_dev fail path handling Jiri Slaby
2011-03-23 9:48 ` [PATCH 2/6] TTY: unify pty_install " Jiri Slaby
2011-03-23 9:48 ` [PATCH 3/6] TTY: unify pty_unix98_install " Jiri Slaby
@ 2011-03-23 9:48 ` Jiri Slaby
2011-03-23 9:48 ` [PATCH 5/6] TTY: plug in deinitialize_tty_struct Jiri Slaby
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-03-23 9:48 UTC (permalink / raw)
To: gregkh; +Cc: jirislaby, linux-kernel, Jiri Slaby, Alan Cox, Julian Anastasov
Introduce deinitialize_tty_struct which should be called after
initialize_tty_struct and before successfull tty_ldisc_setup.
It calls tty_ldisc_deinit which is opposite of tty_ldisc_init. It only
puts a reference to ldisc and assigns NULL to tty->ldisc.
It will be used to shut down ldisc when tty_release cannot be called
yet.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Julian Anastasov <ja@ssi.bg>
---
drivers/tty/tty_io.c | 14 ++++++++++++++
drivers/tty/tty_ldisc.c | 13 +++++++++++++
include/linux/tty.h | 2 ++
3 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6312cc3..a4d47fa 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2889,6 +2889,20 @@ void initialize_tty_struct(struct tty_struct *tty,
}
/**
+ * deinitialize_tty_struct
+ * @tty: tty to deinitialize
+ *
+ * This subroutine deinitializes a tty structure that has been newly
+ * allocated but tty_release cannot be called on that yet.
+ *
+ * Locking: none - tty in question must not be exposed at this point
+ */
+void deinitialize_tty_struct(struct tty_struct *tty)
+{
+ tty_ldisc_deinit(tty);
+}
+
+/**
* tty_put_char - write one character to a tty
* @tty: tty
* @ch: character
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 0fc564a..525a901 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -956,6 +956,19 @@ void tty_ldisc_init(struct tty_struct *tty)
tty_ldisc_assign(tty, ld);
}
+/**
+ * tty_ldisc_init - ldisc cleanup for new tty
+ * @tty: tty that was allocated recently
+ *
+ * The tty structure must not becompletely set up (tty_ldisc_setup) when
+ * this call is made.
+ */
+void tty_ldisc_deinit(struct tty_struct *tty)
+{
+ put_ldisc(tty->ldisc);
+ tty_ldisc_assign(tty, NULL);
+}
+
void tty_ldisc_begin(void)
{
/* Setup the default TTY line discipline. */
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 4e53d46..a39e2b1 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -472,6 +472,7 @@ extern int tty_add_file(struct tty_struct *tty, struct file *file);
extern void free_tty_struct(struct tty_struct *tty);
extern void initialize_tty_struct(struct tty_struct *tty,
struct tty_driver *driver, int idx);
+extern void deinitialize_tty_struct(struct tty_struct *tty);
extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
int first_ok);
extern int tty_release(struct inode *inode, struct file *filp);
@@ -525,6 +526,7 @@ extern int tty_set_ldisc(struct tty_struct *tty, int ldisc);
extern int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty);
extern void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty);
extern void tty_ldisc_init(struct tty_struct *tty);
+extern void tty_ldisc_deinit(struct tty_struct *tty);
extern void tty_ldisc_begin(void);
/* This last one is just for the tty layer internals and shouldn't be used elsewhere */
extern void tty_ldisc_enable(struct tty_struct *tty);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/6] TTY: plug in deinitialize_tty_struct
2011-03-23 9:48 [PATCH 1/6] TTY: unify tty_init_dev fail path handling Jiri Slaby
` (2 preceding siblings ...)
2011-03-23 9:48 ` [PATCH 4/6] TTY: introduce deinit helpers for proper ldisc shutdown Jiri Slaby
@ 2011-03-23 9:48 ` Jiri Slaby
2011-03-23 9:48 ` [PATCH 6/6] TTY: fix fail path in tty_open Jiri Slaby
2011-03-23 20:53 ` [PATCH 1/6] TTY: unify tty_init_dev fail path handling Julian Anastasov
5 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-03-23 9:48 UTC (permalink / raw)
To: gregkh; +Cc: jirislaby, linux-kernel, Jiri Slaby, Alan Cox
Used the newly introduced deinitialize_tty_struct to properly shut
down ldisc.
It is intended to fix the Julian's reported problem. He reports that
kmemleak checker warns about memory leak:
unreferenced object 0xc0e19860 (size 8):
comm cat, pid 1226, jiffies 4294919464 (age 287.476s)
hex dump (first 8 bytes):
44 de 2d c1 01 00 00 00 D.-.....
backtrace:
[<c1065a74>] create_object+0x109/0x1ad
[<c1063d2b>] kmem_cache_alloc+0x60/0x68
[<c113505c>] tty_ldisc_get+0x54/0x76
[<c11358c9>] tty_ldisc_init+0xa/0x20
[<c1130ab4>] initialize_tty_struct+0x2d/0x1ac
[<c1130c8c>] tty_init_dev+0x59/0x10d
[<c113136d>] tty_open+0x24a/0x3a2
...
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Reported-by: Julian Anastasov <ja@ssi.bg>
---
drivers/tty/pty.c | 6 ++++--
drivers/tty/tty_io.c | 5 +++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index b25d6c4..21bddf3 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -304,7 +304,7 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
the easy way .. */
retval = tty_init_termios(tty);
if (retval)
- goto err_module_put;
+ goto err_deinit_tty;
retval = tty_init_termios(o_tty);
if (retval)
@@ -327,7 +327,8 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
return 0;
err_free_termios:
tty_free_termios(tty);
-err_module_put:
+err_deinit_tty:
+ deinitialize_tty_struct(o_tty);
module_put(o_tty->driver->owner);
err_free_tty:
free_tty_struct(o_tty);
@@ -592,6 +593,7 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
pty_count++;
return 0;
err_free_mem:
+ deinitialize_tty_struct(o_tty);
kfree(o_tty->termios);
kfree(tty->termios);
module_put(o_tty->driver->owner);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a4d47fa..48eb1aa 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1399,7 +1399,7 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
retval = tty_driver_install_tty(driver, tty);
if (retval < 0)
- goto err_free_tty;
+ goto err_deinit_tty;
/*
* Structures all installed ... call the ldisc open routines.
@@ -1411,7 +1411,8 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
goto err_release_tty;
return tty;
-err_free_tty:
+err_deinit_tty:
+ deinitialize_tty_struct(tty);
free_tty_struct(tty);
err_module_put:
module_put(driver->owner);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 6/6] TTY: fix fail path in tty_open
2011-03-23 9:48 [PATCH 1/6] TTY: unify tty_init_dev fail path handling Jiri Slaby
` (3 preceding siblings ...)
2011-03-23 9:48 ` [PATCH 5/6] TTY: plug in deinitialize_tty_struct Jiri Slaby
@ 2011-03-23 9:48 ` Jiri Slaby
2011-03-23 20:53 ` [PATCH 1/6] TTY: unify tty_init_dev fail path handling Julian Anastasov
5 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-03-23 9:48 UTC (permalink / raw)
To: gregkh; +Cc: jirislaby, linux-kernel, Jiri Slaby, Alan Cox
When tty_add_file fails we omit to clean up. Fix that by calling
tty_release appropriatelly.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
drivers/tty/tty_io.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 48eb1aa..2ec3df9 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1894,6 +1894,7 @@ got_driver:
retval = tty_add_file(tty, filp);
if (retval) {
tty_unlock();
+ tty_release(inode, filp);
return retval;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/6] TTY: unify tty_init_dev fail path handling
2011-03-23 9:48 [PATCH 1/6] TTY: unify tty_init_dev fail path handling Jiri Slaby
` (4 preceding siblings ...)
2011-03-23 9:48 ` [PATCH 6/6] TTY: fix fail path in tty_open Jiri Slaby
@ 2011-03-23 20:53 ` Julian Anastasov
2011-03-29 12:17 ` Jiri Slaby
5 siblings, 1 reply; 9+ messages in thread
From: Julian Anastasov @ 2011-03-23 20:53 UTC (permalink / raw)
To: Jiri Slaby; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox
Hello,
On Wed, 23 Mar 2011, Jiri Slaby wrote:
> Change it so that we call the deinit functions at one place at the end
> of the function (by gotos). And while at it use some sane label names.
>
> This is a preparation for the deinitialization of tty in the next
> patch.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Julian Anastasov <ja@ssi.bg>
> ---
>
> This is not tested yet, I'm building a kernel to test it a bit, but it
> will take some time. If you could too to check if your problem
> dissapears, it would be great.
Thanks! Patches 1-5 work for me, I backported them
to 2.6.33.8 for the test.
>
> P.S. the drivers/char/tty_<TAB><TAB> habit sucks :).
> ^^^^
>
> drivers/tty/tty_io.c | 23 ++++++++++++-----------
> 1 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 936a4ea..6312cc3 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1391,16 +1391,15 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
> return ERR_PTR(-ENODEV);
>
> tty = alloc_tty_struct();
> - if (!tty)
> - goto fail_no_mem;
> + if (!tty) {
> + retval = -ENOMEM;
> + goto err_module_put;
> + }
> initialize_tty_struct(tty, driver, idx);
>
> retval = tty_driver_install_tty(driver, tty);
> - if (retval < 0) {
> - free_tty_struct(tty);
> - module_put(driver->owner);
> - return ERR_PTR(retval);
> - }
> + if (retval < 0)
> + goto err_free_tty;
>
> /*
> * Structures all installed ... call the ldisc open routines.
> @@ -1409,15 +1408,17 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
> */
> retval = tty_ldisc_setup(tty, tty->link);
> if (retval)
> - goto release_mem_out;
> + goto err_release_tty;
> return tty;
>
> -fail_no_mem:
> +err_free_tty:
> + free_tty_struct(tty);
> +err_module_put:
> module_put(driver->owner);
> - return ERR_PTR(-ENOMEM);
> + return ERR_PTR(retval);
>
> /* call the tty release_tty routine to clean out this slot */
> -release_mem_out:
> +err_release_tty:
> if (printk_ratelimit())
> printk(KERN_INFO "tty_init_dev: ldisc open failed, "
> "clearing slot %d\n", idx);
> --
> 1.7.4.1
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/6] TTY: unify tty_init_dev fail path handling
2011-03-23 20:53 ` [PATCH 1/6] TTY: unify tty_init_dev fail path handling Julian Anastasov
@ 2011-03-29 12:17 ` Jiri Slaby
2011-03-29 13:12 ` Greg KH
0 siblings, 1 reply; 9+ messages in thread
From: Jiri Slaby @ 2011-03-29 12:17 UTC (permalink / raw)
To: Julian Anastasov; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox
On 03/23/2011 09:53 PM, Julian Anastasov wrote:
>
> Hello,
>
> On Wed, 23 Mar 2011, Jiri Slaby wrote:
>
>> Change it so that we call the deinit functions at one place at the end
>> of the function (by gotos). And while at it use some sane label names.
>>
>> This is a preparation for the deinitialization of tty in the next
>> patch.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Cc: Greg Kroah-Hartman <gregkh@suse.de>
>> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
>> Cc: Julian Anastasov <ja@ssi.bg>
>> ---
>>
>> This is not tested yet, I'm building a kernel to test it a bit, but it
>> will take some time. If you could too to check if your problem
>> dissapears, it would be great.
>
> Thanks! Patches 1-5 work for me, I backported them
> to 2.6.33.8 for the test.
Thanks for testing. I see no regressions either.
Greg, will you take it?
regards,
--
js
suse labs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] TTY: unify tty_init_dev fail path handling
2011-03-29 12:17 ` Jiri Slaby
@ 2011-03-29 13:12 ` Greg KH
0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2011-03-29 13:12 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Julian Anastasov, jirislaby, linux-kernel, Alan Cox
On Tue, Mar 29, 2011 at 02:17:42PM +0200, Jiri Slaby wrote:
> On 03/23/2011 09:53 PM, Julian Anastasov wrote:
> >
> > Hello,
> >
> > On Wed, 23 Mar 2011, Jiri Slaby wrote:
> >
> >> Change it so that we call the deinit functions at one place at the end
> >> of the function (by gotos). And while at it use some sane label names.
> >>
> >> This is a preparation for the deinitialization of tty in the next
> >> patch.
> >>
> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> >> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> >> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> >> Cc: Julian Anastasov <ja@ssi.bg>
> >> ---
> >>
> >> This is not tested yet, I'm building a kernel to test it a bit, but it
> >> will take some time. If you could too to check if your problem
> >> dissapears, it would be great.
> >
> > Thanks! Patches 1-5 work for me, I backported them
> > to 2.6.33.8 for the test.
>
> Thanks for testing. I see no regressions either.
>
> Greg, will you take it?
Yes I will queue it up in a week or so, thanks.
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread