Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
Andrew Morton schrieb: > On Wed, 17 Jan 2007 18:42:13 +0100 (CET) > Tilman Schmidt <[EMAIL PROTECTED]> wrote: > >> +mutex_init(>mutex); >> +mutex_lock(>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.0 +0100 +++ local/drivers/isdn/gigaset/common.c 2007-01-26 13:38:42.0 +0100 @@ -640,7 +640,6 @@ struct cardstate *gigaset_initcs(struct return NULL; } mutex_init(>mutex); - mutex_lock(>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(>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(>lock, flags); cs->running = 1; spin_unlock_irqrestore(>lock, flags); + + mutex_unlock(>mutex); + setup_timer(>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(>timer); gig_dbg(DEBUG_INIT, "cs initialized"); - mutex_unlock(>mutex); return cs; error: - mutex_unlock(>mutex); gig_dbg(DEBUG_INIT, "failed"); gigaset_freecs(cs); return NULL; -- Tilman SchmidtE-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
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.0 +0100 +++ local/drivers/isdn/gigaset/common.c 2007-01-26 13:38:42.0 +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 SchmidtE-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
On Wed, 17 Jan 2007 18:42:13 +0100 (CET) Tilman Schmidt <[EMAIL PROTECTED]> wrote: > + mutex_init(>mutex); > + mutex_lock(>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. - 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/
Re: [PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes
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. 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. - 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/
[PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes (resend)
My apologies if anyone is getting this more than once. I still haven't
seen it appear on the list after two attempts, so I'm sending it again,
this time through a different server.
Subject: [PATCH] Gigaset ISDN driver error handling fixes
From: Tilman Schmidt <[EMAIL PROTECTED]>
Fix several flaws in the error handling of the Siemens Gigaset ISDN
driver, including one that would cause an Oops when connecting more
than one device of the same type.
Signed-off-by: Tilman Schmidt <[EMAIL PROTECTED]>
---
--- linux-2.6.20-rc5-orig/drivers/isdn/gigaset/common.c 2007-01-15
00:50:22.0 +0100
+++ linux-2.6.20-rc5-work/drivers/isdn/gigaset/common.c 2007-01-17
11:45:44.0 +0100
@@ -356,16 +356,17 @@ static struct cardstate *alloc_cs(struct
{
unsigned long flags;
unsigned i;
- static struct cardstate *ret = NULL;
+ struct cardstate *ret = NULL;
spin_lock_irqsave(>lock, flags);
for (i = 0; i < drv->minors; ++i) {
if (!(drv->flags[i] & VALID_MINOR)) {
- drv->flags[i] = VALID_MINOR;
- ret = drv->cs + i;
- }
- if (ret)
+ if (try_module_get(drv->owner)) {
+ drv->flags[i] = VALID_MINOR;
+ ret = drv->cs + i;
+ }
break;
+ }
}
spin_unlock_irqrestore(>lock, flags);
return ret;
@@ -376,6 +377,8 @@ static void free_cs(struct cardstate *cs
unsigned long flags;
struct gigaset_driver *drv = cs->driver;
spin_lock_irqsave(>lock, flags);
+ if (drv->flags[cs->minor_index] & VALID_MINOR)
+ module_put(drv->owner);
drv->flags[cs->minor_index] = 0;
spin_unlock_irqrestore(>lock, flags);
}
@@ -579,7 +582,7 @@ static struct bc_state *gigaset_initbcs(
} else if ((bcs->skb = dev_alloc_skb(SBUFSIZE + HW_HDR_LEN)) != NULL)
skb_reserve(bcs->skb, HW_HDR_LEN);
else {
- warn("could not allocate skb\n");
+ warn("could not allocate skb");
bcs->inputstate |= INS_skip_frame;
}
@@ -632,17 +635,25 @@ struct cardstate *gigaset_initcs(struct
int i;
gig_dbg(DEBUG_INIT, "allocating cs");
- cs = alloc_cs(drv);
- if (!cs)
- goto error;
+ if (!(cs = alloc_cs(drv))) {
+ err("maximum number of devices exceeded");
+ return NULL;
+ }
+ mutex_init(>mutex);
+ mutex_lock(>mutex);
+
gig_dbg(DEBUG_INIT, "allocating bcs[0..%d]", channels - 1);
cs->bcs = kmalloc(channels * sizeof(struct bc_state), GFP_KERNEL);
- if (!cs->bcs)
+ if (!cs->bcs) {
+ err("out of memory");
goto error;
+ }
gig_dbg(DEBUG_INIT, "allocating inbuf");
cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
- if (!cs->inbuf)
+ if (!cs->inbuf) {
+ err("out of memory");
goto error;
+ }
cs->cs_init = 0;
cs->channels = channels;
@@ -654,8 +665,6 @@ struct cardstate *gigaset_initcs(struct
spin_lock_init(>ev_lock);
cs->ev_tail = 0;
cs->ev_head = 0;
- mutex_init(>mutex);
- mutex_lock(>mutex);
tasklet_init(>event_tasklet, _handle_event,
(unsigned long) cs);
@@ -684,8 +693,10 @@ struct cardstate *gigaset_initcs(struct
for (i = 0; i < channels; ++i) {
gig_dbg(DEBUG_INIT, "setting up bcs[%d].read", i);
- if (!gigaset_initbcs(cs->bcs + i, cs, i))
+ if (!gigaset_initbcs(cs->bcs + i, cs, i)) {
+ err("could not allocate channel %d data", i);
goto error;
+ }
}
++cs->cs_init;
@@ -720,8 +731,10 @@ struct cardstate *gigaset_initcs(struct
make_valid(cs, VALID_ID);
++cs->cs_init;
gig_dbg(DEBUG_INIT, "setting up hw");
- if (!cs->ops->initcshw(cs))
+ if (!cs->ops->initcshw(cs)) {
+ err("could not allocate device specific data");
goto error;
+ }
++cs->cs_init;
@@ -743,8 +756,8 @@ struct cardstate *gigaset_initcs(struct
mutex_unlock(>mutex);
return cs;
-error: if (cs)
- mutex_unlock(>mutex);
+error:
+ mutex_unlock(>mutex);
gig_dbg(DEBUG_INIT, "failed");
gigaset_freecs(cs);
return NULL;
@@ -1040,7 +1053,6 @@ void gigaset_freedriver(struct gigaset_d
spin_unlock_irqrestore(_lock, flags);
gigaset_if_freedriver(drv);
- module_put(drv->owner);
kfree(drv->cs);
kfree(drv->flags);
@@ -1072,10 +1084,6 @@ struct gigaset_driver *gigaset_initdrive
if (!drv)
return NULL;
- if (!try_module_get(owner))
- goto out1;
-
[PATCH 2.6.20-rc5] Gigaset ISDN driver error handling fixes (resend)
My apologies if anyone is getting this more than once. I still haven't
seen it appear on the list after two attempts, so I'm sending it again,
this time through a different server.
Subject: [PATCH] Gigaset ISDN driver error handling fixes
From: Tilman Schmidt [EMAIL PROTECTED]
Fix several flaws in the error handling of the Siemens Gigaset ISDN
driver, including one that would cause an Oops when connecting more
than one device of the same type.
Signed-off-by: Tilman Schmidt [EMAIL PROTECTED]
---
--- linux-2.6.20-rc5-orig/drivers/isdn/gigaset/common.c 2007-01-15
00:50:22.0 +0100
+++ linux-2.6.20-rc5-work/drivers/isdn/gigaset/common.c 2007-01-17
11:45:44.0 +0100
@@ -356,16 +356,17 @@ static struct cardstate *alloc_cs(struct
{
unsigned long flags;
unsigned i;
- static struct cardstate *ret = NULL;
+ struct cardstate *ret = NULL;
spin_lock_irqsave(drv-lock, flags);
for (i = 0; i drv-minors; ++i) {
if (!(drv-flags[i] VALID_MINOR)) {
- drv-flags[i] = VALID_MINOR;
- ret = drv-cs + i;
- }
- if (ret)
+ if (try_module_get(drv-owner)) {
+ drv-flags[i] = VALID_MINOR;
+ ret = drv-cs + i;
+ }
break;
+ }
}
spin_unlock_irqrestore(drv-lock, flags);
return ret;
@@ -376,6 +377,8 @@ static void free_cs(struct cardstate *cs
unsigned long flags;
struct gigaset_driver *drv = cs-driver;
spin_lock_irqsave(drv-lock, flags);
+ if (drv-flags[cs-minor_index] VALID_MINOR)
+ module_put(drv-owner);
drv-flags[cs-minor_index] = 0;
spin_unlock_irqrestore(drv-lock, flags);
}
@@ -579,7 +582,7 @@ static struct bc_state *gigaset_initbcs(
} else if ((bcs-skb = dev_alloc_skb(SBUFSIZE + HW_HDR_LEN)) != NULL)
skb_reserve(bcs-skb, HW_HDR_LEN);
else {
- warn(could not allocate skb\n);
+ warn(could not allocate skb);
bcs-inputstate |= INS_skip_frame;
}
@@ -632,17 +635,25 @@ struct cardstate *gigaset_initcs(struct
int i;
gig_dbg(DEBUG_INIT, allocating cs);
- cs = alloc_cs(drv);
- if (!cs)
- goto error;
+ if (!(cs = alloc_cs(drv))) {
+ err(maximum number of devices exceeded);
+ 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);
- if (!cs-bcs)
+ if (!cs-bcs) {
+ err(out of memory);
goto error;
+ }
gig_dbg(DEBUG_INIT, allocating inbuf);
cs-inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
- if (!cs-inbuf)
+ if (!cs-inbuf) {
+ err(out of memory);
goto error;
+ }
cs-cs_init = 0;
cs-channels = channels;
@@ -654,8 +665,6 @@ struct cardstate *gigaset_initcs(struct
spin_lock_init(cs-ev_lock);
cs-ev_tail = 0;
cs-ev_head = 0;
- mutex_init(cs-mutex);
- mutex_lock(cs-mutex);
tasklet_init(cs-event_tasklet, gigaset_handle_event,
(unsigned long) cs);
@@ -684,8 +693,10 @@ struct cardstate *gigaset_initcs(struct
for (i = 0; i channels; ++i) {
gig_dbg(DEBUG_INIT, setting up bcs[%d].read, i);
- if (!gigaset_initbcs(cs-bcs + i, cs, i))
+ if (!gigaset_initbcs(cs-bcs + i, cs, i)) {
+ err(could not allocate channel %d data, i);
goto error;
+ }
}
++cs-cs_init;
@@ -720,8 +731,10 @@ struct cardstate *gigaset_initcs(struct
make_valid(cs, VALID_ID);
++cs-cs_init;
gig_dbg(DEBUG_INIT, setting up hw);
- if (!cs-ops-initcshw(cs))
+ if (!cs-ops-initcshw(cs)) {
+ err(could not allocate device specific data);
goto error;
+ }
++cs-cs_init;
@@ -743,8 +756,8 @@ struct cardstate *gigaset_initcs(struct
mutex_unlock(cs-mutex);
return cs;
-error: if (cs)
- mutex_unlock(cs-mutex);
+error:
+ mutex_unlock(cs-mutex);
gig_dbg(DEBUG_INIT, failed);
gigaset_freecs(cs);
return NULL;
@@ -1040,7 +1053,6 @@ void gigaset_freedriver(struct gigaset_d
spin_unlock_irqrestore(driver_lock, flags);
gigaset_if_freedriver(drv);
- module_put(drv-owner);
kfree(drv-cs);
kfree(drv-flags);
@@ -1072,10 +1084,6 @@ struct gigaset_driver *gigaset_initdrive
if (!drv)
return NULL;
- if (!try_module_get(owner))
- goto out1;
-
- drv-cs = NULL;
