Details

Reviewers

imp
cperciva

Commits

rGb5bc47f1e1ab: pci: Clear active PME# and disable PME# generation
rG82d692771239: pci: Clear active PME# and disable PME# generation

Summary

The PCI power management specification requires that the OS clear any
pending PME# interrupt and generation of PME# interrupts during
"initial operating system load". Note that clearing a pending PME#
interrupt requires writing a 1 to the Read/Write-Clear PME bit in the
power management status register. To handle the boot time case, clear
PME# state in pci_read_cap() when scanning new PCI devices. This
should also cover hotplug devices.

In addition, clear this state on every PCI device after resume from
sleep in pci_resume_child before invoking the driver's DEVICE_RESUME
method.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 62800
Build 59684: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Mar 3 2025, 9:03 PM

jhb created this revision.

Harbormaster completed remote builds in B62746: Diff 151822.Mar 3 2025, 9:03 PM

imp accepted this revision.Mar 3 2025, 9:58 PM

imp added inline comments.

sys/dev/pci/pci.c
916	why were you able to remove this check?

This revision is now accepted and ready to land.Mar 3 2025, 9:58 PM

This doesn't cause any problems in EC2, but it doesn't solve the "recognizing that we're ready for an nvme disk to go away" problem either. They're definitely using a write to the power status register as a cue to remove the device (per https://reviews.freebsd.org/D49146 ).

jhb added inline comments.Mar 4 2025, 12:36 AM

sys/dev/pci/pci.c
916	Because it is incorrect. The register is always there. Some PCI capabilities do have optional registers indicated by some flag in another register, but this one is always present. That said, we don't use `pp_bse` or `pp_data` anywhere and we should probably just remove them. Even `pp_status` is rather dubious. For most other capability register sets like this, we only save the base offset in `cfg` and use offsets like `PCIR_POWER_STATUS` inline when accessing registers.

In D49222#1122599, @cperciva wrote:

This doesn't cause any problems in EC2, but it doesn't solve the "recognizing that we're ready for an nvme disk to go away" problem either. They're definitely using a write to the power status register as a cue to remove the device (per https://reviews.freebsd.org/D49146 ).

Humm, I can't imagine what the goal of this behavior is on the part of EC2. Can the engineers there explain why they think the PME is still pending and needs to be cleared? Do they assert a PME as part of the eject notification? (You can read the PME bit to see when it changes from 0 to 1 if that's helpful.)

imp added inline comments.Mar 4 2025, 12:43 AM

sys/dev/pci/pci.c
916	yea, if we don't use them...that's a good idea...

In D49222#1122616, @jhb wrote:

In D49222#1122599, @cperciva wrote:

This doesn't cause any problems in EC2, but it doesn't solve the "recognizing that we're ready for an nvme disk to go away" problem either. They're definitely using a write to the power status register as a cue to remove the device (per https://reviews.freebsd.org/D49146 ).

Humm, I can't imagine what the goal of this behavior is on the part of EC2. Can the engineers there explain why they think the PME is still pending and needs to be cleared? Do they assert a PME as part of the eject notification? (You can read the PME bit to see when it changes from 0 to 1 if that's helpful.)

Yes, the PCIM_PSTAT_PMEENABLE is set to 0 until the eject request arrives at which point it is set to 1.

In D49222#1122856, @cperciva wrote:

Yes, the PCIM_PSTAT_PMEENABLE is set to 0 until the eject request arrives at which point it is set to 1.

Oops, I was misled by some earlier testing. In fact PME# never gets asserted. But we need to turn it off anyway to acknowledge the detach.

Clear PME earlier in resume

This revision now requires review to proceed.Mar 5 2025, 9:53 PM

Harbormaster completed remote builds in B62774: Diff 151930.Mar 5 2025, 9:53 PM

Existing device drivers cleared PME# first in their resume methods before messing with device registers, so do the same in pci_resume_child.

jhb added a child revision: D49250: pci: Add helper routines to manage PME in device drivers.Mar 5 2025, 9:55 PM

Add manpage changes

Herald added a subscriber: ziaee. · View Herald TranscriptMar 5 2025, 10:06 PM

Harbormaster completed remote builds in B62777: Diff 151933.Mar 5 2025, 10:06 PM

Rebase

Harbormaster completed remote builds in B62800: Diff 151973.Mar 6 2025, 6:19 PM

The updated series survived multiple sleep/resume cycles on my X1 Carbon including waking due to key press and waking due to opening the lid. I don't have a way to test any of the WOL driver changes however.

jhb mentioned this in D49266: acpi_pci: Use pci_has_pm and pci_clear_pme.Mar 10 2025, 5:39 PM

This revision was not accepted when it landed; it landed in state Needs Review.Mar 27 2025, 9:00 PM

Closed by commit rG82d692771239: pci: Clear active PME# and disable PME# generation (authored by jhb). · Explain Why

This revision was automatically updated to reflect the committed changes.

jhb added a commit: rG82d692771239: pci: Clear active PME# and disable PME# generation.

jhb added a commit: rGb5bc47f1e1ab: pci: Clear active PME# and disable PME# generation.Apr 29 2025, 6:31 PM

pci: Clear active PME# and disable PME# generation
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 151973

share/man/man9/Makefile

share/man/man9/pci.9

sys/dev/cardbus/cardbus.c

sys/dev/pci/pci.c

sys/dev/pci/pcivar.h

pci: Clear active PME# and disable PME# generationClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 151973

share/man/man9/Makefile

share/man/man9/pci.9

sys/dev/cardbus/cardbus.c

sys/dev/pci/pci.c

sys/dev/pci/pcivar.h

pci: Clear active PME# and disable PME# generation
ClosedPublic
Actions

Revision Contents
Changeset List