The commit mentioned refers to this: https://github.com/freebsd/freebsd-src/pull/1905

In D54130#1240411, @dev_nicolas-provost.fr wrote:

If M_RX gets unset, the device file was closed, so no read will occur anymore and draining or keeping data is useless. I made a successful test with this extra lock/unlock.

How do we arrive at 0.4ms sleep though? What is the logic behind it? Is it just because it seems to work on your setup, or is there some calculation you used to arrive at it?

In D54130#1239510, @dev_nicolas-provost.fr wrote:

I see. You may only have to check for M_RX inside the loop and not at line 433 (file closed), in addition to the extra lock/unlock.

In D54126#1239514, @dev_nicolas-provost.fr wrote:

I suggest you to commit D54129 and my mpu_init changes (+changes here) so that we can make proper tests and incremental changes over it more simply.

In D54129#1239515, @dev_nicolas-provost.fr wrote:

I've no doubt now that MIDI will work, although my set of patches is slightly different from yours, but please make incremental patches over this one, so we can see what is changed!

In D54164#1239214, @ivy wrote:

but this suggests it's safe to use make installkernel if you're using INSTKERNNAME, which isn't true. it may be true, but it also may be safe to use it without INSTKERNNAME set at all - it depends on what you set it to and what pkgbase kernels you have installed.

i would rather add a variable to disable this check (other than DESTDIR), like DISABLE_INSTALLKERNEL_PKG_CHECK, which people could put in /etc/make.conf if they're sure they understand the implications of doing this. alternatively, perhaps we could check if /boot/${INSTKERNNAME} was installed by pkg.

In D54130#1239116, @dev_nicolas-provost.fr wrote:

I'm not convinced we have to unlock here, but this may give a breathe to the interrupt, so why not (during my last test I found too much latency but I don't know yet if it is relative to my userspace code or not).

In D54130#1239093, @dev_nicolas-provost.fr wrote:

Still not sure this is good. It is working as-it for me; uiomove will not sleep if the lock is taken just above!

In D54129#1239062, @dev_nicolas-provost.fr wrote:

For the midi module, I think the right path is:

midi_unload -> midi_uninit -> mpu401_uninit (MPU_UNINIT) then midi_destroy.

mpu401_uninit should only do the hardware uninit, that is, send the MPU reset command.

In D54126#1239049, @dev_nicolas-provost.fr wrote:

Well, I tried to create a bugzilla account last week, but could not manage to get a password!

In D54129#1239035, @dev_nicolas-provost.fr wrote:

To fix the termination problem I mentioned above, I think you can simply remove the midi_uninit call in mpu401_uninit (if you want to keep this as a module).

In D54126#1239027, @dev_nicolas-provost.fr wrote:

Sorry, I can apply diff 54125 but no 54126 over it ?

Bump.

Will combine with other patch.

In D54102#1236569, @christos wrote:

In D54102#1236568, @markj wrote:

What problem does this fix?

From the commit referenced in "Fixes" (5cc34a83e1):

It turns out that snd_uaudio(4) uses sound(4)'s channel lock for its USB
transfer callbacks. I will try to address this at some point, because
this is layering violation, but for now we need to revert the commit, as
it causes a lock recursion panic with USB audio devices.

@dev_nicolas-provost.fr can you test again now? Are channel numbers allocated properly?

Reflect changes in D54126.

Do what I said in my last comment and allocate a unique channel number with
unr(9) as well.

In D54125#1238087, @markj wrote:

I don't think it's worth the effort, presumably you plan to commit everything together? Mine was more of a general comment.

In D48763#1237792, @bz wrote:

In D48763#1237632, @christos wrote:

What about this with regards to the drm-kmod upgrade to 6.10?

This is generally interesting but not relevant for any wifi or graphics updates.

In D54125#1238066, @markj wrote:

In D54125#1238064, @christos wrote:

In D54125#1238042, @markj wrote:

