Page MenuHomeFreeBSD

acpi_powerres: Fix turning off power resources on first D-state switch
ClosedPublic

Authored by obiwac on Jan 8 2025, 11:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 12, 2:06 AM
Unknown Object (File)
Mon, Sep 29, 6:32 PM
Unknown Object (File)
Mon, Sep 29, 6:31 PM
Unknown Object (File)
Mon, Sep 29, 6:31 PM
Unknown Object (File)
Mon, Sep 29, 6:31 PM
Unknown Object (File)
Mon, Sep 29, 6:30 PM
Unknown Object (File)
Wed, Sep 24, 5:01 AM
Unknown Object (File)
Fri, Sep 19, 5:19 AM
Subscribers

Details

Summary

The power resource dependencies for each _PRx object are discovered and cached in ac_prx on the power consumer struct (struct acpi_powerconsumer) when a power consumer is registered. This is done in acpi_pwr_get_power_resources. ACPI guarantees these _PRx objects will evaluate to the same thing each time they are called.

This discovery process also registers those power resources, which were previously only registered when they were referenced by the relevant _PRx object for the target D-state when switching. This meant that the first D-state switch for a power consumer would not turn off any power resources as they wouldn't have been registered yet. This revision fixes this.

ac_prx will be used by subsequent patches.

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.

I don't know if this will make a significant difference, but since devices will now go into D3cold (the power resources under _PR3 wouldn't be turned off previously, which is needed for D3cold), this might make suspended devices use less power (devctl suspend/resume <device>).

Diff Detail

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

Event Timeline

sys/dev/acpica/acpi_powerres.c
275

reslist_object leaked

279

Style: 4 space indent, not open paren indent, here and elsewhere.

336

You're ignoring errors here. Why have them returned?
And when you can't allocate memory or whatever, what's the right evasive action?

obiwac marked 2 inline comments as done and an inline comment as not done.Aug 12 2025, 9:03 PM

Thanks for the review @imp !

Have made the necessary changes, will now play around with acpi_pwr_deregister_consumer to see if it works correctly (as it was unused before).

sys/dev/acpica/acpi_powerres.c
275

thanks :)

279

elsewhere in the file uses this style. Should I make this 4 spaces anyways?

336

now deregistering power consumer on error

okay, acpi_pwr_deregister_consumer seem to work as expected

sys/dev/acpica/acpi_powerres.c
259–261

I guess this is technically redundant as we're allocating this memory with M_ZERO now. Should I keep this explicit anyway?

sys/dev/acpica/acpi_powerres.c
259–261

I think having the explicit initialization is useful for readability.

287

Shouldn't this be allocating an array of size pc->ac_prx[state].prx_count? You can use mallocarray() here.

360

Can we remove this XXX comment?

obiwac marked 3 inline comments as done.

Fix bad malloc size

thanks for the review!

sys/dev/acpica/acpi_powerres.c
287

absolutely, thanks for catching that

I'm not sure how to handle the styling of the wrapping here since aligning the next line to the paren (like in the rest of this file) crosses 80 chars, so I've just done 4 spaces.

360

We're not yet doing this once the consumer has been powered off (only in acpi_pwr_register_consumer when acpi_pwr_get_power_resources fails).

I haven't looked into calling this anywhere else yet.

This revision is now accepted and ready to land.Aug 18 2025, 1:29 PM