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
