Page MenuHomeFreeBSD
Differential D46749

sound: Move global variable initialization to sound_modevent()
ClosedPublic
Actions

Authored by christos on Sep 22 2024, 3:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 11, 2:21 PM
Unknown Object (File)
Fri, Mar 28, 11:22 PM
Unknown Object (File)
Thu, Mar 20, 2:58 AM
Unknown Object (File)
Mar 10 2025, 7:26 AM
Unknown Object (File)
Mar 7 2025, 8:14 AM
Unknown Object (File)
Feb 21 2025, 2:39 PM
Unknown Object (File)
Feb 20 2025, 2:12 PM
Unknown Object (File)
Feb 20 2025, 1:49 PM
View All 39 Files
Subscribers
imp
zlei

Details

Reviewers
markj
emaste
zlei
Commits
rG6cf4db422c21: sound: Move global variable initialization to sound_modevent()
rG0a0301deb5b6: sound: Move global variable initialization to sound_modevent()
Summary

There is no reason to initialize global variables in feeder_register(),
as these variables are unrelated to what the function does. Instead,
initialize them during sound(4) load.

While here, fix the switch statement indentation in sound_modevent().

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59633
Build 56520: arc lint + arc unit

Event Timeline

christos created this revision.Sep 22 2024, 3:32 PM
Herald added a subscriber: imp. · View Herald TranscriptSep 22 2024, 3:32 PM
christos requested review of this revision.Sep 22 2024, 3:32 PM
Harbormaster completed remote builds in B59561: Diff 143616.Sep 22 2024, 3:32 PM
zlei added a subscriber: zlei.Sep 24 2024, 10:05 AM
zlei added inline comments.
sys/dev/sound/pcm/feeder.c
66

This can also be moved to event handler of MOD_LOAD IMHO.

69

For the initialization of root feeder, it is special, I guess it deserves a dedicated function, aka feeder_register_root(). Then change the SYSINIT accordingly,

-SYSINIT(feeder_root, SI_SUB_DRIVERS, SI_ORDER_FIRST, feeder_register, &feeder_root_class);
+SYSINIT(feeder_root, SI_SUB_DRIVERS, SI_ORDER_FIRST, feeder_register_root, &feeder_root_class);
sys/dev/sound/pcm/sound.c
895

I'd support you move the initialization of those global variables into a dedicated function. It will be much clear IMHO.

christos marked 3 inline comments as done.Sep 27 2024, 5:55 PM
christos added inline comments.
sys/dev/sound/pcm/feeder.c
66

This list is internal to feeder.c, so I will just move it to the new feeder_register_root() function.

69

D46821

christos updated this revision to Diff 143822.Sep 27 2024, 5:55 PM
christos marked 2 inline comments as done.

Move to separate function.

Harbormaster completed remote builds in B59633: Diff 143822.Sep 27 2024, 5:55 PM
zlei accepted this revision.Sep 29 2024, 3:02 AM

Looks good to me.

This revision is now accepted and ready to land.Sep 29 2024, 3:02 AM
emaste added a comment.Sep 30 2024, 2:46 PM

I'd probably use the full word global rather than glob, as glob has an existing meaning (even if not related to this context) and it's only two extra letters.

The whitespace fixup should be a separate commit, that can go first.

zlei added a comment.Sep 30 2024, 2:53 PM
In D46749#1068241, @emaste wrote:

I'd probably use the full word global rather than glob, as glob has an existing meaning (even if not related to this context) and it's only two extra letters.

The whitespace fixup should be a separate commit, that can go first.

+1 for global.

christos updated this revision to Diff 144068.Oct 1 2024, 1:53 AM
  • s/sound_glob_init()/sound_global_init()/
  • Move indendation fix to separate patch: D46856
