Andrew Morton schrieb: > On Wed, 17 Jan 2007 18:42:13 +0100 (CET) > Tilman Schmidt <[email protected]> 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: [email protected] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
Attachment:
signature.asc
Description: OpenPGP digital signature
- References:
- Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
- From: Andrew Morton <[email protected]>
- Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
- Prev by Date: Re: [PATCH 2/8] Create the ZONE_MOVABLE zone
- Next by Date: Re: [PATCH 3/8] Allow huge page allocations to use GFP_HIGH_MOVABLE
- Previous by thread: Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
- Next by thread: [git patch] net driver fix
- Index(es):
 |