acpi_spmc(4): Enable multiple instances
ClosedPublic
Actions

Authored by olce on Mon, May 4, 8:33 PM.

Details

Reviewers

obiwac
imp

Commits

rG310cbb8924fa: acpi_spmc(4): Enable multiple instances

Summary

Support the (so far hypothetical) case of a machine with multiple
instances of the PNP0D80 device (e.g., if multiple DSMs are not
implemented on the same device), by allowing multiple instances of the
device to co-exist.

This is achieved by moving 'supported_functions' from 'struct dsm' into
the softc, so each instance has its own view of which functions are
supported.

Consequently, the check on the instance unit on probe can be removed.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

olce created this revision.Mon, May 4, 8:33 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, May 4, 8:33 PM

olce requested review of this revision.Mon, May 4, 8:33 PM

Harbormaster completed remote builds in B72802: Diff 177208.Mon, May 4, 8:33 PM

olce added a parent revision: D56816: acpi_spmc(4): Global message on constraints parsing failure.Mon, May 4, 8:40 PM

olce added a child revision: D56818: acpi_spmc(4): acpi_spmc_probe_dsm(): Remove passing superfluous handle.

imp accepted this revision.Thu, May 7, 2:38 AM

This revision is now accepted and ready to land.Thu, May 7, 2:38 AM

Fix a bug causing supported functions to be reported as missing.
Constify the struct dsm_desc for the MS DSM (as its revision is not tunable at this point).

This revision now requires review to proceed.Thu, May 7, 8:16 PM

Harbormaster completed remote builds in B72902: Diff 177407.Thu, May 7, 8:16 PM

obiwac accepted this revision.Mon, May 11, 9:07 AM

obiwac added inline comments.

sys/dev/acpica/acpi_spmc.c
131	should probs move this change to the revision where this was added.
187	ditto
354–358	i mean, maybe we should still have this error. Because this situation would be very unexpected and we would probably want someone to report this to us so we can investigate

This revision is now accepted and ready to land.Mon, May 11, 9:07 AM

olce marked 3 inline comments as done.Mon, May 11, 4:28 PM

olce added inline comments.

sys/dev/acpica/acpi_spmc.c
131	The revision field, which is variable, is removed by this very revision, so I cannot add the `const` qualifier before it.
187	ditto :-)
354–358	Well, I don't find it that unexpected. Suppose a firmware manufacturer first implemented the Intel DSM, which appeared first. Suppose that now they just want to add the Microsoft one. They can recycle the existing device, or just introduce a separate device with the new DSM. It's almost equally simple, and the second possibility just avoids any possible mess with the already existing code. I think it's possible to deactivate specific unit numbers through hints, but I'll have to check (perhaps you know the precise stanza to use?). If that's not possible, let's perhaps have a debug tunable/knob to prevent multiple instances, disabled by default (so multiple instances will be attached to by default).

olce marked 3 inline comments as done and an inline comment as not done.Mon, May 11, 4:28 PM

obiwac added inline comments.Mon, May 11, 4:53 PM

sys/dev/acpica/acpi_spmc.c
131	ah fair
187	ditto ;)
354–358	I guess it is possible to have multiple PEP devices... but it would kinda defeat the purpose of DSMs for a vendor to ignore `Arg0`. Then again we both know very well how hardware folk are 😁 so I'm fine with this, esp considering this: If that's not possible, let's perhaps have a debug tunable/knob to prevent multiple instances, disabled by default (so multiple instances will be attached to by default). I think its fine to leave things as-is then, because if an issue arises it is trivial to see in dmesg/ask user if they have multiple `acpi_spmc` devices.

olce added inline comments.Tue, May 12, 7:40 AM

sys/dev/acpica/acpi_spmc.c
131	Sorry, I meant the `supported_functions` field.

olce marked an inline comment as done.Tue, May 12, 7:47 AM

olce added inline comments.

sys/dev/acpica/acpi_spmc.c
354–358	I guess it is possible to have multiple PEP devices... but it would kinda defeat the purpose of DSMs for a vendor to ignore `Arg0`. Even if there are multiple devices, the vendor in theory cannot ignore Arg0 (UUID), but perhaps could in practice for the Intel/MS combination, since the indices for both DSMs do not collide. That would be very ugly, though, and would probably lead to malfunction since our driver would think the AMD DSM is also present and would call the same DSM with improper function indices. Which reminds me that having a knob to disable some of the DSMs might be a good idea (a kind of hint, but at the finer-grained level of DSM instead of the whole device). I was just thinking about the case of having a single UUID supported per device, so there would be two devices to support both Intel and MS DSMs, or AMD and MS ones, instead of having both on the same device. I think its fine to leave things as-is then, because if an issue arises it is trivial to see in dmesg/ask user if they have multiple `acpi_spmc` devices. Yeah, I also think that's enough. And `acpi_spmc(4)` does nothing until suspend-to-idle is exercized, so the dmesg is easy to extract before a potential malfunction.

olce marked an inline comment as done.Tue, May 12, 7:47 AM

obiwac added inline comments.Tue, May 12, 9:13 AM

sys/dev/acpica/acpi_spmc.c
354–358	Even if there are multiple devices, the vendor in theory cannot ignore Arg0 (UUID) Yeah I know but I mean that argument is explicitly to allow for multiplexing different UUIDs in a single "method" (the DSM). If you just create a new PEP device for each UUID, then might as well just have some UUID object under the PEP device instead of this DSM mechanism...