Page MenuHomeFreeBSD

sound: Make device registration more intuitive
ClosedPublic

Authored by christos on Oct 28 2024, 10:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 2, 9:43 PM
Unknown Object (File)
Sun, Dec 1, 8:22 PM
Unknown Object (File)
Thu, Nov 21, 2:17 PM
Unknown Object (File)
Thu, Nov 21, 3:17 AM
Unknown Object (File)
Mon, Nov 11, 12:31 AM
Unknown Object (File)
Fri, Nov 8, 9:34 PM
Unknown Object (File)
Fri, Nov 8, 1:54 AM
Unknown Object (File)
Thu, Nov 7, 7:29 PM
Subscribers

Details

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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60234
Build 57118: arc lint + arc unit

Event Timeline

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

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

Good catch. Thanks.

Update arm drivers as well.

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.

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.

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?

This revision is now accepted and ready to land.Oct 30 2024, 10:18 AM
sys/arm/allwinner/a10_codec.c
1175 ↗(On Diff #145608)

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

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 ↗(On Diff #145608)

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

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 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.

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#1082805, @jah 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.).

In D47325#1082805, @jah 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.)

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?

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.