Date: Thu, 15 Sep 2005 11:46:53 +0800 From: Ariff Abdullah <skywizard@MyBSD.org.my> To: pyunyh@gmail.com, minimarmot@gmail.com Cc: freebsd-current@freebsd.org Subject: Re: panic upon kldunload snd_ich (lor # 159) Message-ID: <20050915114653.431f17c5.skywizard@MyBSD.org.my> In-Reply-To: <20050915014509.GA17602@rndsoft.co.kr> References: <47d0403c05091121047a037946@mail.gmail.com> <20050912044212.GC5182@rndsoft.co.kr> <47d0403c05091122276fd0a231@mail.gmail.com> <20050913070149.GE9481@rndsoft.co.kr> <47d0403c0509131235ed58122@mail.gmail.com> <20050914014830.GA13631@rndsoft.co.kr> <20050915033848.2d87da42.skywizard@MyBSD.org.my> <20050915014509.GA17602@rndsoft.co.kr>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --Multipart=_Thu__15_Sep_2005_11_46_53_+0800_M9_q1d0zk/zgEu9E Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 15 Sep 2005 10:45:09 +0900 Pyun YongHyeon <pyunyh@gmail.com> wrote: > > That would be supposed to fix the LOR message. But I'd like to keep > lock ordering between snd_mutex and sndstat_lock. Since sndstat_read() > could be called at any time there is an implicit lock order. > I think switching to sx lock from mutex in sndstat code was to allow > uiomove with lock held. IMO, it would be even better to rewrite > sndstat_read() without using uiomove such that it can also use > standard mutex rather than sx lock. > I tend to agree with you. Since that sndstat_busy() isn't enough, how about we acquire the entire sndstat so nobody can monkey with it (as the proposed / attached diff) and at the same time avoiding this LOR message. It seems much better rather than locking sndstat after sndstat_busy() and much of pcm_unregister() procedures, only to find out that somebody acquire it within that moment. -- Ariff Abdullah MyBSD http://www.MyBSD.org.my (IPv6/IPv4) http://staff.MyBSD.org.my (IPv6/IPv4) http://tomoyo.MyBSD.org.my (IPv6/IPv4) --Multipart=_Thu__15_Sep_2005_11_46_53_+0800_M9_q1d0zk/zgEu9E Content-Type: text/plain; name="nuke_sndstat_lor.diff" Content-Disposition: attachment; filename="nuke_sndstat_lor.diff" Content-Transfer-Encoding: 7bit --- sys/dev/sound/pcm/sound.h.orig Thu Sep 15 11:25:29 2005 +++ sys/dev/sound/pcm/sound.h Thu Sep 15 11:31:19 2005 @@ -238,11 +238,12 @@ int sysctl_hw_snd_vchans(SYSCTL_HANDLER_ARGS); typedef int (*sndstat_handler)(struct sbuf *s, device_t dev, int verbose); +int sndstat_acquire(void); +int sndstat_release(void); int sndstat_register(device_t dev, char *str, sndstat_handler handler); int sndstat_registerfile(char *str); int sndstat_unregister(device_t dev); int sndstat_unregisterfile(char *str); -int sndstat_busy(void); #define SND_DECLARE_FILE(version) \ _SND_DECLARE_FILE(__LINE__, version) --- sys/dev/sound/pcm/sndstat.c.orig Thu Sep 15 11:25:01 2005 +++ sys/dev/sound/pcm/sndstat.c Thu Sep 15 11:31:07 2005 @@ -195,6 +195,42 @@ } int +sndstat_acquire(void) +{ + intrmask_t s; + + s = spltty(); + sx_xlock(&sndstat_lock); + if (sndstat_isopen) { + sx_unlock(&sndstat_lock); + splx(s); + return EBUSY; + } + sndstat_isopen = 1; + sx_unlock(&sndstat_lock); + splx(s); + return 0; +} + +int +sndstat_release(void) +{ + intrmask_t s; + + s = spltty(); + sx_xlock(&sndstat_lock); + if (!sndstat_isopen) { + sx_unlock(&sndstat_lock); + splx(s); + return EBADF; + } + sndstat_isopen = 0; + sx_unlock(&sndstat_lock); + splx(s); + return 0; +} + +int sndstat_register(device_t dev, char *str, sndstat_handler handler) { intrmask_t s; @@ -371,12 +407,6 @@ sx_xunlock(&sndstat_lock); sx_destroy(&sndstat_lock); return 0; -} - -int -sndstat_busy(void) -{ - return (sndstat_isopen); } static void --- sys/dev/sound/pcm/sound.c.orig Thu Sep 15 11:25:05 2005 +++ sys/dev/sound/pcm/sound.c Thu Sep 15 11:31:07 2005 @@ -758,24 +758,25 @@ struct snddev_channel *sce; struct pcm_channel *ch; + if (sndstat_acquire() != 0) { + device_printf(dev, "unregister: sndstat busy\n"); + return EBUSY; + } + snd_mtxlock(d->lock); if (d->inprog) { device_printf(dev, "unregister: operation in progress\n"); snd_mtxunlock(d->lock); + sndstat_release(); return EBUSY; } - if (sndstat_busy() != 0) { - device_printf(dev, "unregister: sndstat busy\n"); - snd_mtxunlock(d->lock); - return EBUSY; - } - SLIST_FOREACH(sce, &d->channels, link) { ch = sce->channel; if (ch->refcount > 0) { device_printf(dev, "unregister: channel %s busy (pid %d)\n", ch->name, ch->pid); snd_mtxunlock(d->lock); + sndstat_release(); return EBUSY; } } @@ -783,6 +784,7 @@ if (mixer_uninit(dev)) { device_printf(dev, "unregister: mixer busy\n"); snd_mtxunlock(d->lock); + sndstat_release(); return EBUSY; } @@ -807,9 +809,10 @@ chn_kill(d->fakechan); fkchan_kill(d->fakechan); - sndstat_unregister(dev); snd_mtxunlock(d->lock); snd_mtxfree(d->lock); + sndstat_unregister(dev); + sndstat_release(); return 0; } --Multipart=_Thu__15_Sep_2005_11_46_53_+0800_M9_q1d0zk/zgEu9E--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050915114653.431f17c5.skywizard>