sound: Simplify locking during device creation

[christos]

Summary

The mechanism of acquiring SD_F_BUSY in pcm_init() and releasing it in
pcm_register() is a leftover from the previous device creation scheme,
where pcm_init() (previously pcm_register()) would create the sysctl
nodes, as well as the device node. In this scenario, acquiring SD_F_BUSY
was necessary, in order to avoid races in case the device was accessed
before the device was ready for use. Commit 66f3eb14e955 ("sound: Move
sysctl and /dev/dspX creation to pcm_setstatus()") fixed this issue, so
we can simplify things now. Only acquire SD_F_BUSY in pcm_addchan(),
because chn_init() expects to be called with SD_F_BUSY acquired.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

[markj]

So now we busy the pcm device during creation of each channel, but the busy state is not maintained between pcm_addchan() calls. I guess we are not worried about something happening in between those calls which might disturb initialization and registration?

[christos]

In D48482#1106184, @markj wrote:

So now we busy the pcm device during creation of each channel, but the busy state is not maintained between pcm_addchan() calls. I guess we are not worried about something happening in between those calls which might disturb initialization and registration?

If it wasn't for the fact that chn_init() is called during runtime as well, then I think it wouldn't make sense to busy the device in pcm_addchan() either. Is there a scenario I'm currently missing? If we're creating the sysctl and device nodes at the end of the device registration, then we are safe from the registration being disturbed by external factors (i.e a process, or the user).

[markj]

In D48482#1106185, @christos wrote:

In D48482#1106184, @markj wrote:

So now we busy the pcm device during creation of each channel, but the busy state is not maintained between pcm_addchan() calls. I guess we are not worried about something happening in between those calls which might disturb initialization and registration?

If it wasn't for the fact that chn_init() is called during runtime as well, then I think it wouldn't make sense to busy the device in pcm_addchan() either. Is there a scenario I'm currently missing? If we're creating the sysctl and device nodes at the end of the device registration, then we are safe from the registration being disturbed by external factors (i.e a process, or the user).

Right, the model we're using here is, the device is newly created, so there are no references to it that another thread might use to somehow manipulate its state unexpectedly. Once you call pcm_addchan(), though, that's not really true anymore. At that point it's hard (for me) to say whether the busy lock is required or not.

[christos]

In D48482#1106227, @markj wrote:

In D48482#1106185, @christos wrote:

In D48482#1106184, @markj wrote:

So now we busy the pcm device during creation of each channel, but the busy state is not maintained between pcm_addchan() calls. I guess we are not worried about something happening in between those calls which might disturb initialization and registration?

If it wasn't for the fact that chn_init() is called during runtime as well, then I think it wouldn't make sense to busy the device in pcm_addchan() either. Is there a scenario I'm currently missing? If we're creating the sysctl and device nodes at the end of the device registration, then we are safe from the registration being disturbed by external factors (i.e a process, or the user).

Right, the model we're using here is, the device is newly created, so there are no references to it that another thread might use to somehow manipulate its state unexpectedly. Once you call pcm_addchan(), though, that's not really true anymore. At that point it's hard (for me) to say whether the busy lock is required or not.

Even though the channels are created, they cannot be accessed if there's no sysctl or device node yet. Isn't it the case that up until the point that we create the sysctls and device, this code is executed only by a single thread?

[christos]

I should also mention that the channels pcm_addchan() creates are the primary (i.e. hardware) ones, and they are not assigned to any process at that point.

[christos]

While here, move the sndstat_register() call further down to be more robust.
There is no good reason to have it where it is. I think we can include this
change in this patch.

[christos]

@markj Could you give it a re-accept if it's good?