lkml.org 
[lkml]   [2012]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] tty: tty_mutex: fix lockdep warning in tty_lock_pair
From
On Fri, May 18, 2012 at 2:48 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, 2012-05-17 at 11:28 -0700, Greg Kroah-Hartman wrote:
>> > +static void __lockfunc tty_lock_nest_lock(struct tty_struct *tty,
>> > +             struct tty_struct *tty2)
>>
>> Duplicating tty_lock() just for this one issue seems wrong and prone to
>> error, don't you think?
>>
>> > +{
>> > +     if (tty->magic != TTY_MAGIC) {
>> > +             printk(KERN_ERR "L Bad %p\n", tty);
>> > +             WARN_ON(1);
>> > +             return;
>> > +     }
>> > +     tty_kref_get(tty);
>> > +     mutex_lock_nest_lock(&tty->legacy_mutex, &tty2->legacy_mutex);
>
> Yeah, its completely broken, even the lockdep annotation is the wrong
> one.
>
> Something like the (completely untested) below patch is the 'right' way.

Yes, it should be, but the warning is still triggered with the patch, also
it is worse, kernel hanged during boot.

> ---
>  drivers/tty/tty_mutex.c |   35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
> index 69adc80..587330b 100644
> --- a/drivers/tty/tty_mutex.c
> +++ b/drivers/tty/tty_mutex.c
> @@ -10,7 +10,7 @@
>  * Getting the big tty mutex.
>  */
>
> -void __lockfunc tty_lock(struct tty_struct *tty)
> +static void __lockfunc tty_lock_nested(struct tty_struct *tty, int subclass)
>  {
>        if (tty->magic != TTY_MAGIC) {
>                printk(KERN_ERR "L Bad %p\n", tty);
> @@ -18,7 +18,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
>                return;
>        }
>        tty_kref_get(tty);
> -       mutex_lock(&tty->legacy_mutex);
> +       mutex_lock_nested(&tty->legacy_mutex, subclass);
> +}
> +
> +void __lockfunc tty_lock(struct tty_struct *tty)
> +{
> +       tty_lock_nested(tty, 0);
>  }
>  EXPORT_SYMBOL(tty_lock);
>
> @@ -38,25 +43,25 @@ EXPORT_SYMBOL(tty_unlock);
>  * Getting the big tty mutex for a pair of ttys with lock ordering
>  * On a non pty/tty pair tty2 can be NULL which is just fine.
>  */
> -void __lockfunc tty_lock_pair(struct tty_struct *tty,
> -                                       struct tty_struct *tty2)
> +void __lockfunc tty_lock_pair(struct tty_struct *tty1, struct tty_struct *tty2)
>  {
> -       if (tty < tty2) {
> -               tty_lock(tty);
> -               tty_lock(tty2);
> -       } else {
> -               if (tty2 && tty2 != tty)
> -                       tty_lock(tty2);
> -               tty_lock(tty);
> +       if (!tty2 || tty1 == tty2) {
> +               tty_lock(tty1);
> +               return;
>        }
> +
> +       if (tty2 < tty1)
> +               swap(tty1, tty2);

That is too crazy.

> +
> +       tty_lock(tty1);
> +       tty_lock_nested(tty2, SINGLE_DEPTH_NESTING);
>  }
>  EXPORT_SYMBOL(tty_lock_pair);
>
> -void __lockfunc tty_unlock_pair(struct tty_struct *tty,
> -                                               struct tty_struct *tty2)
> +void __lockfunc tty_unlock_pair(struct tty_struct *tty1, struct tty_struct *tty2)
>  {
> -       tty_unlock(tty);
> -       if (tty2 && tty2 != tty)
> +       tty_unlock(tty1);
> +       if (tty2 && tty2 != tty1)
>                tty_unlock(tty2);
>  }
>  EXPORT_SYMBOL(tty_unlock_pair);
>

So how about the below one(tested OK)?

diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 69adc80..fecf592 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,8 @@
* Getting the big tty mutex.
*/

-void __lockfunc tty_lock(struct tty_struct *tty)
+static void __lockfunc tty_lock_nested(struct tty_struct *tty,
+ int subclass)
{
if (tty->magic != TTY_MAGIC) {
printk(KERN_ERR "L Bad %p\n", tty);
@@ -18,7 +19,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
return;
}
tty_kref_get(tty);
- mutex_lock(&tty->legacy_mutex);
+ mutex_lock_nested(&tty->legacy_mutex, subclass);
+}
+
+void __lockfunc tty_lock(struct tty_struct *tty)
+{
+ tty_lock_nested(tty, 0);
}
EXPORT_SYMBOL(tty_lock);

@@ -43,11 +49,14 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
{
if (tty < tty2) {
tty_lock(tty);
- tty_lock(tty2);
+ tty_lock_nested(tty2, SINGLE_DEPTH_NESTING);
} else {
- if (tty2 && tty2 != tty)
+ int nested = 0;
+ if (tty2 && tty2 != tty) {
tty_lock(tty2);
- tty_lock(tty);
+ nested = SINGLE_DEPTH_NESTING;
+ }
+ tty_lock_nested(tty, nested);
}
}
EXPORT_SYMBOL(tty_lock_pair);

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-05-18 04:41    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site