ttyprintk driver calls tty_unregister_driver() wrongly in the error
path of tty_register_driver(). Also, setting ttyprintk_driver to NULL
is utterly superfluous, so let's get rid of it, too.
Reported-by: Jean Delvare <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/char/ttyprintk.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index daea84c41743..2a39c5790364 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -210,10 +210,8 @@ static int __init ttyprintk_init(void)
return 0;
error:
- tty_unregister_driver(ttyprintk_driver);
put_tty_driver(ttyprintk_driver);
tty_port_destroy(&tpk_port.port);
- ttyprintk_driver = NULL;
return ret;
}
device_initcall(ttyprintk_init);
--
1.9.1
The driver is well written to be used as a module, just the exit call
is missing.
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/char/Kconfig | 2 +-
drivers/char/ttyprintk.c | 13 ++++++++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 1386749b48ff..97816b133c7f 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -40,7 +40,7 @@ config SGI_MBCS
source "drivers/tty/serial/Kconfig"
config TTY_PRINTK
- bool "TTY driver to output user messages via printk"
+ tristate "TTY driver to output user messages via printk"
depends on EXPERT && TTY
default n
---help---
diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index 2a39c5790364..73606eaaba71 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -17,7 +17,7 @@
#include <linux/device.h>
#include <linux/serial.h>
#include <linux/tty.h>
-#include <linux/export.h>
+#include <linux/module.h>
struct ttyprintk_port {
struct tty_port port;
@@ -214,4 +214,15 @@ error:
tty_port_destroy(&tpk_port.port);
return ret;
}
+
+static void ttyprintk_exit(void)
+{
+ tty_unregister_driver(ttyprintk_driver);
+ put_tty_driver(ttyprintk_driver);
+ tty_port_destroy(&tpk_port.port);
+}
+
device_initcall(ttyprintk_init);
+module_exit(ttyprintk_exit);
+
+MODULE_LICENSE("GPL");
--
1.9.1
Le Wednesday 02 April 2014 à 12:29 +0200, Takashi Iwai a écrit :
> ttyprintk driver calls tty_unregister_driver() wrongly in the error
> path of tty_register_driver(). Also, setting ttyprintk_driver to NULL
> is utterly superfluous, so let's get rid of it, too.
>
> Reported-by: Jean Delvare <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> drivers/char/ttyprintk.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index daea84c41743..2a39c5790364 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -210,10 +210,8 @@ static int __init ttyprintk_init(void)
> return 0;
>
> error:
> - tty_unregister_driver(ttyprintk_driver);
> put_tty_driver(ttyprintk_driver);
> tty_port_destroy(&tpk_port.port);
> - ttyprintk_driver = NULL;
> return ret;
> }
> device_initcall(ttyprintk_init);
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
SUSE L3 Support
Hi Takashi,
Le Wednesday 02 April 2014 à 12:29 +0200, Takashi Iwai a écrit :
> The driver is well written to be used as a module, just the exit call
> is missing.
>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> drivers/char/Kconfig | 2 +-
> drivers/char/ttyprintk.c | 13 ++++++++++++-
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 1386749b48ff..97816b133c7f 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -40,7 +40,7 @@ config SGI_MBCS
> source "drivers/tty/serial/Kconfig"
>
> config TTY_PRINTK
> - bool "TTY driver to output user messages via printk"
> + tristate "TTY driver to output user messages via printk"
> depends on EXPERT && TTY
> default n
> ---help---
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index 2a39c5790364..73606eaaba71 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -17,7 +17,7 @@
> #include <linux/device.h>
> #include <linux/serial.h>
> #include <linux/tty.h>
> -#include <linux/export.h>
> +#include <linux/module.h>
>
> struct ttyprintk_port {
> struct tty_port port;
> @@ -214,4 +214,15 @@ error:
> tty_port_destroy(&tpk_port.port);
> return ret;
> }
> +
> +static void ttyprintk_exit(void)
Could be marked __exit.
> +{
> + tty_unregister_driver(ttyprintk_driver);
> + put_tty_driver(ttyprintk_driver);
> + tty_port_destroy(&tpk_port.port);
> +}
> +
> device_initcall(ttyprintk_init);
> +module_exit(ttyprintk_exit);
> +
> +MODULE_LICENSE("GPL");
Other than this, this looks good, thanks for doing that.
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
SUSE L3 Support
At Wed, 02 Apr 2014 14:31:46 +0200,
Jean Delvare wrote:
>
> Hi Takashi,
>
> Le Wednesday 02 April 2014 à 12:29 +0200, Takashi Iwai a écrit :
> > The driver is well written to be used as a module, just the exit call
> > is missing.
> >
> > Signed-off-by: Takashi Iwai <[email protected]>
> > ---
> > drivers/char/Kconfig | 2 +-
> > drivers/char/ttyprintk.c | 13 ++++++++++++-
> > 2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> > index 1386749b48ff..97816b133c7f 100644
> > --- a/drivers/char/Kconfig
> > +++ b/drivers/char/Kconfig
> > @@ -40,7 +40,7 @@ config SGI_MBCS
> > source "drivers/tty/serial/Kconfig"
> >
> > config TTY_PRINTK
> > - bool "TTY driver to output user messages via printk"
> > + tristate "TTY driver to output user messages via printk"
> > depends on EXPERT && TTY
> > default n
> > ---help---
> > diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> > index 2a39c5790364..73606eaaba71 100644
> > --- a/drivers/char/ttyprintk.c
> > +++ b/drivers/char/ttyprintk.c
> > @@ -17,7 +17,7 @@
> > #include <linux/device.h>
> > #include <linux/serial.h>
> > #include <linux/tty.h>
> > -#include <linux/export.h>
> > +#include <linux/module.h>
> >
> > struct ttyprintk_port {
> > struct tty_port port;
> > @@ -214,4 +214,15 @@ error:
> > tty_port_destroy(&tpk_port.port);
> > return ret;
> > }
> > +
> > +static void ttyprintk_exit(void)
>
> Could be marked __exit.
>
> > +{
> > + tty_unregister_driver(ttyprintk_driver);
> > + put_tty_driver(ttyprintk_driver);
> > + tty_port_destroy(&tpk_port.port);
> > +}
> > +
> > device_initcall(ttyprintk_init);
> > +module_exit(ttyprintk_exit);
> > +
> > +MODULE_LICENSE("GPL");
>
> Other than this, this looks good, thanks for doing that.
>
> Reviewed-by: Jean Delvare <[email protected]>
Thanks! I'll resubmit the fixed patch.
Takashi
On Wed, Apr 02, 2014 at 12:29:42PM +0200, Takashi Iwai wrote:
> The driver is well written to be used as a module, just the exit call
> is missing.
>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> drivers/char/Kconfig | 2 +-
> drivers/char/ttyprintk.c | 13 ++++++++++++-
> 2 files changed, 13 insertions(+), 2 deletions(-)
There was some reason we didn't do this back when the code was accepted,
but I really can't remember why.
If you have tested this out, I don't have any objections to taking these
patches, thanks.
greg k-h
At Wed, 02 Apr 2014 14:24:33 +0200,
Jean Delvare wrote:
>
> Le Wednesday 02 April 2014 à 12:29 +0200, Takashi Iwai a écrit :
> > ttyprintk driver calls tty_unregister_driver() wrongly in the error
> > path of tty_register_driver(). Also, setting ttyprintk_driver to NULL
> > is utterly superfluous, so let's get rid of it, too.
> >
> > Reported-by: Jean Delvare <[email protected]>
> > Signed-off-by: Takashi Iwai <[email protected]>
> > ---
> > drivers/char/ttyprintk.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> > index daea84c41743..2a39c5790364 100644
> > --- a/drivers/char/ttyprintk.c
> > +++ b/drivers/char/ttyprintk.c
> > @@ -210,10 +210,8 @@ static int __init ttyprintk_init(void)
> > return 0;
> >
> > error:
> > - tty_unregister_driver(ttyprintk_driver);
> > put_tty_driver(ttyprintk_driver);
> > tty_port_destroy(&tpk_port.port);
> > - ttyprintk_driver = NULL;
> > return ret;
> > }
> > device_initcall(ttyprintk_init);
>
> Reviewed-by: Jean Delvare <[email protected]>
Meanwhile, I found that tty_unregister_driver() was added
intentionally in the commit f06fb543c1d0,
TTY: ttyprintk, unregister tty driver on failure
When the tty_printk driver fails to create a node in sysfs, the
system
crashes. It is because the driver registers a tty driver and frees
it
without deregistering it first. The fix is easy: add a call to
tty_unregister_driver to the fail path.
But, I failed to see why this is needed in the current code.
Jiri, is your fix still valid at all?
thanks,
Takashi
At Wed, 2 Apr 2014 12:32:22 -0700,
Greg Kroah-Hartman wrote:
>
> On Wed, Apr 02, 2014 at 12:29:42PM +0200, Takashi Iwai wrote:
> > The driver is well written to be used as a module, just the exit call
> > is missing.
> >
> > Signed-off-by: Takashi Iwai <[email protected]>
> > ---
> > drivers/char/Kconfig | 2 +-
> > drivers/char/ttyprintk.c | 13 ++++++++++++-
> > 2 files changed, 13 insertions(+), 2 deletions(-)
>
> There was some reason we didn't do this back when the code was accepted,
> but I really can't remember why.
>
> If you have tested this out, I don't have any objections to taking these
> patches, thanks.
The module worked fine with my quick tests (load/unload/feed something
via /dev/ttyprintk), at least. But it'd be helpful if anyone
remembers and clarifies.
thanks,
Takashi
On 04/03/2014 08:10 AM, Takashi Iwai wrote:
> At Wed, 02 Apr 2014 14:24:33 +0200,
> Jean Delvare wrote:
>>
>> Le Wednesday 02 April 2014 à 12:29 +0200, Takashi Iwai a écrit :
>>> ttyprintk driver calls tty_unregister_driver() wrongly in the error
>>> path of tty_register_driver(). Also, setting ttyprintk_driver to NULL
>>> is utterly superfluous, so let's get rid of it, too.
>>>
>>> Reported-by: Jean Delvare <[email protected]>
>>> Signed-off-by: Takashi Iwai <[email protected]>
>>> ---
>>> drivers/char/ttyprintk.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
>>> index daea84c41743..2a39c5790364 100644
>>> --- a/drivers/char/ttyprintk.c
>>> +++ b/drivers/char/ttyprintk.c
>>> @@ -210,10 +210,8 @@ static int __init ttyprintk_init(void)
>>> return 0;
>>>
>>> error:
>>> - tty_unregister_driver(ttyprintk_driver);
>>> put_tty_driver(ttyprintk_driver);
>>> tty_port_destroy(&tpk_port.port);
>>> - ttyprintk_driver = NULL;
>>> return ret;
>>> }
>>> device_initcall(ttyprintk_init);
>>
>> Reviewed-by: Jean Delvare <[email protected]>
>
> Meanwhile, I found that tty_unregister_driver() was added
> intentionally in the commit f06fb543c1d0,
> TTY: ttyprintk, unregister tty driver on failure
>
> When the tty_printk driver fails to create a node in sysfs, the
> system
> crashes. It is because the driver registers a tty driver and frees
> it
> without deregistering it first. The fix is easy: add a call to
> tty_unregister_driver to the fail path.
>
> But, I failed to see why this is needed in the current code.
>
> Jiri, is your fix still valid at all?
The code was different. There was also device_create after
tty_register_driver. I must admit that I don't know why I didn't change
'goto error' in the tty_register_driver fail path.
ret = tty_register_driver(ttyprintk_driver);
if (ret < 0) {
printk(KERN_ERR "Couldn't register ttyprintk driver\n");
goto error;
}
/* create our unnumbered device */
rp = device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, 3), NULL,
ttyprintk_driver->name);
if (IS_ERR(rp)) {
printk(KERN_ERR "Couldn't create ttyprintk device\n");
ret = PTR_ERR(rp);
goto error;
}
So yes, there should be no tty_unregister_driver in the fail path when
device_create is gone now.
thanks,
--
js
suse labs
Dne 03.04.2014 (čet) ob 08:13 +0200 je Takashi Iwai napisal(a):
> At Wed, 2 Apr 2014 12:32:22 -0700,
> Greg Kroah-Hartman wrote:
> >
> > On Wed, Apr 02, 2014 at 12:29:42PM +0200, Takashi Iwai wrote:
> > > The driver is well written to be used as a module, just the exit call
> > > is missing.
> > >
> > > Signed-off-by: Takashi Iwai <[email protected]>
> > > ---
> > > drivers/char/Kconfig | 2 +-
> > > drivers/char/ttyprintk.c | 13 ++++++++++++-
> > > 2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > There was some reason we didn't do this back when the code was accepted,
> > but I really can't remember why.
> >
> > If you have tested this out, I don't have any objections to taking these
> > patches, thanks.
>
> The module worked fine with my quick tests (load/unload/feed something
> via /dev/ttyprintk), at least. But it'd be helpful if anyone
> remembers and clarifies.
>
My initial requirement was to be able to use this functionality as soon
as possible at the beginning of user-space initialisation, so i never
thought of the ttyprintk code being loaded afterwards as module. Also
nobody brought modularisation subject to the table during code
acceptance.
>
> thanks,
>
> Takashi
regards, Samo