acpi_powerres: `acpi_pwr_get_state` and getting initial D-state for device

[obiwac]

Summary

acpi_pwr_get_state is added as a prerequisite to LPI (low-power idle) states. Since these states define minimum D-state constraints on other devices to be able to enter them, it will be necessary to use this function to check them before attempting to do so.

Aside from that, this function is currently only used to get the initial D-state of a power consumer when registering it (previously the ac_state value would be set to ACPI_STATE_UNKNOWN). It uses the _PSC method if available (older devices), or infers the D-state through the _PRx objects (cached in ac_prx) with acpi_pwr_infer_state if not.

acpi_pwr_switch_consumer now uses this to verify that the D-state of a power consumer was switched correctly.

This was split out from D48294.

Sponsored by: The FreeBSD Foundation

Test Plan

I have tested this on the Framework 13 AMD Ryzen 7040 series. All devices are able to suspend/resume fine and their D-states are set/gotten correctly now (am able to verify that they pass the LPI constraints).

I do not have a device with _PSC objects, so I am unable to test that path.

[obiwac]

Fix shared lock being acquired twice by pulling out acpi_pwr_get_state_locked.

[markj]

sys/dev/acpica/acpi_powerres.c

410

Is it actually possible to have consumer == NULL here, given that this function is defined to be static and its caller already checks?

438

A dumb question: why is this assertion true? The way the loop above is structure, it appears possible to have pc->ac_state == ACPI_STATE_D0 if the initial iteration breaks.

737–759

Is it intentional that we still return OK if the check above failed? If so, I think the comment should explain why.

749

A bit clearer, to me at least, would be:

if (ACPI_FAILURE(...))
    ACPI_DEBUG_PRINT(...)
else {
    if (new_state != state)
        ACPI_DEBUG_PRINT
    pc->ac_state = new_state;
}

[obiwac]

Thanks for your review, Mark! I'll get to fixing the issues you highlighted soon.

sys/dev/acpica/acpi_powerres.c

410

You're right, I do not think this is possible.

438

The only situation where the outer loop can break when pc->ac_state == ACPI_STATE_D0 is if pc->ac_state <= ACPI_STATE_D3_HOT && !all_on is falsey, which is impossible on the first iteration.

737–759

I don't totally know if it makes sense to return an error here or not, as the previous code was not checking for the state after setting it, so I'm afraid that it would break suspend on certain machines where it would otherwise work. I don't think it's unreasonable that this should just be a warning.

In any case comment should indeed be changed.

749

Yeah I agree, the two assignments are slightly redundant.

[markj]

sys/dev/acpica/acpi_powerres.c

438

Oops, yes, I misread.

473

Should this function return status instead of OK?

517

737–759

Having it just be a warning seems reasonable enough.

[obiwac]

Respond to review comments & general cleanup

[obiwac]

sys/dev/acpica/acpi_powerres.c

410

turned into assert

[markj]

sys/dev/acpica/acpi_powerres.c

401

consumer seems to be unused except for the assertion below.

446

The comment above makes me think that the intent was to test pc->ac_prx[ACPI_STATE_D3_COLD].prx_has. Is my understanding wrong?

739

style.9 wants variables declared at the beginning of a lexical scope. This file doesn't follow style.9 generally but in that respect it does, so it'd be nice to be consistent.

741

If acpi_pwr_get_state_locked() failed, shouldn't we report that instead of returning AE_OK without setting pc->ac_state?

751

[obiwac]

pc->ac_state = state when failed to get new D-state, remove useless consumer param in acpi_pwr_infer_state.

[obiwac]

thanks for the review!

sys/dev/acpica/acpi_powerres.c

401

indeed :)

446

D3cold is never explicitly reported.

If the _PR3 object exists (which is what pc->ac_prx[ACPI_STATE_D3_HOT].prx_has represents), then that provides the power resources for D3hot, and we assume that if all those power resources are off we're in D3cold.

If _PR3 doesn't exist then the power consumer only supports D3hot.

C.f. the comments in the first row of the table in ACPI 7.1. Have added this reference in the comment.

I admit the spec, esp 7.3.11 which deals with _PR3 directly, is all a little confusing 😁

741

yes, in fact, we should probably just leave pc->ac_state = state as it was before in this situation.

I have promoted this to a printf. Is this the preferred way of printing these kinds of warnings?

[jrm]