On 12/6/2023 17:04, Bjorn Helgaas wrote:
On Tue, Dec 05, 2023 at 02:12:54PM -0600, Mario Limonciello wrote:
On 11/30/2023 16:29, Bjorn Helgaas wrote:
On Fri, Nov 10, 2023 at 12:55:01PM -0600, Mario Limonciello wrote:
The PCI ACPI _DSM is called across multiple places in the PCI core
with different arguments for the revision.
The PCI firmware specification specifies that this is an incorrect
behavior.
"OSPM must invoke all Functions other than Function 0 with the
same Revision ID value"
This patch passes the same or a larger Revision ID than before, so
everything should work the same because the spec requires backwards
compatibility. But it's conceivable that it could break on firmware
that does the revision check incorrectly.
Is this fixing a problem other than the spec compliance issue?
It was just a spec compliance issue I noticed when implementing the other
two patches.
I agree the PCI FW spec says this. It was added in r3.3 by the ECN at
https://members.pcisig.com/wg/Firmware/document/previewpdf/13988, but
I don't quite understand that ECN.
ACPI r6.5, sec 9.1.1, doesn't include the "must invoke all Functions
with same Revision ID" language, and the ASL example there clearly
treats revisions higher than those implemented by firmware as valid,
although new Functions added by those higher revisions are obviously
not supported.
PCI FW also says OSPM should not use a fixed Revision ID, but should
start with the highest known revision and "successively invoke
Function 0 with decremented values of Revision ID until system
firmware returns a value indicating support for more than Function 0"
(added by the same ECN), and I don't think Linux does this part.
So I think the fixed "pci_acpi_dsm_rev" value as in your patch works
fine with the ACPI ASL example, but it doesn't track the "successively
decrement" part of PCI FW. I don't know the reason for that part of
the ECN.
Do you think it's better to respin to take this into account and be more
stringent or "do nothing"?
To me, it seems better to do nothing unless a change would solve a
problem. I raised it as a question to the PCI Firmware workgroup
(https://members.pcisig.com/wg/Firmware/mail/thread/32031), but I
haven't heard anything.
Regrettably, that link only works for PCI-SIG members; here's the text
of my question:
Sorry to reopen this old topic. This ECN was approved and appears
in r3.3. We're contemplating Linux changes to conform to it.
I think I understand the ACPI requirement for OSPM to invoke _DSM
Function 0 to learn whether a Function is supported (because a
non-zero Function may have completely arbitrary return values, so
invoking that Function has no way to return "this Function Index
isn't supported").
I don't understand why it's important for OSPM to "invoke all
Functions other than Function 0 with the same Revision ID." That
idea doesn't appear in ACPI r6.5, sec 9.1.1 or in the sample ASL
there. Is there a benefit to using the same Revision ID for all
Functions? (Of course OSPM must invoke Function 0 with Revision ID
N to learn whether Function X is supported for Revision ID N.)
I also don't understand why "OSPM should successively invoke
Function 0 with decremented values of Revision ID until system
firmware returns a value indicating support for more than Function
0." ACPI r6.5 doesn't suggest that, and the sample ASL returns
different bitmasks depending on the Revision ID supplied by OSPM,
including a default case that returns a bitmask including all
Functions implemented by the firmware if OSPM supplied a higher
Revision ID from the future. What is the benefit of probing with
decremented Revision IDs?
Is there something PCI-specific here, or should these requirements
be in the ACPI spec instead of the PCI Firmware spec?
Unrelated to this patch, I think it's a bug that Linux fails to invoke
Function 0 in a few cases, e.g., DSM_PCI_PRESERVE_BOOT_CONFIG,
DSM_PCI_POWER_ON_RESET_DELAY, and DSM_PCI_DEVICE_READINESS_DURATIONS.
Per spec, OSPM must invoke Function 0 to learn which other Functions
are supported. It's not explicitly stated, but I think this is
required because a supported non-zero Function may return "any data
object", so there's no return value that would mean "this Function
Index is not supported."
What are your thoughts on the other two patches in the series?
Should they wait for a consumer or prepare the API to match the spec.
I'd prefer to wait until there are users of the new functions.
There's no real benefit to adding functions that are never called.
Bjorn
OK thanks - I'll put aside the whole series for now. The first
immediate consumers of this would be GPU driver, but it's only needed in
very specific circumstances that haven't come up yet in practice.
