In D55473#1307589, @guest-seuros wrote:

Can you land it please?

I don't have the hardware to test this on, but if it works for you, LGTM. Do you have a commit bit or should I commit it?

"sound: Fix lock order reversal in dsp_poll()"

LGTM. Please change the commit title from "dsp: Make witness happy" to "sound: Fix lock order reversal in chn_trigger()".

In D57009#1306470, @aokblast wrote:

In D57009#1306459, @christos wrote:

I understand that the change here locks the channels we actually want, instead of both of them. However, the effect is the same; if priv has both channels allocated, then your code will also lock both of them, just not at the same time. But I don't understand why doing this fixes the issue. Could you elaborate further?

I know it is a little bit hard to understand as I also spend hours to read witness output. Let me explain it with more details:

1. when calling chn_poll(rdch, e, td), we hold both the rdch and wrch locks at the same time.
2. In chn_trigger, we are expected to have lock order "dsp dev" -> "channel". chn_trigger switch the lock order by using chn_unlock -> pcm_lock -> chn_lock.

However, when chn_trigger(rdch) (called by chn_poll(rdch) try to switch the lock order. Witness detects another lock order reverse (wrch) that is unrelated to the call stack.
It is because wrch is also a channel lock and also called by chn_trigger previous - that is, it requires lock order "dsp dev" -> channel lock but we called pcm_lock without unlock wrch channel (the channel is wrch but we are handling chn_trigger(rdch)).

It can be stably reproduced on my laptop by using the following sequence:

1. Start pipewire
2. Start pipewire-pulse
3. Start pavucontrol

I understand that the change here locks the channels we actually want, instead of both of them. However, the effect is the same; if priv has both channels allocated, then your code will also lock both of them, just not at the same time. But I don't understand why doing this fixes the issue. Could you elaborate further?

I suppose you are referring to a seemingly "harmless" LOR coming from chn_trigger(). I've been aware of this for a while already. That being said, I don't see how this change fixes it. You are essentially doing the exact same thing.

Also in the commit message it should be "sound", not "dsp".

Please explain in a commit message what this change does. What does WITNESS complain about? Why are you changing the order of dsp_lock_chans()? And why did you remove the dsp_lock_chans() call in dsp_poll()?

Are there any updates we should do in D40874?

Note that this won't have any effect after a device driver has initialized. snd_mixerdefaults is only used in mixer_init(), so any change with these sysctls will require a driver re-load for the new defaults to take effect. I suppose this is not really an issue if these defaults are set and "forgotten" in a config file, though.

Many translations (e.g., German, Danish, Polish from a quick look) are actually in English.

Bump.

In D52325#1291455, @bz wrote:

In D52325#1291454, @christos wrote:

The patch does not apply cleanly on main:

Yes, it's 7 months old and didn't get much attention.
As said in email, I should rewrite some things and then really push to get it in.

The patch does not apply cleanly on main:

$ git arc patch D52325
Checking patch usr.sbin/fwget/usb/usb_wireless_ralink...
Checking patch usr.sbin/fwget/usb/usb_wireles_header...
Checking patch usr.sbin/fwget/usb/usb_ralink...
Checking patch usr.sbin/fwget/usb/usb_bluetooth_realtek...
Checking patch usr.sbin/fwget/usb/usb_bluetooth_qca...
Checking patch usr.sbin/fwget/usb/usb_bluetooth_intel...
Checking patch usr.sbin/fwget/usb/usb_bluetooth_header...
Checking patch usr.sbin/fwget/usb/usb_bluetooth_brcm...
Checking patch usr.sbin/fwget/usb/usb...
Checking patch usr.sbin/fwget/usb/Makefile...
Checking patch usr.sbin/fwget/pci/pci_wireless_realtek...
Checking patch usr.sbin/fwget/pci/pci_wireless_qca...
Checking patch usr.sbin/fwget/pci/pci_wireless_mediatek...
Checking patch usr.sbin/fwget/pci/pci_wireless_intel...
Checking patch usr.sbin/fwget/pci/pci_wireless_header...
Checking patch usr.sbin/fwget/pci/pci_video_intel...
Checking patch usr.sbin/fwget/pci/pci_video_header...
Checking patch usr.sbin/fwget/pci/pci_video_amd...
error: while searching for:
#-
# SPDX-License-Identifier: BSD-2-Clause
#
# Copyright 2023 Beckhoff Automation GmbH & Co. KG
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted providing that the following conditions
# are met:
# 1. Redistributions of source code must retain the above copyright
#    notice, this list of conditions and the following disclaimer.
# 2. Redistributions in binary form must reproduce the above copyright
#    notice, this list of conditions and the following disclaimer in the
#    documentation and/or other materials provided with the distribution.
#
# THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
# ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
# DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
# STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
# IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
# POSSIBILITY OF SUCH DAMAGE.

Fix.

Address Mark's comments.

In D54360#1291229, @meka_tilda.center wrote:

Given it works with umidi, should we close this? I think not as it works for me.

In D54359#1291226, @meka_tilda.center wrote:

Unification first, as this code is mostly "copy" of the umidi code. I think after unification one place will have the kqueue support and that will probably be copied over from umidi. That being said, do you want me to close this one?

In D54359#1291224, @meka_tilda.center wrote:

So, is it smart to work on this before umidi/midi unification? I can work on that, I just think it would be better if unification happened first.

In D54360#1291221, @meka_tilda.center wrote:

I understood that you need to unify umidi and midi, that's why I left this review as is. Please tell me if I got it wrong.

Let me know if you want me to take this over. No problem for me. :-)

Clarify virtual_oss_default_control_device further.