From owner-freebsd-hackers@FreeBSD.ORG Sun May 9 12:41:07 2004 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5673216A4CE for ; Sun, 9 May 2004 12:41:07 -0700 (PDT) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2CDD943D46 for ; Sun, 9 May 2004 12:41:05 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.12.9p2/8.12.9) with ESMTP id i49Jev7E096965; Sun, 9 May 2004 12:41:02 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <200405091941.i49Jev7E096965@gw.catspoiler.org> Date: Sun, 9 May 2004 12:40:57 -0700 (PDT) From: Don Lewis To: junior@p233.if.pwr.wroc.pl In-Reply-To: <20040509085356.GA15318@p233.if.pwr.wroc.pl> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii cc: freebsd-hackers@FreeBSD.org Subject: Re: non-recursive mutex, sbc, pcm, kernel panic X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 09 May 2004 19:41:07 -0000 On 9 May, Bartek Marcinkiewicz wrote: > Hello, > > I've just experienced kernel panic while trying > to play mp3 file. (My sound card: Creative Sound > Blaster 16, ISA, worked fine on older 5.x system). > > While reading code: > > sys/dev/sound/pcm/sound.c::snd_mtxcreate() creates > non-recursive mutex. > > But in sys/dev/sound/isa/sb16.c::sb_setup() we have: > > sb_setup() > { > > sb_lock(sb); > > /* ... */ > > sb_reset_dsp(sb); > > /* ... */ > } > > sb_reset_dsp() function locks this mutex again, causing > panic with message: > _mtx_lock_sleep: recursed on non-recursive mutex sbc0 @... > > Is this known issue? Why this mutex should be non-recursive? > Attached patch makes it work again. This isn't a known issue. It looks like you are the first person to exercise the sb16 driver in FreeBSD-5.x in quite some time. Allowing recursive locks makes it much more difficult to get the locking correct since you lose the ability to use assertions about the lock state at function entry and exit. Typically the proper fix in this case would be to remove the sb_lock() call from sb_reset_dsp() and always let the caller do the locking. The patch below should do the trick, though I believe the added locking and unlocking (the second section of the patch) could be omitted in sb16_attach() since no other thread can access the device while the attach is in progress. Index: sys/dev/sound/isa/sb16.c =================================================================== RCS file: /home/ncvs/src/sys/dev/sound/isa/sb16.c,v retrieving revision 1.83 diff -u -r1.83 sb16.c --- sys/dev/sound/isa/sb16.c 14 Apr 2004 14:57:48 -0000 1.83 +++ sys/dev/sound/isa/sb16.c 9 May 2004 19:33:09 -0000 @@ -266,12 +266,10 @@ { u_char b; - sb_lock(sb); sb_wr(sb, SBDSP_RST, 3); DELAY(100); sb_wr(sb, SBDSP_RST, 0); b = sb_get_byte(sb); - sb_unlock(sb); if (b != 0xAA) { DEB(printf("sb_reset_dsp 0x%lx failed\n", rman_get_start(sb->io_base))); @@ -799,8 +797,12 @@ if (sb16_alloc_resources(sb, dev)) goto no; - if (sb_reset_dsp(sb)) + sb_lock(sb); + if (sb_reset_dsp(sb)) { + sb_unlock(sb); goto no; + } + sb_unlock(sb); if (mixer_init(dev, &sb16mix_mixer_class, sb)) goto no; if (snd_setup_intr(dev, sb->irq, 0, sb_intr, sb, &sb->ih))