Disable bhyve HDA debug by default.

[madpilot]

Summary

At present the DEBUG_HDA knob is forced turned in bhyve, causing output to /tmp/bhyve_hda.log unconditionally.

I noticed this because I happened to fill /tmp.

My opinion is that this is not correct behaviour for production systems, so I produced a patch to make DEBUG_HDA disabled by default.

While here I also propose to introduce a define HDA_DEBUG_FILE (defaulting to the same /tmp/bhyve_hda.log file used at present) to change the output path.

The other changes related to hda_print_cmd_ctl_data() are needed to make the build suceed whan DEBUG_HDA is off. Otherwise clang errors out about unused argument, since the function resolves to an empty one when DEBUG_HDA is off.

Test Plan

I'm running with this patch and everything works as expected.

When DEBUG_HDA is turned on the actual code is really unchanged from before.

[corvink]

usr.sbin/bhyve/pci_hda.c

732–733

I don't like using ifdef's everywhere. Instead I'd prefer converting DPRINTF into a nop like https://github.com/freebsd/freebsd-src/blob/a1db1097e654272c1af0a1e0581b4a8deb75f168/usr.sbin/bhyve/pci_ahci.c#L119-L124

[madpilot]

usr.sbin/bhyve/pci_hda.c

732–733

It's already like that (see usr.sbin/bhyve/pci_hda.h), but with that this function resolves to an empty one and clang errors out due to the argument being unused.

I worked around that removing the function.

I could try leaving the const char *name = p->name; line unguarder but it looks even worse IMHO.

[corvink]

usr.sbin/bhyve/pci_hda.c

732–733

We could avoid the other ifdef's by declaring this as an empty function:

#if DEBUG_HDA == 1
static inline void
hda_print_cmd_ctl_data(struct hda_codec_cmd_ctl *p)
{
	DPRINTF("%s size: %d", p->name, p->size);
	DPRINTF("%s dma_vaddr: %p", p->name, p->dma_vaddr);
	DPRINTF("%s wp: 0x%x", p->name, p->wp);
	DPRINTF("%s rp: 0x%x", p->name, p->rp);
}
#else
static inline void
hda_print_cmd_ctl_data(struct hda_codec_cmd_ctl *p __unused) {}
#endif

[madpilot]

I'll also change the HDA_DEBUG_FILE define to DEBUG_HDA_FILE, to follow the naming convention of the existing one.

usr.sbin/bhyve/pci_hda.c

732–733

Good idea, I like your proposal, I'll update the patch accordingly.

[madpilot]

Updated code as suggested by corvink.

Also renamed HDA_DEBUG_FILE to DEBUG_HDA_FILE to follow naming of the DEBUG_HDA define.

[corvink]

One general note: Try to split your commits as much as possible. It makes reviewing and bug hunting much easier. So, it's better to create a commit for your DEBUG_HDA change and a second one for your DEBUG_HDA_FILE change.

[madpilot]

Split in two commits.

Also updated the commit messages to what I'd actually use.

I'd like to commit this myself to src, but since I have no src commit bit I need explicit approval for that, so I want to be sure I'm doing everything right.

Thaniks for your help and feedback!

[corvink]

LGTM.

usr.sbin/bhyve/pci_hda.c

732–733

You can keep the temporary variable const char *name = p->name; for a smaller diff. However, this depends on your preferences.

If you want to change it, do it when committing the patch. No need to update this review.