Re: dev/sound/pcm/* patch testers wanted

On 26 Jan, Gordon Bergling wrote:
> On Mon Jan 26, 2004 at 09:25AM -0800, Don Lewis wrote:
>> On 26 Jan, Gordon Bergling wrote:
>> 
>> > The "Danger!" message wasn't appeared, but I got the following on the
>> > console.
>> > 
>> > malloc() of "1024" with the following non-sleepable locks held:
>> > exclusive sleep mutex pcm0:play:0 (pcm play channel) r = 0 (0xc2be8a80)
>> > locked _at_
>> > /storage/os/src/freebsd-current/src/sys/dev/sound/pcm/buffer.c:195
>> > malloc() of "4096" with the following non-sleepable locks held:
>> > exclusive sleep mutex pcm0:play:0 (pcm play channel) r = 0 (0xc2be8a80)
>> > locked _at_
>> > /storage/os/src/freebsd-current/src/sys/dev/sound/pcm/buffer.c:195
>> 
>> A stack trace would be helpful so that we know where malloc() was being
>> called.  I've got a hunch, though.  What hardware specific driver are
>> you using?
> 
> There was no stack trace. Can I print a stack trace with gdb or
> something simliar?
> 
> The hardware is the following:
> pcm0: <AudioPCI ES1373-B> port 0xe000-0xe03f irq 11 at device 12.0 on pci0
> pcm0: <Cirrus Logic CS4297A AC97 Codec>

Ok, the problem is that the setblocksize method in the es137x driver (as
well as many others), calls sndbuf_resize(), which calls malloc(), and
this patch changed the malloc() call from M_NOWAIT to M_WAITOK.

Quick and dirty solutions:
	Switch back to M_NOWAIT even though we don't handle malloc()
        failure gracefully.

	Unlock the channel before calling the setblocksize method, but
	an interrupt happening in the middle of sndbuf_resize() would
        not be a good thing.

	Add a pointer back to the channel back to the snd_dbuf structure
	so that sndbuf_resize() could do the proper unlocking and
        locking.


This is another manifestation of an API problem related to the
setblocksize method.

        /* Increase the size of bufsoft if before increasing bufhard. */
        maxsize = sndbuf_getsize(b);
        if (sndbuf_getsize(bs) > maxsize)
                maxsize = sndbuf_getsize(bs);
        if (reqblksz * blkcnt > maxsize)
                maxsize = reqblksz * blkcnt;
        if (sndbuf_getsize(bs) != maxsize || sndbuf_getblksz(bs) != reqblksz) {
                ret = sndbuf_remalloc(bs, maxsize/reqblksz, reqblksz, c->lock);
                if (ret)
                        goto out1;
        }

        sndbuf_setblksz(b, CHANNEL_SETBLOCKSIZE(c->methods, c->devinfo, blksz));

        /* Decrease the size of bufsoft after decreasing bufhard. */
        maxsize = sndbuf_getsize(b);
        if (reqblksz * blkcnt > maxsize)
                maxsize = reqblksz * blkcnt;
        if (maxsize > sndbuf_getsize(bs))
                printf("Danger! %s bufsoft size increasing from %d to %d after C
HANNEL_SETBLOCKSIZE()\n",
                    c->name, sndbuf_getsize(bs), maxsize);
        if (sndbuf_getsize(bs) != maxsize || sndbuf_getblksz(bs) != reqblksz) {
                ret = sndbuf_remalloc(bs, maxsize/reqblksz, reqblksz, c->lock);
                if (ret)
                        goto out1;
        }

Ideally, the the setblocksize method would calculate its desired
blocksize and then perform a callback into the channel or buffer code
that would resize both bufhard and bufsoft if necessary, with all the
malloc() calls grouped together so that the channel lock only had to be
dropped and grabbed once.  The setblocksize method would have to pass
through the desired bufsoft blksz (before scaling for format & speed) so
that bufsoft is properly resized.  This would eliminate the need for
the two sndbuf_remalloc() calls above since we would know the actual
size needed.

Also, calling sndbuf_setblksz() on the return value from the
setblocksize method is dangerous, since we end up with blksz*blkcnt !=
bufsize.