Introduce tty_unregister_ldisc()
Subsystems (3)
PATCH 1/2 Introduce tty_unregister_ldisc()
It's a bit strange to see tty_register_ldisc() call in modules' exit functions.
---
Documentation/tty.txt | 2 +- drivers/char/tty_io.c | 35 ++++++++++++++++++++++++----------- include/linux/tty.h | 1 + 3 files changed, 26 insertions(+), 12 deletions(-)
Documentation/tty.txt +1 -1
@@ -22,7 +22,7 @@ copy of the structure. You must not re-r discipline even with the same data or your computer again will be eaten by demons. -In order to remove a line discipline call tty_register_ldisc passing NULL. +In order to remove a line discipline call tty_unregister_ldisc(). In ancient times this always worked. In modern times the function will return -EBUSY if the ldisc is currently in use. Since the ldisc referencing code manages the module counts this should not usually be a concern.
drivers/char/tty_io.c +24 -11
@@ -262,17 +262,10 @@ int tty_register_ldisc(int disc, structreturn -EINVAL ;spin_lock_irqsave (&tty_ldisc_lock, flags); -if (new_ldisc) { - tty_ldiscs[disc] = *new_ldisc; - tty_ldiscs[disc].num = disc; - tty_ldiscs[disc].flags |=LDISC_FLAG_DEFINED ; - tty_ldiscs[disc].refcount =0 ; - }else { -if (tty_ldiscs[disc].refcount) - ret = -EBUSY ; -else - tty_ldiscs[disc].flags &= ~LDISC_FLAG_DEFINED ; - } + tty_ldiscs[disc] = *new_ldisc; + tty_ldiscs[disc].num = disc; + tty_ldiscs[disc].flags |=LDISC_FLAG_DEFINED ; + tty_ldiscs[disc].refcount =0 ;spin_unlock_irqrestore (&tty_ldisc_lock, flags);return ret;
@@ -280,6 +273,26 @@ int tty_register_ldisc(int disc, structEXPORT_SYMBOL (tty_register_ldisc); +int tty_unregister_ldisc (int disc) +{ +unsigned long flags; +int ret =0 ; + +if (disc <N_TTY || disc >=NR_LDISCS ) +return -EINVAL ; + +spin_lock_irqsave (&tty_ldisc_lock, flags); +if (tty_ldiscs[disc].refcount) + ret = -EBUSY ; +else + tty_ldiscs[disc].flags &= ~LDISC_FLAG_DEFINED ; +spin_unlock_irqrestore (&tty_ldisc_lock, flags); + +return ret; +} + +EXPORT_SYMBOL (tty_unregister_ldisc); +struct tty_ldisc *tty_ldisc_get (int disc) {unsigned long flags;
include/linux/tty.h +1 -0
@@ -345,6 +345,7 @@ extern int tty_check_change(struct tty_sextern void stop_tty (struct tty_struct * tty);extern void start_tty (struct tty_struct * tty);extern int tty_register_ldisc (int disc,struct tty_ldisc *new_ldisc); +extern int tty_unregister_ldisc (int disc);extern int tty_register_driver (struct tty_driver *driver);extern int tty_unregister_driver (struct tty_driver *driver);extern void tty_register_device (struct tty_driver *driver,unsigned index,struct device *dev);
This patch is wrong. Please do not apply.
An unmodified ldisc driver (externally maintained) will continue to call tty_register_ldisc with NULL, but the new behavior will be to set the ldisc pointer to NULL but have LDISC_FLAG_DEFINED set.
If you feel it is absolutely necessary to add this new function for cosmetic reasons, then leave the old behavior in place and make tty_unregister_ldisc a wrapper to tty_register_ldisc that uses a NULL pointer.
This preserves sanity and compatibility.
The existing function has a bug:
If new_ldisc != NULL and LDISC_FLAG_DEFINED is set, the ldisc is switched to new_ldisc without checking for non-zero refcount of current ldisc.
--
Paul Fulghum
[email protected]
Quoted 81 lines · Alexey Dobriyan
It's a bit strange to see
tty_register_ldisc()call in modules' exit functions.
...Documentation/tty.txt +1
-1@@ -22,7 +22,7 @@ copy of the structure. You must not re-r discipline even with the same data or your computer again will be eaten by demons. -In order to remove a line discipline call tty_register_ldisc passing NULL. +In order to remove a line discipline call tty_unregister_ldisc(). In ancient times this always worked. In modern times the function will return -EBUSY if the ldisc is currently in use. Since the ldisc referencing code manages the module counts this should not usually be a concern.diff -uprN
linux-tty_register_ldisc_000/drivers/char/tty_io.clinux-tty_register_ldisc_001/drivers/char/tty_io.cdrivers/char/tty_io.c +24
-11@@ -262,17 +262,10 @@ int tty_register_ldisc(int disc, structreturn -EINVAL ;spin_lock_irqsave (&tty_ldisc_lock, flags); -if (new_ldisc) { - tty_ldiscs[disc] = *new_ldisc; - tty_ldiscs[disc].num = disc; - tty_ldiscs[disc].flags |=LDISC_FLAG_DEFINED ; - tty_ldiscs[disc].refcount =0 ; - }else { -if (tty_ldiscs[disc].refcount) - ret = -EBUSY ; -else - tty_ldiscs[disc].flags &= ~LDISC_FLAG_DEFINED ; - } + tty_ldiscs[disc] = *new_ldisc; + tty_ldiscs[disc].num = disc; + tty_ldiscs[disc].flags |=LDISC_FLAG_DEFINED ; + tty_ldiscs[disc].refcount =0 ;spin_unlock_irqrestore (&tty_ldisc_lock, flags);return ret;@@ -280,6 +273,26 @@ int tty_register_ldisc(int disc, structEXPORT_SYMBOL (tty_register_ldisc); +int tty_unregister_ldisc (int disc) +{ +unsigned long flags; +int ret =0 ; + +if (disc <N_TTY || disc >=NR_LDISCS ) +return -EINVAL ; + +spin_lock_irqsave (&tty_ldisc_lock, flags); +if (tty_ldiscs[disc].refcount) + ret = -EBUSY ; +else + tty_ldiscs[disc].flags &= ~LDISC_FLAG_DEFINED ; +spin_unlock_irqrestore (&tty_ldisc_lock, flags); + +return ret; +} + +EXPORT_SYMBOL (tty_unregister_ldisc); +struct tty_ldisc *tty_ldisc_get (int disc) {unsigned long flags;diff -uprN
linux-tty_register_ldisc_000/include/linux/tty.hlinux-tty_register_ldisc_001/include/linux/tty.hinclude/linux/tty.h +1
-1@@ -345,6 +345,7 @@ extern int tty_check_change(struct tty_sextern void stop_tty (struct tty_struct * tty);extern void start_tty (struct tty_struct * tty);extern int tty_register_ldisc (int disc,struct tty_ldisc *new_ldisc); +extern int tty_unregister_ldisc (int disc);extern int tty_register_driver (struct tty_driver *driver);extern int tty_unregister_driver (struct tty_driver *driver);extern void tty_register_device (struct tty_driver *driver,unsigned index,struct device *dev); -To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
-- Paul Fulghum [email protected]
Quoted 3 lines · Paul Fulghum
An unmodified ldisc driver (externally maintained) will continue to call
tty_register_ldiscwith NULL, but the new behavior will be to set the ldisc pointer to NULL but haveLDISC_FLAG_DEFINEDset.
I was partially wrong.
After Alexey's patch, a NULL new_ldisc is accessed (not assigned) causing an oops. This is still not good behavior.
As for the refcount bug, this patch should fix it. Comments?
--
Paul Fulghum
[email protected]
drivers/char/tty_io.c +5 -5
@@ -262,16 +262,16 @@ int tty_register_ldisc(int disc, structreturn -EINVAL ;spin_lock_irqsave (&tty_ldisc_lock, flags); -if (new_ldisc) { +if ((tty_ldiscs[disc].flags &LDISC_FLAG_DEFINED ) && + tty_ldiscs[disc].refcount) + ret = -EBUSY ; +else if (new_ldisc) { tty_ldiscs[disc] = *new_ldisc; tty_ldiscs[disc].num = disc; tty_ldiscs[disc].flags |=LDISC_FLAG_DEFINED ; tty_ldiscs[disc].refcount =0 ; }else { -if (tty_ldiscs[disc].refcount) - ret = -EBUSY ; -else - tty_ldiscs[disc].flags &= ~LDISC_FLAG_DEFINED ; + tty_ldiscs[disc].flags &= ~LDISC_FLAG_DEFINED ; }spin_unlock_irqrestore (&tty_ldisc_lock, flags);
The following patch would be more appropriate than my last suggestion for the case of trying to register a ldisc driver to an occupied slot.
It does not make sense to allow an existing registered driver to be overwritten, even
if the refcount is zero.
This *should* not happen with unique ldisc numbers, but it seems like a reasonable check. Even if Alexey's patch is applied, this would be a reasonable check to integrate.
Signature · 24 lines
Paul Fulghum [email protected] --- linux-2.6.11/drivers/char/tty_io.c 2005-03-02 01:38:10.000000000 -0600 +++ b/drivers/char/tty_io.c 2005-06-01 08:34:05.000000000 -0500 @@ -263,10 +263,14 @@ int tty_register_ldisc(int disc, struct spin_lock_irqsave(&tty_ldisc_lock, flags); if (new_ldisc) { - tty_ldiscs[disc] = *new_ldisc; - tty_ldiscs[disc].num = disc; - tty_ldiscs[disc].flags |= LDISC_FLAG_DEFINED; - tty_ldiscs[disc].refcount = 0; + if (tty_ldiscs[disc].flags & LDISC_FLAG_DEFINED) + ret = -EBUSY; + else { + tty_ldiscs[disc] = *new_ldisc; + tty_ldiscs[disc].num = disc; + tty_ldiscs[disc].flags |= LDISC_FLAG_DEFINED; + tty_ldiscs[disc].refcount = 0; + } } else { if(tty_ldiscs[disc].refcount) ret = -EBUSY;
Quoted 5 lines · Paul Fulghum
This patch is wrong. Please do not apply.
An unmodified ldisc driver (externally maintained) will continue to call
tty_register_ldiscwith NULL, but the new behavior will be to set the ldisc pointer to NULL but haveLDISC_FLAG_DEFINEDset.
Then fix that code. We can not do anything about externally maintained code, sorry.
Quoted 6 lines
If you feel it is absolutely necessary to add this new function for cosmetic reasons, then leave the old behavior in place and make
tty_unregister_ldisca wrapper totty_register_ldiscthat uses a NULL pointer.This preserves sanity and compatibility.
We don't care about compatibility in order to keep our sanity. See Documentation/stable_api_nonsense.txt for details :)
thanks,
greg k-h
Quoted 10 lines · Paul Fulghum
This patch is wrong. Please do not apply.
An unmodified ldisc driver (externally maintained) will continue to call
tty_register_ldiscwith NULL, but the new behavior will be to set the ldisc pointer to NULL but haveLDISC_FLAG_DEFINEDset.If you feel it is absolutely necessary to add this new function for cosmetic reasons, then leave the old behavior in place and make
tty_unregister_ldisca wrapper totty_register_ldiscthat uses a NULL pointer.
Nah, we don't want to allow staying out of tree folks out of sync. Adding a BUG_ON for ldisk beeing NULL is much more reasoable.
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Paul Fulghum
An unmodified ldisc driver (externally maintained) will continue to call
tty_register_ldiscwith NULL, but the new behavior will be to set the ldisc pointer to NULL but haveLDISC_FLAG_DEFINEDset.
Quoted 7 lines
Quoted 4 lines
If you feel it is absolutely necessary to add this new function for cosmetic reasons, then leave the old behavior in place and make
tty_unregister_ldisca wrapper totty_register_ldiscthat uses a NULL pointer.Nah, we don't want to allow staying out of tree folks out of sync. Adding a
BUG_ONfor ldisk beeing NULL is much more reasoable.
Why add BUG_ON?
tty_register_ldisc(N_FOO, NULL); ... tty_ldiscs[disc] = *new_ldisc; <= NULL