This revision now requires review to proceed.Oct 1 2024, 1:53 AM
Harbormaster completed remote builds in B59666: Diff 144068.Oct 1 2024, 1:53 AM
christos added a parent revision: D46856: sound: Fix indendation in sound_modevent() switch statement.Oct 1 2024, 1:54 AM
zlei accepted this revision.Oct 1 2024, 10:36 AM
This revision is now accepted and ready to land.Oct 1 2024, 10:36 AM
Closed by commit rG0a0301deb5b6: sound: Move global variable initialization to sound_modevent() (authored by christos). · Explain WhyOct 18 2024, 8:45 AM
This revision was automatically updated to reflect the committed changes.
christos added a commit: rG0a0301deb5b6: sound: Move global variable initialization to sound_modevent().
christos added a commit: rG6cf4db422c21: sound: Move global variable initialization to sound_modevent().Oct 20 2024, 11:22 AM

Revision Contents
Changeset List

PathSize
sys/
dev/
sound/
pcm/
43 lines
115 lines

Diff 143822

View Options

sys/dev/sound/pcm/feeder.c

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines
static uint8_t cpu_cst_cnt; /* Indicate we are _CST aware. */ static uint8_t cpu_cst_cnt; /* Indicate we are _CST aware. */
static int cpu_quirks; /* Indicate any hardware bugs. */ static int cpu_quirks; /* Indicate any hardware bugs. */
/* Values for sysctl. */ /* Values for sysctl. */
static struct sysctl_ctx_list cpu_sysctl_ctx; static struct sysctl_ctx_list cpu_sysctl_ctx;
static struct sysctl_oid *cpu_sysctl_tree; static struct sysctl_oid *cpu_sysctl_tree;
static int cpu_cx_generic; static int cpu_cx_generic;
static int cpu_cx_lowest_lim; static int cpu_cx_lowest_lim;
static bool cppc_notify;
static struct acpi_cpu_softc **cpu_softc; static struct acpi_cpu_softc **cpu_softc;
ACPI_SERIAL_DECL(cpu, "ACPI CPU"); ACPI_SERIAL_DECL(cpu, "ACPI CPU");
static int acpi_cpu_probe(device_t dev); static int acpi_cpu_probe(device_t dev);
static int acpi_cpu_attach(device_t dev); static int acpi_cpu_attach(device_t dev);
static int acpi_cpu_suspend(device_t dev); static int acpi_cpu_suspend(device_t dev);
static int acpi_cpu_resume(device_t dev); static int acpi_cpu_resume(device_t dev);
▲ Show 20 Lines • Show All 227 Lines • ▼ Show 20 Lines
#if defined(__i386__) || defined(__amd64__) #if defined(__i386__) || defined(__amd64__)
/* /*
* Ask for MWAIT modes if not disabled and interrupts work * Ask for MWAIT modes if not disabled and interrupts work
* reasonable with MWAIT. * reasonable with MWAIT.
*/ */
if (!acpi_disabled("mwait") && cpu_mwait_usable()) if (!acpi_disabled("mwait") && cpu_mwait_usable())
sc->cpu_features |= ACPI_CAP_SMP_C1_NATIVE | ACPI_CAP_SMP_C3_NATIVE; sc->cpu_features |= ACPI_CAP_SMP_C1_NATIVE | ACPI_CAP_SMP_C3_NATIVE;
/*
* Work around a lingering SMM bug which leads to freezes when handling
* CPPC notifications. Tell the SMM we will handle any CPPC notifications.
*/
if ((cpu_power_eax & CPUTPM1_HWP_NOTIFICATION) && cppc_notify)
sc->cpu_features |= ACPI_CAP_INTR_CPPC;
#endif #endif
jhbUnsubmitted
Not Done
Inline Actions

I would maybe call the hint cppc_notify with the sense being that "1" enables it and "0" disables notifications (we try to avoid "negative" hints/tunables). You would then just want to set the default value to 1 and change the last test to cppcval != 0. I would also perhaps rename cppcval to cppc_notify as well.

