No idea why I would have done this, I've done plenty of atf-c... thanks!

I'm not that familiar with hastd, but this looks straightforward and correct to me.

review feedback: go with the first suggestion, and leave pluton_wait_reply() to
always poll for a reply at least once. The timeout == 0 case shouldn't really
happen in practice today, but it's worth defining the semantics now.

In D58779#1359549, @christos wrote:

Can you please take care of MFCing this?

In D59296#1360620, @ziaee wrote:

Can you put this in rc.conf.5 too?

Review feedback:

• Scope the backlight settings off into /var/db/backlight
• Turn it off by default, mention it in the manpage
• Condense the license/copyright header down and fix the PROVIDE/REQUIRE
• Use my preferred quoting style while we're here

In D59277#1360135, @ngie wrote:

I think the prior approach suggested by @dim to make as minimal of a change as possible makes a lot of sense: there's no real single-maintainer in inetd and the daemon has remained in maintenance mode for many years.

I was a little torn when it went into GENERIC the first time, but moving into a world where we can reliably do more interesting things with various PCR measurements (which I did see your review for, but haven't had time to get back to it) I think it's a solid addition.

In D59239#1359053, @kbowling wrote:

Thanks for adding csprng. One thing that bears a bit of scrutiny here and the existing tpm20 collector is the registered quality level. https://trustedcomputinggroup.org/resource/sha-1-uses-in-tpm-v1-2/ claims the SHA-1 mixing function does not materially degrade the output and I can link some papers that claim they tend to outperform PRNGs. In a tpm20, you generally have a high(er) quality random source and mixing function, but it could also be a vTPM proxying CPU rdrand or the like.

The inline comment isn't really all that critical because the types of errors retuned today would be interpreted as fatal, so maybe it isn't worth it

CC @des as well

CC releng because this should go into 14.5, IMO, which might mean an expedited MFC. I think this is reasonably safe.

This LGTM and also tests fine with my Yubikey. My test setup is naive, but just to document it:

In D59186#1357635, @aokblast wrote:

@kevans Could you please check if there is any regression on your yubikey? Thanks!

Tag them as monitors instead and re-word the comment slightly. The commit
message has also been updated locally.

This seems to work reasonably well and happens quickly enough that there is infact no feedback:

In D58779#1354918, @christos wrote:

In D58779#1354917, @kevans wrote:

static u_int16_t snd_mixerdefaults[SOUND_MIXER_NRDEVICES] = {
[...]
        [SOUND_MIXER_MONITOR]   = 75,                                                                         
};

Anythng above 15 or 20% results in major feedback, and it's not clear how to best handle this. You can hint it lower, of course, but I wonder if it'd make sense to add a function to the kernel mixer interface to allow uaudio to override the default.

You could call mix_set() from uaudio once everything is set up, and set the volume like that. Should work I think.

In D58779#1354788, @christos wrote:

In D58779#1354787, @kevans wrote:

It isn't tied to any of those now, with this patch. It probably makes sense to add a way to plumb the name out so it can be controlled via sysctl, but IGAIN and MONITOR both sounded like they're historically used for subtly different things than this.

I think it'd be better to use one of the existing OSS devices and show it under mixer(8), than implement yet another sysctl.

It isn't tied to any of those now, with this patch. It probably makes sense to add a way to plumb the name out so it can be controlled via sysctl, but IGAIN and MONITOR both sounded like they're historically used for subtly different things than this.

In D58779#1354749, @christos wrote:

on this headset, the vol control does absolutely nothing while pcm controls the volume.

Which controls do you get in mixer(8)?

It should probably be noted that this effectively orphans the corresponding __*_chk symbols in libc/secure. I don't think that's a bad thing or that we should stop exposing a default version for them, it just means that we won't emit references to them with our own implementation.

That's a bit embarrasing. =\ I have no idea how this one was overlooked, I did a lot of grepping around for cr_groups indexing...

Gentle ping

In D56845#1352108, @christos wrote:

In D56845#1349251, @fernape wrote:

I would argue that the user might not know it. "Invalid argument" is barely descriptive IMHO. If mixer knows the problem is that the supplied device number is negative, why not tell the user the exact problem instead of trusting the user "should know" something?

The reason I'm pushing back against this is because, apart from the fact that the -d option is already documented in the man page, the logic of the option is that you supply the name of the device (e.g., pcm0) or just its unit number. The error messages you introduce are accurate in the sense that they are more fine-grained in an input sense, but the point is that the user is trying to reference a device that does not exist, and these messages in my opinion make the issue more about the fact that the numbers are negative/out of range/etc, and not about the fact that the device simply does not exist. That's why I think "Invalid argument" for example is enough, because if someone deliberately tries to use the -d option, they should know what arguments it expects also, they wouldn't use an option randomly.

There's also a copy in if_detach_internal that should be resized, this should have tripped a _Static_assert there. (No opinion on the literal)

Ah, vol is neutralized because of that mixer #36. It defaults to ctl == 0 from the struct being zero-init, so we don't create the synthetic vol parent from the pcm feature unit. I don't have enough hardware available to feel confident about poking that hornet's nest.

Writing a test for this seems to be a little harder than I anticipated, but I'm also wondering now if deleting the knotes was infact the right thing to do. From revoke(2):