Page MenuHomeFreeBSD

sound: Merge pcm_chn_create() and chn_init()
ClosedPublic

Authored by christos on Sun, Apr 28, 4:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 8, 1:33 PM
Unknown Object (File)
Mon, May 6, 6:37 PM
Unknown Object (File)
Tue, Apr 30, 9:35 PM
Unknown Object (File)
Tue, Apr 30, 2:09 PM
Unknown Object (File)
Tue, Apr 30, 7:57 AM
Unknown Object (File)
Tue, Apr 30, 7:56 AM
Unknown Object (File)
Tue, Apr 30, 7:56 AM
Unknown Object (File)
Tue, Apr 30, 7:56 AM
Subscribers

Details

Summary

Follow-up of 6e1d5a97d55c4b2d082a2ca9132f16bf434cac92 ("sound: Merge
pcm_chn_destroy() and chn_kill()")

While here, add device_printf()'s to all failure points.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

In out2, call PCM_LOCK() before freeing the kobj methods and the channel, since
that's what pcm_chn_create() did.

sys/dev/sound/pcm/channel.c
1373–1375

Some of the out2 exits don't hold the channel lock. It gets a bit convoluted here with cleaning up different initialization stages. Maybe use additional exit labels, or extract some parts into separate functions?

1402

Do we have to acquire lock here? I think we already hold it for the out1 exits above?

sys/dev/sound/pcm/channel.c
1373–1375

It looks like this is a bug that was there already. The left diff also has this problem in the first 2 goto outs...

1402

We unlock before the call to malloc().

sys/dev/sound/pcm/channel.c
1373–1375

I think:

	if (CHN_LOCKOWNED(c))
		CHN_UNLOCK(c);

will suffice, no?

sys/dev/sound/pcm/channel.c
1402

Sorry, my bad, I confused it with out2. You're right, I'll remove this.

christos marked 2 inline comments as done.

Address Florian's comments.

@dev_submerge.ch If there's nothing else that needs fixing I think we can go ahead with it.

@dev_submerge.ch If there's nothing else that needs fixing I think we can go ahead with it.

I didn't find the time yet to go through the code in detail (and fully awake - it's late here). I'll catch on tomorrow.

sys/dev/sound/pcm/channel.c
1210

Is the num parameter actually used? It comes from a variety of places, but all I ever see is a value of -1. For values >= 0 implementation below checks that there are no allocated channels >= num, I don't get the use case.

If it is still needed, I'd recommend to check for num < 0 here and below to make it more robust. Otherwise we could remove the parameter here and in the callers.

sys/dev/sound/pcm/channel.c
1210

I remove it in a follow-up patch to avoid having to modify a few dependent patches.

sys/dev/sound/pcm/channel.c
1210
This revision is now accepted and ready to land.Mon, May 6, 4:22 PM