acpi_spmc(4): Be less verbose by default

[olce]

Summary

Do not print by default details of failures that are unlikely to help
a normal user and to have a crucial influence on whether suspension
works correctly. Do so only on a verbose boot or if requested
explicitly by the user via 'debug.acpi.spmc.verbose'.

In particular:

• On an Intel Framework laptop, the Microsoft DSM only reports the SLEEP_ENTRY and SLEEP_EXIT functions. That makes some sense since, according to its specification, all functions of a Microsoft DSM except these two are in fact redundant with Intel DSM's ones (also, that of AMD DSM's ones). Those functions being missing are only a potential problem if there is no other DSM than Microsoft's (yet to be observed in the field).
• The details of malformed/unapplicable constraints or ones with a newer format the driver does not know about are not readily actionable pieces of information, but rather debug/developer-oriented ones. When verbosity is not requested, only print the details of the first such failure to encourage reporting and at the same time avoid cluttering the output.
• Detecting and printing unknown DSM functions is not directly actionable either, and the driver not using these functions may not prevent suspending (but might, e.g., prevent reaching deeper sleep states).

Sponsored by: The FreeBSD Foundation

[olce]

I'm not completely sure about this one. On one hand, I don't expect these messages to be frequent, so we could keep them unconditionally. On the other hand, there's not much a regular user can do about them. Also, in D56816, I'm adding an unconditional error message if loading the constraints globally failed, which is probably sufficient.

[obiwac]

sys/dev/acpica/acpi_spmc.c

589–590

this feels like something we wouldn't wanna only do when verbose. Considering there's no spec for this, I'd like to be aware when something unexpected happens.

[olce]

sys/dev/acpica/acpi_spmc.c

589–590

Yes.

As an alternative, this Phab revision should be viewed in conjunction with D56816, which leads to printing an error message when constraints parsing was interrupted. So the user will be aware that something unexpected happened, but we wouldn't just print what exactly. He could then obtain the detailed error message by using a verbose boot. I agree that's less convenient, I was just trying to balance annoying/worrying verbosity with debuggability (i.e., the latter should really offset the inconvenience of printing non-actionable and worrying messages to the administrator).

There's also something I apparently missed here: There's only one print, not once per constraint, as we bail out immediately if this problem occurs. That a priori makes sense, because it's hard to imagine that FW designers would play with changing the constraint size from one constraint to the other (but after having read a bunch of AML code, I wouldn't bet this can't happen...). Then, as it's only one printed line (and only once), I certainly won't object to keeping it (I was already hesitating before).

[obiwac]

sys/dev/acpica/acpi_spmc.c

589–590

As an alternative, this Phab revision should be viewed in conjunction with D56816, which leads to printing an error message when constraints parsing was interrupted. So the user will be aware that something unexpected happened, but we wouldn't just print what exactly. He could then obtain the detailed error message by using a verbose boot. I agree that's less convenient, I was just trying to balance annoying/worrying verbosity with debuggability (i.e., the latter should really offset the inconvenience of printing non-actionable and worrying messages to the administrator).

I don't know, I think this is anyway unlikely enough to happen that it's worth worrying the user over. The alternative is that we accidentally miss constraints, and a user's machine doesn't enter S0i3, which is way more annoying to debug. Much rather the noise of an extra bug on the off chance this ever happens than missing a reason why a machine isn't entering S0i3

There's also something I apparently missed here: There's only one print, not once per constraint, as we bail out immediately if this problem occurs. That a priori makes sense, because it's hard to imagine that FW designers would play with changing the constraint size from one constraint to the other (but after having read a bunch of AML code, I wouldn't bet this can't happen...). Then, as it's only one printed line (and only once), I certainly won't object to keeping it (I was already hesitating before).

That isn't true, we just set the constraint's handle to NULL and continue

[olce]

sys/dev/acpica/acpi_spmc.c

589–590

Much rather the noise of an extra bug on the off chance this ever happens than missing a reason why a machine isn't entering S0i3

Not exactly. We won't miss a reason why the machine isn't entering S0i3 because of D56816, so an error will be printed in any case, it just won't have details. But anyway, I'm fine with instead printing just one occurrence of a constraints error with all details.

That isn't true, we just set the constraint's handle to NULL and continue

Not here. That happens in another place later in the file, and I'm taking care of it also by just printing the first occurrence if !VERBOSE().

[obiwac]

sys/dev/acpica/acpi_spmc.c

589–590

Not exactly. We won't miss a reason why the machine isn't entering S0i3 because of D56816, so an error will be printed in any case, it just won't have details. But anyway, I'm fine with instead printing just one occurrence of a constraints error with all details.

Ah yeah fair enough. I did read it the thought didn't cross my mind haha. But yes I still do prefer printing the detail

Not here. That happens in another place later in the file, and I'm taking care of it also by just printing the first occurrence if !VERBOSE().

totally right - man my reading comprehension skills are down the gutter today 😄 too much reviewing!

[olce]

Print details of the first constraints parsing failure in the Intel parser (for a failure that is likely to repeat itself for other constraints).