Page MenuHomeFreeBSD
Differential D48386 Diff 160554 sys/dev/acpica/acpi_powerres.c

Changeset View

Standalone View

View Options

sys/dev/acpica/acpi_powerres.c

Show First 20 Lines • Show All 111 Lines • ▼ Show 20 Linesstatic void acpi_pwr_reference_resource(ACPI_OBJECT *obj,
void *arg); void *arg);
static int acpi_pwr_dereference_resource(struct acpi_powerconsumer static int acpi_pwr_dereference_resource(struct acpi_powerconsumer
*pc); *pc);
static ACPI_STATUS acpi_pwr_switch_power(void); static ACPI_STATUS acpi_pwr_switch_power(void);
static struct acpi_powerresource static struct acpi_powerresource
*acpi_pwr_find_resource(ACPI_HANDLE res); *acpi_pwr_find_resource(ACPI_HANDLE res);
static struct acpi_powerconsumer static struct acpi_powerconsumer
*acpi_pwr_find_consumer(ACPI_HANDLE consumer); *acpi_pwr_find_consumer(ACPI_HANDLE consumer);
static ACPI_STATUS acpi_pwr_infer_state(struct acpi_powerconsumer *pc);
static ACPI_STATUS acpi_pwr_get_state_locked(ACPI_HANDLE consumer, int *state);
/* /*
* Register a power resource. * Register a power resource.
* *
* It's OK to call this if we already know about the resource. * It's OK to call this if we already know about the resource.
*/ */
static ACPI_STATUS static ACPI_STATUS
acpi_pwr_register_resource(ACPI_HANDLE res) acpi_pwr_register_resource(ACPI_HANDLE res)
▲ Show 20 Lines • Show All 211 Lines • ▼ Show 20 Lines return_ACPI_STATUS (AE_NO_MEMORY);
if (ACPI_FAILURE(status)) { if (ACPI_FAILURE(status)) {
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS,
"failed to get power resources for %s\n", "failed to get power resources for %s\n",
acpi_name(consumer))); acpi_name(consumer)));
acpi_pwr_deregister_consumer(consumer); acpi_pwr_deregister_consumer(consumer);
return_ACPI_STATUS (status); return_ACPI_STATUS (status);
} }
/* XXX we should try to find its current state */ /* Find its initial state. */
if (ACPI_FAILURE(acpi_pwr_get_state_locked(consumer, &pc->ac_state)))
pc->ac_state = ACPI_STATE_UNKNOWN; pc->ac_state = ACPI_STATE_UNKNOWN;
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "registered power consumer %s\n", ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "registered power consumer %s\n",
acpi_name(consumer))); acpi_name(consumer)));
return_ACPI_STATUS (AE_OK); return_ACPI_STATUS (AE_OK);
} }
/* /*
Show All 27 Lines if (pc->ac_prx[i].prx_deps != NULL)
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "deregistered power consumer %s\n", ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "deregistered power consumer %s\n",
acpi_name(consumer))); acpi_name(consumer)));
return_ACPI_STATUS (AE_OK); return_ACPI_STATUS (AE_OK);
} }
/* /*
* Set a power consumer to a particular power state. * The _PSC control method isn't required if it's possible to infer the D-state
* from the _PRx control methods. (See 7.3.6.)
* We can infer that a given D-state has been achieved when all the dependencies
* are in the ON state.
*/ */
static ACPI_STATUS
acpi_pwr_infer_state(struct acpi_powerconsumer *pc)
markjUnsubmitted
Done
Inline Actions

consumer seems to be unused except for the assertion below.

markj: `consumer` seems to be unused except for the assertion below.
obiwacAuthorUnsubmitted
Done
Inline Actions

indeed :)

