Page MenuHomeFreeBSD

acpi_spmc(4): Human-readably print supported DSMs and their functions
ClosedPublic

Authored by olce on May 1 2026, 5:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 18, 5:04 AM
Unknown Object (File)
Wed, Jun 10, 3:33 AM
Unknown Object (File)
Tue, Jun 9, 6:08 AM
Unknown Object (File)
Fri, Jun 5, 4:16 AM
Unknown Object (File)
Fri, Jun 5, 12:23 AM
Unknown Object (File)
Thu, Jun 4, 10:05 PM
Unknown Object (File)
Thu, Jun 4, 8:53 AM
Unknown Object (File)
Wed, Jun 3, 8:33 PM
Subscribers

Details

Summary

To this end, revamp how DSMs and their functions are represented.
Replace enumerations, which only bring minimal advantages, with plain
macros, allowing to get rid of 'union dsm_index' and in passing the associated bug
that an underlying type smaller than 'int' is aliased to an 'int', which
assumes little-endian architectures. Associate to each function
a printable name, in the form of a per-DSM array that maps a function
index into a string. Make sure that every used array and their number
of items are sized at compile-time and are declared constant, and that
as little code as possible depends on the particular set of present DSMs
and associated functions.

Since the set of DSMs and sets of per-DSM functions are represented as
bitsets, introduce print_bit_field() to print such sets. This new
function is akin to printf("%b", ...) but with more flexibility. It
takes a function associating a name to some bit index and an opaque
pointer, allowing to leverage existing structures containing names
instead of imposing the use of a separate string containing all names to
be printed. It also provides a default name to bits without an explicit
name, composed of a common prefix and the bit index as a suffix.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

olce requested review of this revision.May 1 2026, 5:13 PM
olce planned changes to this revision.May 4 2026, 2:36 PM

Too many changes entangled here, splitting this to several commits.

olce edited the summary of this revision. (Show Details)

Disentangle all the changes (put in parent revisions).

This revision is now accepted and ready to land.May 7 2026, 2:40 AM
olce edited the summary of this revision. (Show Details)
  • Impacts of D56802's update
  • When printing the DSMs, don't show the numerical value of our DSM present mask, it is purely internal and cannot be interpreted from the outside.
  • acpi_spmc_dsm_check_functions() => acpi_spmc_dsm_print_functions(): We are printing supported functions as diagnostics, but we always continue.
  • Access a DSM in the dsms[] array through a new function resolve_dsm() to be able to perform an out-of-bound check on INVARIANTS.
This revision now requires review to proceed.May 7 2026, 7:57 PM

i haven't entirely finished reviewing this, but here's a few comments already. Overall I do really like these changes; makes things a bit saner

sys/dev/acpica/acpi_spmc.c
25

nit

38–42

I don't understand what the advantage of not having this as an enum is?

67

flag was not a great name on my part, but I think index isn't great either. How about calling this the DSM vendor? or maybe uuid_index? not a big deal if we leave as is, I think the nomenclature surrounding this will always be confusing.

111–117

idk if this is phab not showing this right, but I think this would be clearer with four spaces after the tab

122–131

could we just combine this with the intel list? I don't think it would be an issue to have the same list in both DSM descs, right?

169–170
246–318

this feels a little overengineered to me, but I don't know what a better solution would be. We can't use %b here, because then falling back wouldn't work, which is definitely a nice feature to have.

376–378

mega nit but why not sizeof(buf)?

476

maybe still useful to specify function #0 is "enum functions"?

olce marked 8 inline comments as done.May 11 2026, 8:53 AM
olce added inline comments.
sys/dev/acpica/acpi_spmc.c
25

As intended. I like to have two empty lines to separate conceptual "sections" in a source file, and typically after the headers (which is a block where there must already be blank lines to separate the sections "kernel", "userland" and "local", so a single line after includes does not feel enough).

38–42

