LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
       [not found] <20070117174213.A39123427B1@gx110.ts.pxnet.com>
@ 2007-01-24  6:47 ` Andrew Morton
  2007-01-26 16:29   ` Tilman Schmidt
  0 siblings, 1 reply; 2+ messages in thread
From: Andrew Morton @ 2007-01-24  6:47 UTC (permalink / raw)
  To: Tilman Schmidt; +Cc: Linux Kernel Mailing List, Hansjoerg Lipp

On Wed, 17 Jan 2007 18:42:13 +0100 (CET)
Tilman Schmidt <tilman@imap.cc> wrote:

> +	mutex_init(&cs->mutex);
> +	mutex_lock(&cs->mutex);

I have vague memories of making rude comments about this a few months ago.

It is very weird to lock a mutex just after intialising it.  I mean, if any other
thread can lock this mutex then there's a race.  If no other thread can
lock it, then this thread doesn't need to either.

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
  2007-01-24  6:47 ` [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes Andrew Morton
@ 2007-01-26 16:29   ` Tilman Schmidt
  0 siblings, 0 replies; 2+ messages in thread
From: Tilman Schmidt @ 2007-01-26 16:29 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Linux Kernel Mailing List, Hansjoerg Lipp

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

Andrew Morton schrieb:
> On Wed, 17 Jan 2007 18:42:13 +0100 (CET)
> Tilman Schmidt <tilman@imap.cc> wrote:
> 
>> +	mutex_init(&cs->mutex);
>> +	mutex_lock(&cs->mutex);
> 
> I have vague memories of making rude comments about this a few months ago.

Rude comments from you? I must have missed that. ;-)

> It is very weird to lock a mutex just after intialising it.  I mean, if any other
> thread can lock this mutex then there's a race.  If no other thread can
> lock it, then this thread doesn't need to either.

No other thread can lock it at this point, because the data structure
pointed to by cs is only just being created. The possibility for other
threads to access it is created a little later, however, specifically
by the calls to gigaset_if_init(cs) and gigaset_init_dev_sysfs(cs) in
line 741 and 744, so the mutex has to be locked before entering these.

But I admit the mutexed code section is cut a bit generously here.
Would something like the following reduce the weirdness sufficiently?
(compile tested only)


--- linux-2.6.20-rc6-work/drivers/isdn/gigaset/common.c	2007-01-25 23:53:17.000000000 +0100
+++ local/drivers/isdn/gigaset/common.c	2007-01-26 13:38:42.000000000 +0100
@@ -640,7 +640,6 @@ struct cardstate *gigaset_initcs(struct
 		return NULL;
 	}
 	mutex_init(&cs->mutex);
-	mutex_lock(&cs->mutex);

 	gig_dbg(DEBUG_INIT, "allocating bcs[0..%d]", channels - 1);
 	cs->bcs = kmalloc(channels * sizeof(struct bc_state), GFP_KERNEL);
@@ -738,6 +737,9 @@ struct cardstate *gigaset_initcs(struct

 	++cs->cs_init;

+	mutex_lock(&cs->mutex);
+
+	/* set up character device */
 	gigaset_if_init(cs);

 	/* set up device sysfs */
@@ -746,6 +748,9 @@ struct cardstate *gigaset_initcs(struct
 	spin_lock_irqsave(&cs->lock, flags);
 	cs->running = 1;
 	spin_unlock_irqrestore(&cs->lock, flags);
+
+	mutex_unlock(&cs->mutex);
+
 	setup_timer(&cs->timer, timer_tick, (unsigned long) cs);
 	cs->timer.expires = jiffies + msecs_to_jiffies(GIG_TICK);
 	/* FIXME: can jiffies increase too much until the timer is added?
@@ -753,11 +758,9 @@ struct cardstate *gigaset_initcs(struct
 	add_timer(&cs->timer);

 	gig_dbg(DEBUG_INIT, "cs initialized");
-	mutex_unlock(&cs->mutex);
 	return cs;

 error:
-	mutex_unlock(&cs->mutex);
 	gig_dbg(DEBUG_INIT, "failed");
 	gigaset_freecs(cs);
 	return NULL;


-- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2007-01-26 16:30 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20070117174213.A39123427B1@gx110.ts.pxnet.com>
2007-01-24  6:47 ` [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes Andrew Morton
2007-01-26 16:29   ` Tilman Schmidt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).