sound: remove PCM_KLDSTRING() and fix status strings

[christos]

Summary

PCM_KLDSTRING() prints the kernel module associated with a given audio
device only when that module is not compiled in. Get rid of
PCM_KLDSTRING() altogether and print the driver name (even for modules
that are compiled in) instead, as it implies the module as well.

While here, convert all status strings to the following dmesg-like
format:

[<port|mem> <irq>] on <driver>

Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks

Test Plan

Old:

pcm0:mixer: <Focusrite Scarlett Solo USB> at uaudio0 kld snd_uaudio (play/rec) (default)

New:

pcm0:mixer: <Focusrite Scarlett Solo USB> on uaudio0 (play/rec) (default)

[markj]

sys/dev/sound/pcm/sound.h

601

Probably should change "kld" to "module" then. "kld" specifically means a loadable kernel module.

[christos]

sys/dev/sound/pcm/sound.h

601

Another option is to make it a bit cleaner and get rid of the "module" or "kld" string altogether. This way we can make the string passed to pcm_setstatus() more concise (will require to modify all related calls, but they are not that many anyway), something like at snd_uaudio/uaudio0, instead of at uaudio0 module snd_uaudio.

What do you think?

[markj]

sys/dev/sound/pcm/sound.h

601

So the idea is to change the focus from the KLD name to the driver name. Seems fine to me.

[christos]

sys/dev/sound/pcm/sound.h

601

The idea is to make the "status" string a bit more concise and easy to read (e.g in mixer(8)):

From:

pcm4:mixer: <Realtek ALC662 rev3 (Analog 2.0+HP/2.0)> on hdaa1 module snd_hda [...]

To:

pcm4:mixer: <Realtek ALC662 rev3 (Analog 2.0+HP/2.0)> on snd_hda/hdaa1 [...]

[christos]

Remove PCM_KLDSTRING() completely, change string format slightly.

[markj]

I'm ok with this, aside from the comment.

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

"snd_als4000/io" doesn't really make sense. Here "io" is referring to the method that the driver is using to access the device's register interface. "io" means that it's using x86 I/O ports, whereas "memory" means memory-mapped I/O. The "at" is referring to the locations of the port or mappings, it has nothing to do with the driver.

The original format is fine, or you could put the driver name before the "at".

[christos]

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

I think it's good to have a uniform style for all devices, so that it's simpler for scripts that might want this information. Personally, I'd get rid of the "at" altogether and just make the strings as <module>/<driver> and <module>/<io|memory>/<irq>, or something like that.

[christos]

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

Scratch that. It might just be better to follow the rest of the driver's format, like in dmesg: on <driver> and at <io|mem> <irq> on <driver>. We can get rid of the kernel module name altogether since the module is implied by <driver> after all. Is there a reason those devices do not include the driver name here by the way?

[christos]

Improve format.

[imp]

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

Ideally, it would use the same methods that the other nwebus drivers use.
So, if we look at nvme drive:
nvme7: <Generic NVMe Device> mem 0xddc20000-0xddc23fff irq 150 at device 0.0 numa-domain 3 on pci25
111111: 222222222222222222 33333333333333333333333333333 at 444444444444444 on 55555

We have 5 parts

1: Device name from device_get_nameunit()
2: Description from device_get_desc()
3: printed by the bus. PCI prints io, mem, and irq in that order, prefixed by "port" "mem" or "irq". I don't think there's a generic way to get this string.
4: "at" followed by device_get_location()

optionally followed by 'numa-domain X' if we can BUS_GET_DOMAIN, though there's no generic way to get this string either

5: on device_get_nameunit(parent(dev))

So. this confuses the 'at' meaning of other drivers. No other driver in the system has "at <resources>" though the very old dmesg format from 4.4BSD might have (we changed all this in FreeBSD 3.2 with newbus, but not everything got changed at the time, including sound). If you want to match dmesg, it should be something closer to snd_als4000: port 0xPPP irq DDD. I'd do this everywhere unless there's programs that parse this status string for some reason... I'd also consider making this a interface (either .m or function call) that sound drivers just call....

This is an area that's been mostly, but not quite completely, cleaned up and is in need of a cleanup though.

And yea, I know this feedback may be expanding the scope of the review...

[emaste]

We'll want to think about relnotes and MFCs for these changes, since there are probably scripts, bug reports, guides, handbook examples etc. that will need to adapt.

IMO this will all be good to get into stable/14 (and probably not stable/13).

[christos]

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

I think it's a bit overkill to include all this information in the status string, since this is part of mixer(8)'s output for each device it lists, and so we shouldn't clutter it too much IMHO. I think the current information is fine as long as "io" is translated to "port".

[christos]

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

Alternatively we could add everything to the status string and let userland filter (e.g mixer(8)) filter out whatever it doesn't want.

[christos]

s/io/port/

[imp]

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

I'm good including a subset... My comments were about what we do generally with demsg output, and this is a subset of that used in a slightly different context. I also think it might be overkill to include everything. A lot of the description of I/O address dates from the ISA sound era and was just carried over to the PCI era. IT can be useful to disambiguate, but we already should have a driver + unit number to do that...

Also, since it is another context, if there's a good reason to deviate from the form, that's fine too. I got a little carried away describing what we have today in other demsg lines, but doing a poor job of qualifying that wasn't necessarily to dictate that you must follow it in all its verbosity.

[christos]

sys/dev/sound/pci/als4000.c

850 ↗

(On Diff #132399)

Do you think these devices here still need to list port/mem and irq in the status string (again, this is what mixer(8) prints)? Maybe it's better to just bus_print_child_header() + print port/mem, irq + bus_print_child_footer() so that this is info printed in dmesg instead and we can keep the status string clean with just the driver name, as in snd_hda and snd_uaudio.

[christos]

@imp Are you good with this so I can commit it with the other patches all at once?

[christos]

This has already been committed, but Phabricator doesn't close it for some reason.