In D54125#1236599, @christos wrote:

In D54125#1236580, @markj wrote:

But now unload errors can't be reported.

They will be reported by the driver or sound(4). This here functions essentially like pcm/sndstat.c, we just offer some functions to the drivers, this is not supposed to be a separate module, at least at this state.

Are you saying that the EBUSY case in midi_sysuninit() can't happen? If so, it should be an assertion, if not then the change looks wrong since we are leaking objects in midi_devs.

I eventually remove this in D54126, I just didn't want to make this a single patch because D54126 includes other changes which are not really related to this one.

Ok, please consider though that patches should be self-contained. This patch introduces a bug and the next one fixes it. It would be better to just combine them.

In D54126#1237784, @dev_nicolas-provost.fr wrote:

Don't forget that some cards, as my common Soundblaster, have two ports; they actually appear as /dev/midi0.0 and /dev/midi0.1, which is useful to know that they belong to the same card.

Using more than one card could make difficult to find which port is tied to which card (although the devices discovery may be always done in the same order ?).

In D54125#1238042, @markj wrote:

In D54125#1236599, @christos wrote:

In D54125#1236580, @markj wrote:

But now unload errors can't be reported.

They will be reported by the driver or sound(4). This here functions essentially like pcm/sndstat.c, we just offer some functions to the drivers, this is not supposed to be a separate module, at least at this state.

Are you saying that the EBUSY case in midi_sysuninit() can't happen? If so, it should be an assertion, if not then the change looks wrong since we are leaking objects in midi_devs.

Address Mark's comments.

What about this with regards to the drm-kmod upgrade to 6.10?

In D54129#1237624, @dev_nicolas-provost.fr wrote:

You can compare the body of my midi.c/midi_read function with yours. What is difficult with this file is that there is a lot of small changes to do everywhere.

I'll test my mods using some MIDI software I'm also working on. Then, if I do not see some bug anymore, I'll try to integrate these changes over yours.

So if you apply this patch to a clean branch, the panic goes away, right? At least that's the behavior on my system.

Address Mark's comment.

Do you trigger this panic with this patch? I cannot reproduce it in blocking mode. Are you talking about the current state of the code?

In D54129#1237597, @dev_nicolas-provost.fr wrote:

Currently, opening the midi device in blocking mode triggers a kernel panic (line 442), but opening with O_NONBLOCK works well. Furthermore I think the data queues do not work properly (midiq.h).

To fix this and some other problems, I'm currently rewriting small parts of this file.

Linux defines eventfd_ctx as:

struct eventfd_ctx {
	struct kref kref;
	wait_queue_head_t wqh;
	/*
	 * Every time that a write(2) is performed on an eventfd, the
	 * value of the __u64 being written is added to "count" and a
	 * wakeup is performed on "wqh". If EFD_SEMAPHORE flag was not
	 * specified, a read(2) will return the "count" value to userspace,
	 * and will reset "count" to zero. The kernel side eventfd_signal()
	 * also, adds to the "count" counter and issue a wakeup.
	 */
	__u64 count;
	unsigned int flags;
	int id;
};

This is quite different from our version. Although ugly, wouldn't it be safer to either explicitly alias it in LinuxKPI or even translate it there?

Abandoned D54134. Update.

LGTM. Please take a look at the inline comments.

In D54125#1236580, @markj wrote:

But now unload errors can't be reported.

In D54102#1236568, @markj wrote:

What problem does this fix?

Bump.

Refer to xo_options(7), not libxo(3).

We can have this check for SNDCTL_DSP_CHANNELS as well. Also the commit title and message have to be adapted.

You can extend this to the rest of the settings in this patch.

Update man page as well.

Bump. Again, this is problematic when running with -v parallelism because it might unload snd_dummy when the other tests are using it.

Are you working on the timing issues? I haven't really looked into those.

I think this needs to be rebased since we committed the oss.h changes.