It avoids problems when converting to int back and forth. If that conversion is made through "regular" means (assignments, other operators), truncation can hide bugs when manipulating the integer (in the other direction, integer promotion is fine). As an example of a problem with previous code, the use of an index union was wrong (would work only on little endian machines; yes, I'm picky) because you don't know which integer type the compiler will choose as the enum's underlying type; it may as well choose a char, which the index union previously aliased to an int without precaution.

It would be possible to avoid such kind of drawbacks with enum by forcing the underlying type to int if we were using a very recent C standard, but currently that's not the case (and there would still be no implicit check that a conversion to an enum yields a declared value).

In general, the only advantage of using enum is having the compiler (and not only the preprocessor) see the names (helps track name collisions) and treat them as constants. That's only a small improvement compared to the drawback of the hard to find bugs exemplified above. In the current state, I do not think it's worth it (and who would redefine preprocessor constants in the middle of the file anyway?).

Also, I'm quite reluctant to use enums below for the different function indices for each vendor because some of the bits are common and using enums would force having distinct names.

67

Mmm, this is a driver's purely internal index really, the number we put in here has no meaning outside it. (That's also why I'm removing printing the numerical value of the corresponding bitset in this Phab revision, now that we can print names.) Conceptually, this index could indeed be seen as a vendor identifier (at least at the moment), it's just that its content is not official and does not relate to any standard, and calling it vendor may give the impression it is on first read. In the end, it's just the index of the DSM in the dsms[] array, nothing more. I'll put a comment before that field.

111–117

Phab is showing it as it is. With respect to style(9), strictly speaking it seems we are in unknown territory here. I tend to agree that we should treat these as continuation lines, so add 4 spaces. Only thing is, my editor cannot handle that here in a simple way (I would have to spend hours to convince it for this case, and that would not even solve other similar cases). Let me try to change that and see if that does not cause too many conflicts down the stack (else I'll keep it as is and add a final commit to introduce the 4 spaces).

122–131

There's no SLEEP_ENTRY and following functions on Intel DSMs currently (at least according to the spec), so if these indices show up I really want them to be printed as fallbacks and not as SLEEP_ENTRY or others, which they may not be (we have no hint that Intel would adopt the same functions with same indices).

Same constant strings are mutualized by the compiler.

246–318

Not only the controllable fallback (which could be made even more tunable) but also removing the constraint of having to put all strings together in the same object (structure or string). It's much nicer to have names in descriptor structures. At some (indefinite) point, we would move this function to libkern.

376–378

That's basically the same, and also supports other types than just char and variants.

But yes, sizeof() is (unfortunately?) the common idiom for printing functions with an octet buffer, so changing it.

476

The reason why I changed the original comment is that it feels a bit absurd to say that we test for the existence of the "enum functions" we have just called to obtain the result we are testing on... Bit 0 is actually different from the others: It does not indicate the presence of the corresponding function index ("enum functions" is always present). Quote from ACPI: "Bit 0 indicates whether there is support for any functions other than function 0 for the specified UUID and Revision ID. " So bit 0 is in fact not really tied to "enum functions", and I feel mentioning the latter would be more confusing than anything else.

olce marked 8 inline comments as done.May 11 2026, 8:53 AM
olce marked an inline comment as not done.
sys/dev/acpica/acpi_spmc.c
38–42

As an example of a problem with previous code, the use of an index union was wrong (would work only on little endian machines; yes, I'm picky) because you don't know which integer type the compiler will choose as the enum's underlying type; it may as well choose a char, which the index union previously aliased to an int without precaution.

I did not know this! I assumed unions would automatically align the types in C.

It would be possible to avoid such kind of drawbacks with enum by forcing the underlying type to int if we were using a very recent C standard, but currently that's not the case (and there would still be no implicit check that a conversion to an enum yields a declared value).

other options are __attribute__((__packed__)) on the enum or a static assert.

In general, the only advantage of using enum is having the compiler (and not only the preprocessor) see the names (helps track name collisions) and treat them as constants. That's only a small improvement compared to the drawback of the hard to find bugs exemplified above. In the current state, I do not think it's worth it (and who would redefine preprocessor constants in the middle of the file anyway?).

In this case I just like having the readability of having this type have a specific name in the parameters instead of just int.

Also, I'm quite reluctant to use enums below for the different function indices for each vendor because some of the bits are common and using enums would force having distinct names.

that is true. I won't fight this, I'm happy to let you decide

67

okay happy with this

122–131

ok happy

246–318

yeah that would be great

376–378

That's basically the same, and also supports other types than just char and variants.

well, when passing a buffer's size I feel like it's often implied we're passing byte-size and not element-size.

476

alrighty

olce marked an inline comment as done.May 11 2026, 9:20 AM
olce marked 6 inline comments as done.May 11 2026, 9:31 AM
olce added inline comments.
sys/dev/acpica/acpi_spmc.c
38–42

other options are __attribute__((__packed__)) on the enum or a static assert.

Mmm, __packed__ would not work. And the static assert is a defensive measure, but what if the compiler already chooses a char? Or if it does not now, but will in next version?

In this case I just like having the readability of having this type have a specific name in the parameters instead of just int.

I also feel the pain, I much prefer higher-level constructs when possible and they do not obfuscate the intent, but in C enumerations have minimal benefits and annoying drawbacks.

sys/dev/acpica/acpi_spmc.c
38–42

Mmm, packed would not work. And the static assert is a defensive measure, but what if the compiler already chooses a char? Or if it does not now, but will in next version?

I think it would, it tells the compiler to use the smallest size which can represent the values in the enum, i.e. always use char in our case. That + a static assert to make extra sure

olce added inline comments.
sys/dev/acpica/acpi_spmc.c
38–42

I think it would, it tells the compiler to use the smallest size which can represent the values in the enum, i.e. always use char in our case. That + a static assert to make extra sure

It's supposed to (only) tell the compiler not to pad at all between fields, but here there are no fields, so I would bet this does not work. :-)

111–117

I don't get conflicts from that, so fine to change it in this commit.

sys/dev/acpica/acpi_spmc.c
38–42

It has different behaviour when applied to an enum than a struct:

This attribute, attached to struct or union type definition, specifies that each member (other than zero-width bitfields) of the structure or union is placed to minimize the memory required. When attached to an enum definition, it indicates that the smallest integral type should be used.

From https://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/Type-Attributes.html

olce added inline comments.
sys/dev/acpica/acpi_spmc.c
38–42

Oh... I had searched __packed__ just to be sure and too quickly jumped on a link to the GNU C Language Manual which doesn't mention that. TIL.

olce edited the summary of this revision. (Show Details)
  • Cater to comments.
  • Impacts of changes in previous revisions.

Missed occurrences of nitems(buf) => sizeof(buf).

This revision is now accepted and ready to land.May 12 2026, 2:17 PM