Fix one more race by moving pcm_unregister() after callout_stop() and
callout_drain().

In D47665#1086817, @christos wrote:

Actually, we can now make the function void.

Actually, we can now make the function void.

I don't have a doc commit bit so if someone can commit it I'd appreciate it.

In D47639#1086741, @emaste wrote:

The reason this program exists is to provide a console beep (^G) in the absence of a PC speaker (with a bit more plumbing). At the end of the day even for that use performance isn't really going to matter -- but also the docs recommend against using AFMT_FLOAT.

I can see that this is useful for testing the kernel support, though.

In D47639#1086236, @emaste wrote:

I'd suggest we not do this, at least not before quantifying the performance impact of float support (see comment in D47638).

Assert that we enter pcm_killchans() with !PCM_REGISTERED.

In D47462#1085187, @dev_submerge.ch wrote:

The refactored implementation of pcm_killchans() relies on the SD_F_REGISTERED flag (unset) to prevent addition of new channels, right? Should we make this explicit via assert or a comment?

Address Ed's comment.

Great. @markj what do you think?

Abandoning since __FreeBSD_version was bumped already. Also reverted KPI changes for stable/14: https://cgit.freebsd.org/src/commit/?h=stable/14&id=b64867e332f8196cb61947fc189ff108ca1065db

@dougm @pho Could you please let me know if the panic persists with this patch?

Minor fix in CHN_F_EXCLUSIVE.

Address Mark's comment.

Use correct version value.

Bump.

In D47460#1083443, @emaste wrote:

There may be cases where NULL checks like these eliminate some crashes but leave races that still exist but are harder to hit.

In D47460#1083229, @markj wrote:

When are these fields null?

In D47325#1082805, @jah wrote:

In D47325#1082798, @christos wrote:

In D47325#1082791, @jah wrote:

These appear to be both KPI and KBI-breaking changes, so shouldn't they have included a __FreeBSD_version bump? Also, it looks like you MFCed these to stable/14, don't we at least frown on that kind of breakage in a -stable branch?

I am pretty confident these changes should not cause any breakage in the -stable branch. Also all the drivers that use this KPI have been updated in this patch.

There can be out-of-tree drivers that use the pcm_* KPIs; I still maintain a fairly old one: multimedia/cx88.
It's usually difficult-to-impossible to know with absolute certainty that no one anywhere is relying on a given KPI/KBI, which is why we usually bump the version when making changes like this. That at least gives out-of-tree consumers a version number to check in their code and prevents unpredictable behavior from loading kmods built against the prior KBI.

In D47325#1082791, @jah wrote:

These appear to be both KPI and KBI-breaking changes, so shouldn't they have included a __FreeBSD_version bump? Also, it looks like you MFCed these to stable/14, don't we at least frown on that kind of breakage in a -stable branch?

In D47462#1082599, @dev_submerge.ch wrote:

In D47462#1082422, @christos wrote:

@dev_submerge.ch Can you also test whether this patch along with all the other patches applied (D47459, D47460, D47461, D47463) fix the hot-unload (or any other) panics?

Seems good, for me the panics we had are not reproducible anymore. Didn't have time yet to look at the code, though.

@dev_submerge.ch Can you also test whether this patch along with all the other patches applied (D47459, D47460, D47461, D47463) fix the hot-unload panics (or any other panics)?

Continued in D47462.

Committed.

LGTM, feel free to propose the patch for ai2s and davbus. :)

In D47325#1079580, @dev_submerge.ch wrote:

Not in scope for this patch, but pcm_addchan() may return an error and I wonder whether / how the drivers should handle this error case?

In D47325#1079371, @dev_submerge.ch wrote:

In D47325#1079347, @christos wrote:

Update arm drivers as well.

On a side note, could you check that we didn't miss sys/arm in any other sound driver refactoring? We didn't change them in D43467 for example, although that may not be applicable if these are single instance device drivers.

Update arm drivers as well.

In D47325#1079345, @dev_submerge.ch wrote:

What about the sound drivers under sys/arm? They seem to still reference pcm_setstatus(), did I miss something?

Approved.

https://docs.freebsd.org/en/books/arch-handbook/sound/ will have to be updated as well.

Address Mark's comment.

Also note that pcm_register() does not really touch the root feeder in the first place.