jhb: I would maybe call the hint `cppc_notify` with the sense being that "1" enables it and "0"…
if (devclass_get_drivers(device_get_devclass(dev), &drivers, if (devclass_get_drivers(device_get_devclass(dev), &drivers,
&drv_count) == 0) { &drv_count) == 0) {
for (i = 0; i < drv_count; i++) { for (i = 0; i < drv_count; i++) {
if (ACPI_GET_FEATURES(drivers[i], &features) == 0) if (ACPI_GET_FEATURES(drivers[i], &features) == 0)
sc->cpu_features |= features; sc->cpu_features |= features;
} }
free(drivers, M_TEMP); free(drivers, M_TEMP);
▲ Show 20 Lines • Show All 563 Lines • ▼ Show 20 Lines if ((sc = cpu_softc[i]) != NULL)
acpi_cpu_startup_cx(sc); acpi_cpu_startup_cx(sc);
} }
/* Add a sysctl handler to handle global Cx lowest setting */ /* Add a sysctl handler to handle global Cx lowest setting */
SYSCTL_ADD_PROC(&cpu_sysctl_ctx, SYSCTL_CHILDREN(cpu_sysctl_tree), SYSCTL_ADD_PROC(&cpu_sysctl_ctx, SYSCTL_CHILDREN(cpu_sysctl_tree),
OID_AUTO, "cx_lowest", CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_MPSAFE, OID_AUTO, "cx_lowest", CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_MPSAFE,
NULL, 0, acpi_cpu_global_cx_lowest_sysctl, "A", NULL, 0, acpi_cpu_global_cx_lowest_sysctl, "A",
"Global lowest Cx sleep state to use"); "Global lowest Cx sleep state to use");
/* Add sysctl handler to control registering for CPPC notifications */
cppc_notify = 1;
SYSCTL_ADD_BOOL(&cpu_sysctl_ctx, SYSCTL_CHILDREN(cpu_sysctl_tree),
OID_AUTO, "cppc_notify", CTLFLAG_RDTUN | CTLFLAG_MPSAFE,
&cppc_notify, 0, "Register for CPPC Notifications");
/* Take over idling from cpu_idle_default(). */ /* Take over idling from cpu_idle_default(). */
cpu_cx_lowest_lim = 0; cpu_cx_lowest_lim = 0;
CPU_FOREACH(i) { CPU_FOREACH(i) {
if ((sc = cpu_softc[i]) != NULL) if ((sc = cpu_softc[i]) != NULL)
enable_idle(sc); enable_idle(sc);
} }
#if defined(__i386__) || defined(__amd64__) #if defined(__i386__) || defined(__amd64__)
▲ Show 20 Lines • Show All 525 Lines • Show Last 20 Lines
View Options

sys/dev/sound/pcm/sound.c

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines
static uint8_t cpu_cst_cnt; /* Indicate we are _CST aware. */ static uint8_t cpu_cst_cnt; /* Indicate we are _CST aware. */
static int cpu_quirks; /* Indicate any hardware bugs. */ static int cpu_quirks; /* Indicate any hardware bugs. */
/* Values for sysctl. */ /* Values for sysctl. */
static struct sysctl_ctx_list cpu_sysctl_ctx; static struct sysctl_ctx_list cpu_sysctl_ctx;
static struct sysctl_oid *cpu_sysctl_tree; static struct sysctl_oid *cpu_sysctl_tree;
static int cpu_cx_generic; static int cpu_cx_generic;
static int cpu_cx_lowest_lim; static int cpu_cx_lowest_lim;
static bool cppc_notify;
static struct acpi_cpu_softc **cpu_softc; static struct acpi_cpu_softc **cpu_softc;
ACPI_SERIAL_DECL(cpu, "ACPI CPU"); ACPI_SERIAL_DECL(cpu, "ACPI CPU");
static int acpi_cpu_probe(device_t dev); static int acpi_cpu_probe(device_t dev);
static int acpi_cpu_attach(device_t dev); static int acpi_cpu_attach(device_t dev);
static int acpi_cpu_suspend(device_t dev); static int acpi_cpu_suspend(device_t dev);
static int acpi_cpu_resume(device_t dev); static int acpi_cpu_resume(device_t dev);
▲ Show 20 Lines • Show All 227 Lines • ▼ Show 20 Lines
#if defined(__i386__) || defined(__amd64__) #if defined(__i386__) || defined(__amd64__)
/* /*
* Ask for MWAIT modes if not disabled and interrupts work * Ask for MWAIT modes if not disabled and interrupts work
* reasonable with MWAIT. * reasonable with MWAIT.
*/ */
if (!acpi_disabled("mwait") && cpu_mwait_usable()) if (!acpi_disabled("mwait") && cpu_mwait_usable())
sc->cpu_features |= ACPI_CAP_SMP_C1_NATIVE | ACPI_CAP_SMP_C3_NATIVE; sc->cpu_features |= ACPI_CAP_SMP_C1_NATIVE | ACPI_CAP_SMP_C3_NATIVE;
/*
* Work around a lingering SMM bug which leads to freezes when handling
* CPPC notifications. Tell the SMM we will handle any CPPC notifications.
*/
if ((cpu_power_eax & CPUTPM1_HWP_NOTIFICATION) && cppc_notify)
sc->cpu_features |= ACPI_CAP_INTR_CPPC;
#endif #endif
jhbUnsubmitted
Not Done
Inline Actions

