sound: Retire snd_midi->qlock

[christos]

Summary

snd_midi->qlock is used to protect snd_midi->{in,out}q. However, apart
from the numerous LORs present already in the code, there is no reason
not to use snd_midi->lock, as we do for the rest of the structure

Started by: https://github.com/freebsd/freebsd-src/pull/1902
Sponsored by: The FreeBSD Foundation
MFC after: 1 week

[dev_nicolas-provost.fr]

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.

[christos]

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.

Can you share the panic message as well as how you reproduce this?

[christos]

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?

[dev_nicolas-provost.fr]

I triggered the kernel panic using my changes from the PR1902 from github.

Anyway, it is now working with my last updates. For more clarity, you'll find all current changes to midi.c and mpu401.c at https://github.com/n-p-soft/freebsd-src/tree/midi-all/sys/dev/sound/midi, because they overlap with yours:

• changes to MPU-401 published in #1905
• remove MPU_CALLBACKP from the MPU interface (same as MPU_CALLBACK)
• changes to midi.c published in #1902 (locks) with additional fixes
• midi.c: fix a kernel panic when opening MIDI device in blocking mode
• midi.c: remove useless 'busy' field in MIDI device data
• midi.c: silently ignore errors when closing device (ex: process crash)
• midi.c: correct how to unload MIDI device when unloading a driver
• midi.c: clarify when rchan/wchan are set to 1 or 0
• midi.c: ignore return value of midi_destroy, simplify midi_uninit (called from MPU)

MIDI seems to work correctly with these patches (currently testing).
Feel free to incorporate whatever you want.

[christos]

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

Also, regarding the patches in your branch, it would be very good to split them into individual patches, one for each logically-related functionality, and submit them as pull requests. If my qlock patch here works, then skip that one when submitting the pull requests.

[dev_nicolas-provost.fr]

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.

[christos]

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.

Your midi_read() will produce an error during the uiomove() because you keep holding m->lock which is non-sleepable. Apart from that the functionality looks largely the same, barring the fact that you dropped the PDROP flag in msleep(). But we can continue this discussion in the appropriate place. I would suggest you apply this patch to your tree, and also split your patches into logically-related ones, rather than a big one that does a bunch of things at once. I'll be happy to review those.

I have a few more pending MIDI patches:
D54125, D54126, D54130, D54131

[dev_nicolas-provost.fr]

Sorry for duplicating my message on Github for #1904, I found this patch (D54129) to be working on top of FreeBSD 15.0 release source, with my last update for #1904 (mpu_init.c), and the MIDI enabled for my Audigy2 soundcard (#1905).

The only problem I've found is that I cannot properly shutdown the machine (hangs after "Terminated").

[dev_nicolas-provost.fr]

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

[christos]

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

Although this is not related to this patch, your proposal is not correct. midi_uninit() does all the freeing. If we do not call it, then it won't get called anywhere. I'm not sure your hang is related to this.

[dev_nicolas-provost.fr]

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.

[christos]

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.

midi_unload() won't exist once D54125 is committed, so the devices have to get destroyed by the driver, which is normal. We want to destroy the device when the driver wants to detach, so the current design is correct IMHO. midi_destroy() also is getting retired, see D54192.

[dev_nicolas-provost.fr]

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!

[christos]

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!

This is an incremental patch, it changes only 1 thing... You can see the stack of patches and apply them accordingly.