Page MenuHomeFreeBSD
Differential D52598

acpi: Fix panic on poweroff
AbandonedPublic
Actions

Authored by obiwac on Sep 17 2025, 9:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 31, 2:04 PM
Unknown Object (File)
Fri, Jan 30, 4:01 PM
Unknown Object (File)
Wed, Jan 28, 9:16 PM
Unknown Object (File)
Fri, Jan 23, 5:34 PM
Unknown Object (File)
Thu, Jan 22, 9:15 AM
Unknown Object (File)
Thu, Jan 22, 9:12 AM
Unknown Object (File)
Thu, Jan 22, 9:07 AM
Unknown Object (File)
Wed, Jan 14, 5:55 PM
View All 51 Files
Subscribers
bz
eduardo
emaste
imp
madpilot
zarychtam_plan-b.pwste.edu.pl

Details

Reviewers
cy
markj
jkim
madpilot
Summary
Previously we were erroneously passing the device softc to
acpi_stype_to_sstate in acpi_wake/run_sleep_prep instead of the ACPI
bus device softc.

Now, we pass the ACPI bus device softc when walking devices to prep for
sleep/wake.

Use the opportunity to pass the stype instead of the ACPI S-state to
acpi_wake_prep_walk.

PR: 289634
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 67139
Build 64022: arc lint + arc unit

Event Timeline

obiwac created this revision.Sep 17 2025, 9:12 PM
Herald added a subscriber: imp. · View Herald TranscriptSep 17 2025, 9:12 PM
obiwac requested review of this revision.Sep 17 2025, 9:12 PM
Harbormaster completed remote builds in B67136: Diff 162294.Sep 17 2025, 9:12 PM
obiwac edited the summary of this revision. (Show Details)Sep 17 2025, 9:16 PM
markj added a comment.Sep 17 2025, 9:31 PM

Is the proper fix "just" to plumb the acpi softc through acpi_wake_prep_walk()->AcpiWalkNamespace()->acpi_wake_prep()->...? That doesn't seem too painful.

I'm ok with this patch as an intermediate step to fix the panics, but the above should also be straightforward.

obiwac planned changes to this revision.Sep 17 2025, 9:56 PM

yeah you're absolutely right, I thought acpi_shutdown's dev was a device but its the actual bus of course so this is easy. Will do that in a minute

it's been a really long day, sorry I missed this :D

eduardo added a subscriber: eduardo.Sep 17 2025, 10:22 PM
obiwac updated this revision to Diff 162298.Sep 17 2025, 10:56 PM

Do this correctly.

Harbormaster completed remote builds in B67139: Diff 162298.Sep 17 2025, 10:56 PM
emaste added a subscriber: emaste.Sep 18 2025, 12:00 AM
bz added a subscriber: bz.Sep 18 2025, 1:14 AM
bz added inline comments.
sys/dev/acpica/acpivar.h
137

Is there a reason to not keep this file-local in acpi.c?

obiwac mentioned this in D52600: acpi: Add back `hw.acpi.suspend_state` sysctl.Sep 18 2025, 8:46 AM
obiwac added inline comments.Sep 18 2025, 8:49 AM
sys/dev/acpica/acpivar.h
137

I could but I saw that no other structs were being kept file-local so I decided to put this here.

But none of these are entirely local actually so I'll move this to acpi.c.

obiwac updated this revision to Diff 162302.Sep 18 2025, 8:52 AM

Move struct acpi_wake_prep_context out of acpivar.h

Harbormaster completed remote builds in B67140: Diff 162302.Sep 18 2025, 8:52 AM
obiwac marked an inline comment as done.Sep 18 2025, 8:52 AM
obiwac added inline comments.
sys/dev/acpica/acpivar.h
137

actually struct acpi_interface was, my bad.

zarychtam_plan-b.pwste.edu.pl added a subscriber: zarychtam_plan-b.pwste.edu.pl.EditedSep 18 2025, 8:56 AM

This patch fixes panic. Please push it when it's ready.

Edit: Unfortunately, suspend is still broken, PC wakes up immediately.

obiwac edited the summary of this revision. (Show Details)Sep 18 2025, 9:15 AM
obiwac added a comment.Sep 18 2025, 9:43 AM
In D52598#1201370, @zarychtam_plan-b.pwste.edu.pl wrote:

This patch fixes panic. Please push it when it's ready.

Edit: Unfortunately, suspend is still broken, PC wakes up immediately.

Okay, but this is a separate issue I guess. This is going to be hard for me to debug as I do not have an S3 machine. Are you willing to try some patches out for me to fix this issue? Also, can you send me your kernel logs (obiwac@freebsd.org)?

In the meantime I'll just revert the changes which touch the ACPI logic.

markj accepted this revision.Sep 18 2025, 12:38 PM
markj added inline comments.
sys/dev/acpica/acpi.c
3760

Stylistically this looks a bit weird. More typical would be something like

struct acpi_wake_prep_context *ctx = context;

if (AcpiGbl_SystemAwakeAndRunning)
    acpi_wake_sleep_prep(ctx->sc, handle, ctx->stype);
else
    acpi_wake_run_prep(ctx->sc, handle, ctx->stype);

or declare more local variables after, struct acpi_softc *sc = ctx->sc; etc.

This revision is now accepted and ready to land.Sep 18 2025, 12:38 PM
madpilot accepted this revision.Sep 18 2025, 1:52 PM
madpilot added a subscriber: madpilot.

I finally tested this and works fine for me.

I'm sorry I was unable to test it earlier!

obiwac added a comment.Sep 18 2025, 5:56 PM
In D52598#1201481, @madpilot wrote:

I finally tested this and works fine for me.

I'm sorry I was unable to test it earlier!

Don't worry about it, I wasn't behind my computer today anyways. Thanks for testing!

obiwac mentioned this in D52705: acpi: Use sleep types defined in sys/power.h.Sep 24 2025, 11:42 AM
obiwac abandoned this revision.Sep 24 2025, 11:44 AM

these changes have been rolled into D52705 because the original commit that had these changes had to be reverted anyways.

obiwac marked an inline comment as done.Sep 24 2025, 11:48 AM
obiwac added inline comments.
sys/dev/acpica/acpi.c
3760

applied this change in D52705

Revision Contents
Changeset List

PathSize
sys/
dev/
acpica/
40 lines
5 lines

Diff 162298

View Options

sys/dev/acpica/acpi.c

Loading...
View Options

sys/dev/acpica/acpivar.h

Loading...