I would maybe call the hint cppc_notify with the sense being that "1" enables it and "0" disables notifications (we try to avoid "negative" hints/tunables). You would then just want to set the default value to 1 and change the last test to cppcval != 0. I would also perhaps rename cppcval to cppc_notify as well.

jhb: I would maybe call the hint `cppc_notify` with the sense being that "1" enables it and "0"…
if (devclass_get_drivers(device_get_devclass(dev), &drivers, if (devclass_get_drivers(device_get_devclass(dev), &drivers,
&drv_count) == 0) { &drv_count) == 0) {
for (i = 0; i < drv_count; i++) { for (i = 0; i < drv_count; i++) {
if (ACPI_GET_FEATURES(drivers[i], &features) == 0) if (ACPI_GET_FEATURES(drivers[i], &features) == 0)
sc->cpu_features |= features; sc->cpu_features |= features;
} }
free(drivers, M_TEMP); free(drivers, M_TEMP);
▲ Show 20 Lines • Show All 563 Lines • ▼ Show 20 Lines if ((sc = cpu_softc[i]) != NULL)
acpi_cpu_startup_cx(sc); acpi_cpu_startup_cx(sc);
} }
/* Add a sysctl handler to handle global Cx lowest setting */ /* Add a sysctl handler to handle global Cx lowest setting */
SYSCTL_ADD_PROC(&cpu_sysctl_ctx, SYSCTL_CHILDREN(cpu_sysctl_tree), SYSCTL_ADD_PROC(&cpu_sysctl_ctx, SYSCTL_CHILDREN(cpu_sysctl_tree),
OID_AUTO, "cx_lowest", CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_MPSAFE, OID_AUTO, "cx_lowest", CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_MPSAFE,
NULL, 0, acpi_cpu_global_cx_lowest_sysctl, "A", NULL, 0, acpi_cpu_global_cx_lowest_sysctl, "A",
"Global lowest Cx sleep state to use"); "Global lowest Cx sleep state to use");
/* Add sysctl handler to control registering for CPPC notifications */
cppc_notify = 1;
SYSCTL_ADD_BOOL(&cpu_sysctl_ctx, SYSCTL_CHILDREN(cpu_sysctl_tree),
OID_AUTO, "cppc_notify", CTLFLAG_RDTUN | CTLFLAG_MPSAFE,
&cppc_notify, 0, "Register for CPPC Notifications");
/* Take over idling from cpu_idle_default(). */ /* Take over idling from cpu_idle_default(). */
cpu_cx_lowest_lim = 0; cpu_cx_lowest_lim = 0;
CPU_FOREACH(i) { CPU_FOREACH(i) {
if ((sc = cpu_softc[i]) != NULL) if ((sc = cpu_softc[i]) != NULL)
enable_idle(sc); enable_idle(sc);
} }
#if defined(__i386__) || defined(__amd64__) #if defined(__i386__) || defined(__amd64__)
▲ Show 20 Lines • Show All 525 Lines • Show Last 20 Lines