sound: Stop building midi as a module
ClosedPublic
Actions

Authored by christos on Mon, Dec 8, 2:38 PM.

Details

Reviewers

markj
emaste

Commits

rG45bb10e0ed5a: sound: Stop building midi as a module

Summary

There is no reason for this. The midi functions are used by drivers
on-demand anyway. Use SYSINIT(9) instead to do the appropriate
(de-)initilizations.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

christos created this revision.Mon, Dec 8, 2:38 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, Dec 8, 2:38 PM

christos requested review of this revision.Mon, Dec 8, 2:38 PM

Harbormaster completed remote builds in B69122: Diff 167706.Mon, Dec 8, 2:38 PM

But now unload errors can't be reported.

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.

christos added a child revision: D54126: sound: Retire midi_devs and mstat_lock.Mon, Dec 8, 3:32 PM

christos mentioned this in D54129: sound: Retire snd_midi->qlock.Thu, Dec 11, 11:37 AM

I don't think it is a good idea to not build this as a module because this code will not be used by a large majority of hosts (although personally I'll appreciate to use MIDI on FreeBSD!).

Loading and unloading soundcard modules is convenient. Furthermore, using OSS modules requires to build "sound" as a module only, so why not doing the same for MIDI?

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.

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.

In D54125#1237779, @dev_nicolas-provost.fr wrote:

I don't think it is a good idea to not build this as a module because this code will not be used by a large majority of hosts (although personally I'll appreciate to use MIDI on FreeBSD!).

Not building it as a module does not mean it will now be used more. In fact, it is right now now that this code is "used" without reason, because it gets automatically loaded with sound(4). Whereas, with this change, it will be executed only by the devices that support MIDI.

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.

This revision is now accepted and ready to land.Fri, Dec 12, 3:18 PM

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.

I know, the only reason was that I didn't want to create one big patch that does a bunch of things unrelated to each other. That being said, I can update the patch to handle the EBUSY case, do you think an assertion would be desirable here?

In D54125#1238083, @christos wrote:

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.

I know, the only reason was that I didn't want to create one big patch that does a bunch of things unrelated to each other. That being said, I can update the patch to handle the EBUSY case, do you think an assertion would be desirable here?

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

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.

Yes, they'll all get committed at once.

christos mentioned this in D54126: sound: Retire midi_devs and mstat_lock.Tue, Dec 16, 3:29 PM

Closed by commit rG45bb10e0ed5a: sound: Stop building midi as a module (authored by christos). · Explain WhyFri, Jan 2, 4:58 PM

This revision was automatically updated to reflect the committed changes.

christos added a commit: rG45bb10e0ed5a: sound: Stop building midi as a module.