Page MenuHomeFreeBSD

acpi_pcib: Don't implement the ACPI flags IVAR
Needs ReviewPublic

Authored by jhb on Fri, Feb 27, 3:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 18, 11:21 PM
Unknown Object (File)
Wed, Mar 18, 10:14 PM
Unknown Object (File)
Tue, Mar 17, 11:10 PM
Unknown Object (File)
Tue, Mar 17, 11:05 PM
Unknown Object (File)
Thu, Mar 12, 2:35 AM
Unknown Object (File)
Thu, Mar 12, 12:19 AM
Unknown Object (File)
Tue, Mar 10, 11:22 PM
Unknown Object (File)
Sun, Mar 8, 12:57 AM
Subscribers
None

Details

Summary

This causes the immediate pciY children of top-level pcibX devices to
have a duplicate "wake" sysctl and duplicate set of flags that are not
kept in sync with the sysctl and flags for the parent pcibX device
managed by the acpi(4) driver.

Note that PCI devices on an ACPI device such as PCI-PCI bridge devices
have "wake" sysctls provided by the ACPI flags IVAR in the acpi_pci
driver.

The end result is that for each PCI "bus" there should only be a
single "wake" sysctl on the parent bridge device if the bus handle has
a _PRW method.

Diff Detail

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

Event Timeline

I'm a bit puzzled... how does this differ from the other revisions?

I'm a bit puzzled... how does this differ from the other revisions?

This is a conceptual cleanup.

Today, a given ACPI handle for a PCI "bus" has two new-bus devices associated with that handle: the pcibX device and the pciY child bus. The pcibX devices use two different drivers (acpi_pcib_acpi.c and acpi_pcib_pci.c depending on if it is a top-level bridge (former) or a PCI-PCI bridge (latter)). pciY devices always use the acpi_pci.c driver. Right now, the acpi_pcib_acpi.c and acpi_pci.c drivers (but _not_ the acpi_pcib_pci.c driver) implement the "flags" IVAR, which means that when you have a top-level PCI bus that implements _PRW (e.g. PCI bus 0), both pcib0 and pci0 have a "wake" sysctl node added. But they are separate nodes, and they reference separate variables (pcib0 in the acpi_pcib_acpi.c softc, and pci0 in the acpi_pci.c softc). If you change the value of one of the sysctls, only one of the variables gets updated, so the other sysctl is then "wrong". (So you can have pci0.wake == 0, and pcib0.wake == 1, and it's your guess as the user as to which one represents what is actually true in the hardware.) OTOH, for PCI-PCI bridges, the pcibX devices don't have a "wake" sysctl at all, only the child pciY devices since the acpi_pcib_pci.c driver doesn't implement the IVAR.

By removing the IVAR from this driver, we are removing the "wake" sysctl node from pcib0 (and bridges for other top-level PCI buses). This means that the "wake" node will only ever exist for pciY devices, and since there is only one node, it will always be in sync and match what the hardware thinks is true, and the user experience is that if for some reason you want to enable wake for a PCI bridge, you can just set it on the child PCI bus device. The alternative fix would be to remove the flags IVAR from acpi_pci.c and add it to acpi_pcib_pci.c. In that case, the "wake" node would only exist for pcibX devices, but not for pciY devices. Either way, we need to avoid the duplicate nodes that get out of sync. I chose this route because it is simplest, but the other route would be fine with me as well.

OTOH, it's not at all clear to me why you would ever want "wake for device" enabled for a PCI bridge. If a leaf PCI device needs all its parent bridges enabled up the tree, when we should implement that logic instead of having a node on bridge devices, but the ACPI spec doesn't really clarify what _PRW means on a bridge device. There is some reference to the fact that you might use bus-specific logic to identify a child device (which PCI has via the PME registers). I suspect it is true that we should really not be advertising wake nodes for bridges but automatically treating this value as 1 if any child device of a bridge has wake enabled. @obiwac may have further thoughts about this meta question.