obiwac: indeed :)
{
ACPI_HANDLE *res;
uint32_t on;
bool all_on = false;
ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
ACPI_SERIAL_ASSERT(powerres);
/* It is important we go from the hottest to the coldest state. */
markjUnsubmitted
Done
Inline Actions

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

markj: Is it actually possible to have `consumer == NULL` here, given that this function is defined to…
obiwacAuthorUnsubmitted
Done
Inline Actions

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

obiwac: You're right, I do not think this is possible.
obiwacAuthorUnsubmitted
Done
Inline Actions

turned into assert

obiwac: turned into assert
for (
pc->ac_state = ACPI_STATE_D0;
pc->ac_state <= ACPI_STATE_D3_HOT && !all_on;
pc->ac_state++
) {
MPASS(pc->ac_state <= sizeof(pc->ac_prx) / sizeof(*pc->ac_prx));
if (!pc->ac_prx[pc->ac_state].prx_has)
continue;
all_on = true;
for (size_t i = 0; i < pc->ac_prx[pc->ac_state].prx_count; i++) {
res = pc->ac_prx[pc->ac_state].prx_deps[i];
/* If failure, better to assume D-state is hotter than colder. */
if (ACPI_FAILURE(acpi_GetInteger(res, "_STA", &on)))
continue;
if (on == 0) {
all_on = false;
break;
}
}
}
MPASS(pc->ac_state != ACPI_STATE_D0);
/*
* If none of the power resources required for the shallower D-states are
markjUnsubmitted
Done
Inline Actions

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.

markj: A dumb question: why is this assertion true? The way the loop above is structure, it appears…
obiwacAuthorUnsubmitted
Done
Inline Actions

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.

obiwac: The only situation where the outer loop can break when `pc->ac_state == ACPI_STATE_D0` is if…
markjUnsubmitted
Done
Inline Actions

Oops, yes, I misread.

markj: Oops, yes, I misread.
* on, then we can assume it is unpowered (i.e. D3cold). A device is not
* required to support D3cold however; in that case, _PR3 is not explicitly
* provided. Those devices should default to D3hot instead.
*
* See comments of first row of table 7.1 in ACPI spec.
*/
if (!all_on)
pc->ac_state = pc->ac_prx[ACPI_STATE_D3_HOT].prx_has ?
markjUnsubmitted
Not Done
Inline Actions

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?

markj: The comment above makes me think that the intent was to test `pc->ac_prx[ACPI_STATE_D3_COLD].
obiwacAuthorUnsubmitted
Not Done
Inline Actions

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 😁

obiwac: D3cold is never explicitly reported. If the `_PR3` object exists (which is what `pc->ac_prx…
ACPI_STATE_D3_COLD : ACPI_STATE_D3_HOT;
else
pc->ac_state--;
return_ACPI_STATUS (AE_OK);
}
static ACPI_STATUS
acpi_pwr_get_state_locked(ACPI_HANDLE consumer, int *state)
{
struct acpi_powerconsumer *pc;
ACPI_HANDLE method_handle;
ACPI_STATUS status;
ACPI_BUFFER result;
ACPI_OBJECT *object = NULL;
ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
ACPI_SERIAL_ASSERT(powerres);
if (consumer == NULL)
return_ACPI_STATUS (AE_NOT_FOUND);
if ((pc = acpi_pwr_find_consumer(consumer)) == NULL) {
if (ACPI_FAILURE(status = acpi_pwr_register_consumer(consumer)))
goto out;
if ((pc = acpi_pwr_find_consumer(consumer)) == NULL)
panic("acpi added power consumer but can't find it");
markjUnsubmitted
Done
Inline Actions

Should this function return status instead of OK?

markj: Should this function return `status` instead of OK?
}
status = AcpiGetHandle(consumer, "_PSC", &method_handle);
if (ACPI_FAILURE(status)) {
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "no _PSC object - %s\n",
AcpiFormatException(status)));
status = acpi_pwr_infer_state(pc);
if (ACPI_FAILURE(status)) {
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "couldn't infer D-state - %s\n",
AcpiFormatException(status)));
pc->ac_state = ACPI_STATE_UNKNOWN;
}
goto out;
}
result.Pointer = NULL;
result.Length = ACPI_ALLOCATE_BUFFER;
status = AcpiEvaluateObjectTyped(method_handle, NULL, NULL, &result, ACPI_TYPE_INTEGER);
if (ACPI_FAILURE(status) || result.Pointer == NULL) {
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "failed to get state with _PSC - %s\n",
AcpiFormatException(status)));
pc->ac_state = ACPI_STATE_UNKNOWN;
goto out;
}
object = (ACPI_OBJECT *)result.Pointer;
pc->ac_state = ACPI_STATE_D0 + object->Integer.Value;
status = AE_OK;
out:
if (object != NULL)
AcpiOsFree(object);
*state = pc->ac_state;
return_ACPI_STATUS (status);
}
/*
* Get a power consumer's D-state.
*/
ACPI_STATUS ACPI_STATUS
acpi_pwr_get_state(ACPI_HANDLE consumer, int *state)
{
ACPI_STATUS res;
markjUnsubmitted
Done
Inline Actions
acpi_pwr_get_state(ACPI_HANDLE consumer, int *state)
{
- int res;
+ ACPI_STATUS res;
ACPI_SERIAL_BEGIN(powerres);
markj:
ACPI_SERIAL_BEGIN(powerres);
res = acpi_pwr_get_state_locked(consumer, state);
ACPI_SERIAL_END(powerres);
return (res);
}
/*
* Set a power consumer to a particular D-state.
*/
ACPI_STATUS
acpi_pwr_switch_consumer(ACPI_HANDLE consumer, int state) acpi_pwr_switch_consumer(ACPI_HANDLE consumer, int state)
{ {
struct acpi_powerconsumer *pc; struct acpi_powerconsumer *pc;
ACPI_HANDLE method_handle, reslist_handle, pr0_handle; ACPI_HANDLE method_handle, reslist_handle, pr0_handle;
ACPI_BUFFER reslist_buffer; ACPI_BUFFER reslist_buffer;
ACPI_OBJECT *reslist_object; ACPI_OBJECT *reslist_object;
ACPI_STATUS status; ACPI_STATUS status;
char *method_name, *reslist_name = NULL; char *method_name, *reslist_name = NULL;
int new_state;
ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__); ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
/* It's never ok to switch a non-existent consumer. */ /* It's never ok to switch a non-existent consumer. */
if (consumer == NULL) if (consumer == NULL)
return_ACPI_STATUS (AE_NOT_FOUND); return_ACPI_STATUS (AE_NOT_FOUND);
reslist_buffer.Pointer = NULL; reslist_buffer.Pointer = NULL;
reslist_object = NULL; reslist_object = NULL;
▲ Show 20 Lines • Show All 184 Lines • ▼ Show 20 Lines if (ACPI_FAILURE(status)) {
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "failed to set state - %s\n", ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "failed to set state - %s\n",
AcpiFormatException(status))); AcpiFormatException(status)));
pc->ac_state = ACPI_STATE_UNKNOWN; pc->ac_state = ACPI_STATE_UNKNOWN;
/* XXX Should we return to previous state? */ /* XXX Should we return to previous state? */
goto out; goto out;
} }
} }
/* Transition was successful */ /*
* Make sure the transition succeeded. If getting new state failed,
markjUnsubmitted
Done
Inline Actions

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.

markj: style.9 wants variables declared at the beginning of a lexical scope. This file doesn't follow…
* just assume the new state is what we wanted. This was the behaviour
* before we were checking D-states.
markjUnsubmitted
Not Done
Inline Actions

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

markj: If acpi_pwr_get_state_locked() failed, shouldn't we report that instead of returning AE_OK…
obiwacAuthorUnsubmitted
Not Done
Inline Actions

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?

obiwac: yes, in fact, we should probably just leave `pc->ac_state = state` as it was before in this…
*/
if (ACPI_FAILURE(acpi_pwr_get_state_locked(consumer, &new_state))) {
printf("%s: failed to get new D-state\n", __func__);
pc->ac_state = state; pc->ac_state = state;
} else {
if (new_state != state)
printf("%s: new power state %s is not the one requested %s\n",
__func__, acpi_d_state_to_str(new_state),
markjUnsubmitted
Done
Inline Actions

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;
}
markj: A bit clearer, to me at least, would be: ``` if (ACPI_FAILURE(...)) ACPI_DEBUG_PRINT(...)…
obiwacAuthorUnsubmitted
Done
Inline Actions

Yeah I agree, the two assignments are slightly redundant.

obiwac: Yeah I agree, the two assignments are slightly redundant.
acpi_d_state_to_str(state));
pc->ac_state = new_state;
markjUnsubmitted
Done
Inline Actions
}
/*
- * We consider transition was successful even if the state we got doesn't
+ * We consider the transition successful even if the state we got doesn't
* reflect what we set it to. This is because we weren't previously
markj:
}
/*
* We consider the transition successful even if the state we got doesn't
* reflect what we set it to. This is because we weren't previously
* checking the new state at all, so there might exist buggy platforms on
* which suspend would otherwise succeed if we failed here.
*/
markjUnsubmitted
Done
Inline Actions

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

markj: Is it intentional that we still return OK if the check above failed? If so, I think the comment…
obiwacAuthorUnsubmitted
Done
Inline Actions

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.

obiwac: I don't totally know if it makes sense to return an error here or not, as the previous code was…
markjUnsubmitted
Done
Inline Actions

Having it just be a warning seems reasonable enough.

markj: Having it just be a warning seems reasonable enough.
status = AE_OK; status = AE_OK;
out: out:
ACPI_SERIAL_END(powerres); ACPI_SERIAL_END(powerres);
if (reslist_buffer.Pointer != NULL) if (reslist_buffer.Pointer != NULL)
AcpiOsFree(reslist_buffer.Pointer); AcpiOsFree(reslist_buffer.Pointer);
return_ACPI_STATUS (status); return_ACPI_STATUS (status);
} }
▲ Show 20 Lines • Show All 251 Lines • Show Last 20 Lines