Message-ID: <74f6598e-0220-46d3-9f53-ec9cb92e02d3@amd.com>
Date:   Wed, 6 Dec 2023 20:02:33 -0600
From:   Mario Limonciello <mario.limonciello@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        "open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision
 argument

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.

Powered by blists - more mailing lists