sound: Make device registration more intuitive

[christos]

Summary

The way a sound driver currently registers to sound(4) is using the
following sequence of function calls:

1. pcm_register() to initialize snddev_info.
2. pcm_addchan() calls to create the device's primary channels.
3. pcm_setstatus() to do the final setup.

While using 3 different functions in a specific order might not be very
elegant, this pattern cannot be easily avoided. However, pcm_register()
and pcm_setstatus() are especially confusing, since one would
intuitively expect:

1. pcm_register() to actually do the registration, as opposed to a basic initialization.
2. pcm_setstatus() to, as the name suggests, set some kind of status, as opposed to finalizing the registration.

This patch renames pcm_register() to pcm_init(), and pcm_setstatus() to
pcm_register(). Drivers are modified accordingly.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

[christos]

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

[dev_submerge.ch]

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

[christos]

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?

Good catch. Thanks.

[christos]

Update arm drivers as well.

[dev_submerge.ch]

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.

[christos]

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.

I will check.

[dev_submerge.ch]

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?

[markj]

sys/arm/allwinner/a10_codec.c

1175

Do we need to do some cleanup in these error paths?

[christos]

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?

I suppose the reason most drivers do not do any error checking for pcm_addchan() is because it might not be preferable to fail the device attach altogether, but continue without a primary channel (and as a result, vchans) in a given direction. However, I think we could improve this by printing a warning if pcm_addchan() fails (chn_init() will print warnings as well, though), at least.

sys/arm/allwinner/a10_codec.c

1175

Haven't really looked into this yet, but perhaps in a future patch I could do some cleanup where needed.

[christos]

Committed.

[jah]

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?

[christos]

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.

[jah]

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.

[christos]

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.

The thing is out-of-tree consumers will not actually be able to compile if they build against this version, so updating them will be a necessity anyway. pcm_init() is a "new" function (a rename of pcm_register()) and pcm_setstatus() has been removed (again, a rename of pcm_setstatus()), so I think we should be safe from unpredictable behavior. Also note that there have been quite a few patches in the last year already that have introduced KPI changes (function removals/additions, behavior changes, etc.).

[markj]

In D47325#1082806, @christos wrote:

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.

The thing is out-of-tree consumers will not actually be able to compile if they build against this version, so updating them will be a necessity anyway.

Yes, but they still need to compile against old versions of FreeBSD as well, hence the need for a __FreeBSD_version bump so that out-of-tree drivers can write

#if __FreeBSD_version > x
...
#else
...
#endif

pcm_init() is a "new" function (a rename of pcm_register()) and pcm_setstatus() has been removed (again, a rename of pcm_setstatus()), so I think we should be safe from unpredictable behavior. Also note that there have been quite a few patches in the last year already that have introduced KPI changes (function removals/additions, behavior changes, etc.).

Not all KPI changes are relevant to out-of-tree drivers, but this one is since any sound driver needs to use these KPIs.

So I agree with Jason; this commit at minimum needs a __FreeBSD_version bump, if there hasn't already been one for something unrelated. And it should probably be reverted from stable/14 since it'll break multimedia/cx88, as kernel modules from ports will be compiled against releng/14.0, even when shipping for 14.2. (This policy is really problematic for out-of-tree kernel modules for exactly this reason, but we don't have a solution yet.)

[dev_submerge.ch]

Yeah, sorry, I forgot about the out-of-tree modules too. The most important ones are probably HDMI audio devices from graphics modules (intel, radeon, amd, nvidia). What are the CURRENT pkg builds based on?

[emaste]

What are the CURRENT pkg builds based on?

-CURRENT package builds are a moving target - the are not pinned to a release version. There is some latency involved, so there is occasionally a short period where the package that's available was built against a base system from before a change and will not work with an up-to-date user-built kernel. In general -CURRENT users are best served by building their own up-to-date kernel module packages.