| Message ID | 20250401153225.96379-3-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VRAM Self Refresh | expand |
On Tue, Apr 01, 2025 at 09:02:15PM +0530, Anshuman Gupta wrote: > Implement _DSM Method 11 as per PCI firmware specs > section 4.6.11 Rev 3.3. "PCI Firmware r3.3, sec 4.6.11" so the citation is major to minor. "0xb" or "0Bh" to match spec usage. > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/pci/pci-acpi.c | 53 ++++++++++++++++++++++++++++++++++++++++ > include/linux/pci-acpi.h | 7 ++++++ > 2 files changed, 60 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index ebd49e43457e..04149f037664 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -1499,6 +1499,59 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power) > } > EXPORT_SYMBOL(pci_acpi_request_d3cold_aux_power); > > +/** > + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM > + * @dev: PCI device instance > + * @delay_us: Requested delay_us > + * > + * This function sends a request to the host BIOS via ACPI _DSM to grant the > + * required PERST# delay for the specified PCI device. It evaluates the _DSM > + * to request the PERST# delay and handles the response accordingly. > + * > + * Return: returns 0 on success and errno on failure. > + */ > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > +{ > + union acpi_object in_obj = { > + .integer.type = ACPI_TYPE_INTEGER, > + .integer.value = delay_us, > + }; > + > + union acpi_object *out_obj; > + acpi_handle handle; > + int result, ret = -EINVAL; > + > + if (!dev || !ACPI_HANDLE(&dev->dev)) > + return -EINVAL; > + > + handle = ACPI_HANDLE(&dev->dev); acpi_check_dsm(). > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, > + DSM_PCI_PERST_ASSERTION_DELAY, > + &in_obj, ACPI_TYPE_INTEGER); > + if (!out_obj) > + return -EINVAL; > + > + result = out_obj->integer.value; > + > + if (result == delay_us) { > + dev_info(&dev->dev, "PERST# Assertion Delay set to" > + "%u microseconds\n", delay_us); pci_info(). Join these into a single string, even though they won't fit in a line without wrapping. This is to make them easier to grep for when a user reports seeing the message. (Do this on the previous patch too, where I forgot to mention it.) > + ret = 0; > + } else if (result == 0) { > + dev_warn(&dev->dev, "PERST# Assertion Delay request failed," > + "no previous valid request\n"); > + } else { > + dev_warn(&dev->dev, > + "PERST# Assertion Delay request failed" > + "Previous valid delay: %u microseconds\n", result); > + } > + > + ACPI_FREE(out_obj); > + return ret; > +} > +EXPORT_SYMBOL(pci_acpi_add_perst_assertion_delay);
On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > Implement _DSM Method 11 as per PCI firmware specs > section 4.6.11 Rev 3.3. > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> I have basically the same comments as for the previous patch (in addition to Bjorn's comments). > --- > drivers/pci/pci-acpi.c | 53 ++++++++++++++++++++++++++++++++++++++++ > include/linux/pci-acpi.h | 7 ++++++ > 2 files changed, 60 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index ebd49e43457e..04149f037664 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -1499,6 +1499,59 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power) > } > EXPORT_SYMBOL(pci_acpi_request_d3cold_aux_power); > > +/** > + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM > + * @dev: PCI device instance > + * @delay_us: Requested delay_us How's the caller supposed to find out what value to use here? > + * > + * This function sends a request to the host BIOS via ACPI _DSM to grant the > + * required PERST# delay for the specified PCI device. It evaluates the _DSM > + * to request the PERST# delay and handles the response accordingly. > + * > + * Return: returns 0 on success and errno on failure. > + */ > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > +{ > + union acpi_object in_obj = { > + .integer.type = ACPI_TYPE_INTEGER, > + .integer.value = delay_us, > + }; > + > + union acpi_object *out_obj; > + acpi_handle handle; > + int result, ret = -EINVAL; > + > + if (!dev || !ACPI_HANDLE(&dev->dev)) > + return -EINVAL; > + > + handle = ACPI_HANDLE(&dev->dev); Please use ACPI_HANDLE() once. > + > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, This is something I haven't noticed in the previous patch, but also applies to it. Why is rev 4 of the interface hard-coded here? > + DSM_PCI_PERST_ASSERTION_DELAY, > + &in_obj, ACPI_TYPE_INTEGER); > + if (!out_obj) > + return -EINVAL; > + > + result = out_obj->integer.value; > + > + if (result == delay_us) { > + dev_info(&dev->dev, "PERST# Assertion Delay set to" > + "%u microseconds\n", delay_us); > + ret = 0; > + } else if (result == 0) { > + dev_warn(&dev->dev, "PERST# Assertion Delay request failed," > + "no previous valid request\n"); > + } else { > + dev_warn(&dev->dev, > + "PERST# Assertion Delay request failed" > + "Previous valid delay: %u microseconds\n", result); > + } > + > + ACPI_FREE(out_obj); > + return ret; > +} > +EXPORT_SYMBOL(pci_acpi_add_perst_assertion_delay); EXPORT_SYMBOL_GPL()? > + > static void pci_acpi_set_external_facing(struct pci_dev *dev) > { > u8 val; > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h > index dbc4b0ed433c..4b7373f91a9a 100644 > --- a/include/linux/pci-acpi.h > +++ b/include/linux/pci-acpi.h > @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid; > #define DSM_PCI_POWER_ON_RESET_DELAY 0x08 > #define DSM_PCI_DEVICE_READINESS_DURATIONS 0x09 > #define DSM_PCI_D3COLD_AUX_POWER_LIMIT 0x0A > +#define DSM_PCI_PERST_ASSERTION_DELAY 0x0B > > #ifdef CONFIG_PCIE_EDR > void pci_acpi_add_edr_notifier(struct pci_dev *pdev); > @@ -134,6 +135,7 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { } > int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *)); > void pci_acpi_clear_companion_lookup_hook(void); > int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power); > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us); > > #else /* CONFIG_ACPI */ > static inline void acpi_pci_add_bus(struct pci_bus *bus) { } > @@ -142,6 +144,11 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power) > { > return -EOPNOTSUPP; > } > + > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > +{ > + return -EOPNOTSUPP; > +} > #endif /* CONFIG_ACPI */ > > #endif /* _PCI_ACPI_H_ */ > --
On Wed, Apr 02, 2025 at 01:06:42PM +0200, Rafael J. Wysocki wrote: > On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > > > Implement _DSM Method 11 as per PCI firmware specs > > section 4.6.11 Rev 3.3. > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > > +{ > > + union acpi_object in_obj = { > > + .integer.type = ACPI_TYPE_INTEGER, > > + .integer.value = delay_us, > > + }; > > + > > + union acpi_object *out_obj; > > + acpi_handle handle; > > + int result, ret = -EINVAL; > > + > > + if (!dev || !ACPI_HANDLE(&dev->dev)) > > + return -EINVAL; > > + > > + handle = ACPI_HANDLE(&dev->dev); > > + > > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, > > This is something I haven't noticed in the previous patch, but also > applies to it. > > Why is rev 4 of the interface hard-coded here? Thanks for asking this because it's related to the whole _DSM revision question that I don't understand. If we didn't use rev 4 here, what should we use? The PCI Firmware spec, r3.3, sec 4.6.11, documents this interface and says "lowest valid Revision ID value is 4", so that's the source of the 4. My argument is that the spec documents rev 4, the kernel code was tested with rev 4, so what would be the benefit of using a different revision here? > > + DSM_PCI_PERST_ASSERTION_DELAY, > > + &in_obj, ACPI_TYPE_INTEGER);
On Wed, Apr 2, 2025 at 4:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Apr 02, 2025 at 01:06:42PM +0200, Rafael J. Wysocki wrote: > > On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > > > > > Implement _DSM Method 11 as per PCI firmware specs > > > section 4.6.11 Rev 3.3. > > > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > > > +{ > > > + union acpi_object in_obj = { > > > + .integer.type = ACPI_TYPE_INTEGER, > > > + .integer.value = delay_us, > > > + }; > > > + > > > + union acpi_object *out_obj; > > > + acpi_handle handle; > > > + int result, ret = -EINVAL; > > > + > > > + if (!dev || !ACPI_HANDLE(&dev->dev)) > > > + return -EINVAL; > > > + > > > + handle = ACPI_HANDLE(&dev->dev); > > > + > > > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, > > > > This is something I haven't noticed in the previous patch, but also > > applies to it. > > > > Why is rev 4 of the interface hard-coded here? > > Thanks for asking this because it's related to the whole _DSM revision > question that I don't understand. > > If we didn't use rev 4 here, what should we use? The PCI Firmware > spec, r3.3, sec 4.6.11, documents this interface and says "lowest > valid Revision ID value is 4", so that's the source of the 4. Well, the "lowest vaild Revision ID" does not generally mean the "only valid Revision ID". > My argument is that the spec documents rev 4, the kernel code was > tested with rev 4, so what would be the benefit of using a different > revision here? I'm talking about using a symbol to represent the number 4, not about possibly using a different number, along the lines of using, say, ACPI_FADT_LOW_POWER_S0 instead of putting BIT(21) directly into the code. The value is not likely to change, but using a symbol for representing it has merit (it can be meaningfully used in searches, it can be documented etc.). Now, I'm not sure how likely it is for the PCI Firmware spec to bump up the revision of this interface (I suppose that it will do so if a new function is defined), but even if it does so, the kernel will have to check both the new revision and rev 4 anyway, in case the firmware doesn't know about the new revision.
On Wed, Apr 02, 2025 at 04:52:55PM +0200, Rafael J. Wysocki wrote: > On Wed, Apr 2, 2025 at 4:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Apr 02, 2025 at 01:06:42PM +0200, Rafael J. Wysocki wrote: > > > On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > > > > > > > Implement _DSM Method 11 as per PCI firmware specs > > > > section 4.6.11 Rev 3.3. > > > > > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > > > > +{ > > > > + union acpi_object in_obj = { > > > > + .integer.type = ACPI_TYPE_INTEGER, > > > > + .integer.value = delay_us, > > > > + }; > > > > + > > > > + union acpi_object *out_obj; > > > > + acpi_handle handle; > > > > + int result, ret = -EINVAL; > > > > + > > > > + if (!dev || !ACPI_HANDLE(&dev->dev)) > > > > + return -EINVAL; > > > > + > > > > + handle = ACPI_HANDLE(&dev->dev); > > > > + > > > > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, > > > > > > This is something I haven't noticed in the previous patch, but also > > > applies to it. > > > > > > Why is rev 4 of the interface hard-coded here? > > > > Thanks for asking this because it's related to the whole _DSM revision > > question that I don't understand. > > > > If we didn't use rev 4 here, what should we use? The PCI Firmware > > spec, r3.3, sec 4.6.11, documents this interface and says "lowest > > valid Revision ID value is 4", so that's the source of the 4. > > Well, the "lowest valid Revision ID" does not generally mean the "only > valid Revision ID". True, but why should the kernel change from using the tested revision ID to some other revision ID? What would be the benefit of using rev 5? PCI Firmware r3.3 does contain a statement that "OSPM must invoke all Functions other than Function 0 with the same Revision ID value," but IMO that was a mistake, and we just approved an ECR to remove it. > > My argument is that the spec documents rev 4, the kernel code was > > tested with rev 4, so what would be the benefit of using a different > > revision here? > > I'm talking about using a symbol to represent the number 4, not about > possibly using a different number, along the lines of using, say, > ACPI_FADT_LOW_POWER_S0 instead of putting BIT(21) directly into the > code. > > The value is not likely to change, but using a symbol for representing > it has merit (it can be meaningfully used in searches, it can be > documented etc.). DSM_PCI_PERST_ASSERTION_DELAY (the function number) is a great thing to search for. I doubt a symbol for "4" would really add anything. At least in PCI, changes to a particular _DSM function are relatively rare. > Now, I'm not sure how likely it is for the PCI Firmware spec to bump > up the revision of this interface (I suppose that it will do so if a > new function is defined), but even if it does so, the kernel will have > to check both the new revision and rev 4 anyway, in case the firmware > doesn't know about the new revision. I guess the reason the OS would check both rev 5 and rev 4 would be that a new platform might implement DSM_PCI_PERST_ASSERTION_DELAY only at rev 5? I think this would be a real mistake in terms of implementing something to the spec. The spec documents DSM_PCI_PERST_ASSERTION_DELAY rev 4. Some platforms implemented and tested DSM_PCI_PERST_ASSERTION_DELAY rev 4. Linux added and tested support for DSM_PCI_PERST_ASSERTION_DELAY rev 4. I propose new platforms should also implement and test DSM_PCI_PERST_ASSERTION_DELAY rev 4. If a new platform implements only DSM_PCI_PERST_ASSERTION_DELAY rev 5, there is no actual documentation for that rev other than the spec assertion that "rev 5 must support all functions defined by previous revs of the UUID." IMO this is nonsense. The platform might have no need to implement all the functions defined for the UUID. Even if the platform makes all its functions valid for "the lowest valid Revision ID" through the "current Revision ID", there's nothing other than the spec assertion to guarantee that they all behave the same. And dealing with multiple revisions that are "supposed" to be the same just makes work and risk for the OS. Bjorn
On Wed, Apr 2, 2025 at 5:50 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Apr 02, 2025 at 04:52:55PM +0200, Rafael J. Wysocki wrote: > > On Wed, Apr 2, 2025 at 4:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Wed, Apr 02, 2025 at 01:06:42PM +0200, Rafael J. Wysocki wrote: > > > > On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > > > > > > > > > Implement _DSM Method 11 as per PCI firmware specs > > > > > section 4.6.11 Rev 3.3. > > > > > > > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > > > > > +{ > > > > > + union acpi_object in_obj = { > > > > > + .integer.type = ACPI_TYPE_INTEGER, > > > > > + .integer.value = delay_us, > > > > > + }; > > > > > + > > > > > + union acpi_object *out_obj; > > > > > + acpi_handle handle; > > > > > + int result, ret = -EINVAL; > > > > > + > > > > > + if (!dev || !ACPI_HANDLE(&dev->dev)) > > > > > + return -EINVAL; > > > > > + > > > > > + handle = ACPI_HANDLE(&dev->dev); > > > > > + > > > > > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, > > > > > > > > This is something I haven't noticed in the previous patch, but also > > > > applies to it. > > > > > > > > Why is rev 4 of the interface hard-coded here? > > > > > > Thanks for asking this because it's related to the whole _DSM revision > > > question that I don't understand. > > > > > > If we didn't use rev 4 here, what should we use? The PCI Firmware > > > spec, r3.3, sec 4.6.11, documents this interface and says "lowest > > > valid Revision ID value is 4", so that's the source of the 4. > > > > Well, the "lowest valid Revision ID" does not generally mean the "only > > valid Revision ID". > > True, but why should the kernel change from using the tested revision > ID to some other revision ID? What would be the benefit of using rev > 5? TBH I'm not exactly buying the "tested revision ID" concept because what does it really mean? If a given _DSM interface has been tested on one platform, does it necessarily mean that it will work on all of the other platforms implementing it? > PCI Firmware r3.3 does contain a statement that "OSPM must invoke all > Functions other than Function 0 with the same Revision ID value," but > IMO that was a mistake, and we just approved an ECR to remove it. > > > > My argument is that the spec documents rev 4, the kernel code was > > > tested with rev 4, so what would be the benefit of using a different > > > revision here? > > > > I'm talking about using a symbol to represent the number 4, not about > > possibly using a different number, along the lines of using, say, > > ACPI_FADT_LOW_POWER_S0 instead of putting BIT(21) directly into the > > code. > > > > The value is not likely to change, but using a symbol for representing > > it has merit (it can be meaningfully used in searches, it can be > > documented etc.). > > DSM_PCI_PERST_ASSERTION_DELAY (the function number) is a great thing > to search for. I doubt a symbol for "4" would really add anything. > At least in PCI, changes to a particular _DSM function are relatively > rare. > > > Now, I'm not sure how likely it is for the PCI Firmware spec to bump > > up the revision of this interface (I suppose that it will do so if a > > new function is defined), but even if it does so, the kernel will have > > to check both the new revision and rev 4 anyway, in case the firmware > > doesn't know about the new revision. > > I guess the reason the OS would check both rev 5 and rev 4 would be > that a new platform might implement DSM_PCI_PERST_ASSERTION_DELAY only > at rev 5? I think this would be a real mistake in terms of > implementing something to the spec. This is a real possibility, however, because there's nothing to prevent this kind of spec interpretation. I didn't mean this, though. Say the kernel wants to use a function that is only defined at rev 5. It will invoke function 0 at rev 5 to see if this function is available, but then it may as well see if the other functions it wants to use are available at rev 5 because it will get this information from the same _DSM call. If the platform firmware responds that they all are implemented, why won't the kernel use them all at rev 5? > The spec documents DSM_PCI_PERST_ASSERTION_DELAY rev 4. Some > platforms implemented and tested DSM_PCI_PERST_ASSERTION_DELAY rev 4. > Linux added and tested support for DSM_PCI_PERST_ASSERTION_DELAY rev > 4. I propose new platforms should also implement and test > DSM_PCI_PERST_ASSERTION_DELAY rev 4. > > If a new platform implements only DSM_PCI_PERST_ASSERTION_DELAY rev 5, > there is no actual documentation for that rev other than the spec > assertion that "rev 5 must support all functions defined by previous > revs of the UUID." IMO this is nonsense. The platform might have no > need to implement all the functions defined for the UUID. Sure. That's why function 0 should always be used. > Even if the platform makes all its functions valid for "the lowest > valid Revision ID" through the "current Revision ID", there's nothing > other than the spec assertion to guarantee that they all behave the > same. And dealing with multiple revisions that are "supposed" to be > the same just makes work and risk for the OS. You seem to be questioning the interface versioning at large by saying that there is no real guarantee of backwards compatibility between consecutive interface revisions on the same platform. Presumably, though, the interface is adhering to some specification which defines the behavior of the functions (and the set of available functions in the first place) for all of the valid revisions of it. The OS and the platform firmware both follow the same spec and so they should be mutually compatible. If this particular spec defines rev 5 to be an exact superset of rev 4, there is no reason to expect anything else.
On Wed, Apr 02, 2025 at 07:51:29PM +0200, Rafael J. Wysocki wrote: > On Wed, Apr 2, 2025 at 5:50 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Apr 02, 2025 at 04:52:55PM +0200, Rafael J. Wysocki wrote: > > > On Wed, Apr 2, 2025 at 4:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Wed, Apr 02, 2025 at 01:06:42PM +0200, Rafael J. Wysocki wrote: > > > > > On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > > > > > > > > > > > Implement _DSM Method 11 as per PCI firmware specs > > > > > > section 4.6.11 Rev 3.3. > > > > > > > > > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > > > > > > +{ > > > > > > + union acpi_object in_obj = { > > > > > > + .integer.type = ACPI_TYPE_INTEGER, > > > > > > + .integer.value = delay_us, > > > > > > + }; > > > > > > + > > > > > > + union acpi_object *out_obj; > > > > > > + acpi_handle handle; > > > > > > + int result, ret = -EINVAL; > > > > > > + > > > > > > + if (!dev || !ACPI_HANDLE(&dev->dev)) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + handle = ACPI_HANDLE(&dev->dev); > > > > > > + > > > > > > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, > > > > > > > > > > This is something I haven't noticed in the previous patch, but also > > > > > applies to it. > > > > > > > > > > Why is rev 4 of the interface hard-coded here? > > > > > > > > Thanks for asking this because it's related to the whole _DSM revision > > > > question that I don't understand. > > > > > > > > If we didn't use rev 4 here, what should we use? The PCI Firmware > > > > spec, r3.3, sec 4.6.11, documents this interface and says "lowest > > > > valid Revision ID value is 4", so that's the source of the 4. > > > > > > Well, the "lowest valid Revision ID" does not generally mean the "only > > > valid Revision ID". > > > > True, but why should the kernel change from using the tested revision > > ID to some other revision ID? What would be the benefit of using rev > > 5? > > TBH I'm not exactly buying the "tested revision ID" concept because > what does it really mean? > > If a given _DSM interface has been tested on one platform, does it > necessarily mean that it will work on all of the other platforms > implementing it? No, all we can say is that this OS rev 4 code works on the platforms we've tested. Other platforms might have their own defects. My point is that this OS code was written to the rev 4 spec and has been tested, and we shouldn't change the code or the revision it uses unless we're fixing something. > > > Now, I'm not sure how likely it is for the PCI Firmware spec to > > > bump up the revision of this interface (I suppose that it will > > > do so if a new function is defined), but even if it does so, the > > > kernel will have to check both the new revision and rev 4 > > > anyway, in case the firmware doesn't know about the new > > > revision. > > > > I guess the reason the OS would check both rev 5 and rev 4 would > > be that a new platform might implement > > DSM_PCI_PERST_ASSERTION_DELAY only at rev 5? I think this would > > be a real mistake in terms of implementing something to the spec. > > This is a real possibility, however, because there's nothing to > prevent this kind of spec interpretation. > > I didn't mean this, though. > > Say the kernel wants to use a function that is only defined at rev > 5. It will invoke function 0 at rev 5 to see if this function is > available, but then it may as well see if the other functions it > wants to use are available at rev 5 because it will get this > information from the same _DSM call. If the platform firmware > responds that they all are implemented, why won't the kernel use > them all at rev 5? This is an unnecessary change, since the OS tested its rev 4 code and now we're saying the OS should use rev 5 instead, which the OS did not test. The reason rev 5 exists at all is probably that some completely unrelated new function was added, and somebody decided it should be rev 5. I guess function 0 *could* have been defined to answer "yes/no" about whether a single (function, revision) is implemented. Then we probably wouldn't be having this conversation. But we do get an entire bitmask of implemented functions back from function 0, which allows the possibility of using the same revision for all the functions. I suppose we could have a boot-time function that calls function 0 with rev 0, 1, 2, ..., until it gets an empty bitmask, and saves the highest rev with a non-empty mask. In PCI we don't do that; instead, we call acpi_check_dsm() before every acpi_evaluate_dsm(). The main reason is that I don't think it's safe to change the function X rev just because function Y was added later with a higher revision. > > The spec documents DSM_PCI_PERST_ASSERTION_DELAY rev 4. Some > > platforms implemented and tested DSM_PCI_PERST_ASSERTION_DELAY rev > > 4. Linux added and tested support for > > DSM_PCI_PERST_ASSERTION_DELAY rev 4. I propose new platforms > > should also implement and test DSM_PCI_PERST_ASSERTION_DELAY rev > > 4. > > > > If a new platform implements only DSM_PCI_PERST_ASSERTION_DELAY > > rev 5, there is no actual documentation for that rev other than > > the spec assertion that "rev 5 must support all functions defined > > by previous revs of the UUID." IMO this is nonsense. The > > platform might have no need to implement all the functions defined > > for the UUID. > > Sure. That's why function 0 should always be used. Yes. But the requirement that rev 5 must support all functions ever previously documented for the UUID still makes no sense to me. I don't think there's any requirement that a platform implement ALL of the documented PCI functions. Maybe the intent is that this only applies to a given platform, i.e., that new firmware for that platform must continue to support all functions and revisions that were ever supported on that platform? That seems obvious to me and hardly worth mentioning. > > Even if the platform makes all its functions valid for "the lowest > > valid Revision ID" through the "current Revision ID", there's > > nothing other than the spec assertion to guarantee that they all > > behave the same. And dealing with multiple revisions that are > > "supposed" to be the same just makes work and risk for the OS. > > You seem to be questioning the interface versioning at large by > saying that there is no real guarantee of backwards compatibility > between consecutive interface revisions on the same platform. That's exactly what I'm saying, although I think problems are more likely across different platforms. There's the requirement in the spec, but that's just words on a page. There's no way to enforce or validate it. > Presumably, though, the interface is adhering to some specification > which defines the behavior of the functions (and the set of > available functions in the first place) for all of the valid > revisions of it. The OS and the platform firmware both follow the > same spec and so they should be mutually compatible. If this > particular spec defines rev 5 to be an exact superset of rev 4, > there is no reason to expect anything else. I don't *expect* rev 5 to be different. But as a user of it, why would I change working, tested code that is not broken? The PCI DPC function 0xc is an example where rev 5 (per ECN) and rev 6 (per r3.3) are not compatible. If the OS implemented rev 5, then learns via function 0 that function 0xc is also supported at rev 6, and starts using the same OS code with rev 6, the OS is broken. Bjorn
On Wed, Apr 2, 2025 at 8:48 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Apr 02, 2025 at 07:51:29PM +0200, Rafael J. Wysocki wrote: > > On Wed, Apr 2, 2025 at 5:50 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Wed, Apr 02, 2025 at 04:52:55PM +0200, Rafael J. Wysocki wrote: > > > > On Wed, Apr 2, 2025 at 4:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > On Wed, Apr 02, 2025 at 01:06:42PM +0200, Rafael J. Wysocki wrote: > > > > > > On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > > > > > > > > > > > > > Implement _DSM Method 11 as per PCI firmware specs > > > > > > > section 4.6.11 Rev 3.3. > > > > > > > > > > > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) > > > > > > > +{ > > > > > > > + union acpi_object in_obj = { > > > > > > > + .integer.type = ACPI_TYPE_INTEGER, > > > > > > > + .integer.value = delay_us, > > > > > > > + }; > > > > > > > + > > > > > > > + union acpi_object *out_obj; > > > > > > > + acpi_handle handle; > > > > > > > + int result, ret = -EINVAL; > > > > > > > + > > > > > > > + if (!dev || !ACPI_HANDLE(&dev->dev)) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > + handle = ACPI_HANDLE(&dev->dev); > > > > > > > + > > > > > > > + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, > > > > > > > > > > > > This is something I haven't noticed in the previous patch, but also > > > > > > applies to it. > > > > > > > > > > > > Why is rev 4 of the interface hard-coded here? > > > > > > > > > > Thanks for asking this because it's related to the whole _DSM revision > > > > > question that I don't understand. > > > > > > > > > > If we didn't use rev 4 here, what should we use? The PCI Firmware > > > > > spec, r3.3, sec 4.6.11, documents this interface and says "lowest > > > > > valid Revision ID value is 4", so that's the source of the 4. > > > > > > > > Well, the "lowest valid Revision ID" does not generally mean the "only > > > > valid Revision ID". > > > > > > True, but why should the kernel change from using the tested revision > > > ID to some other revision ID? What would be the benefit of using rev > > > 5? > > > > TBH I'm not exactly buying the "tested revision ID" concept because > > what does it really mean? > > > > If a given _DSM interface has been tested on one platform, does it > > necessarily mean that it will work on all of the other platforms > > implementing it? > > No, all we can say is that this OS rev 4 code works on the platforms > we've tested. Other platforms might have their own defects. > > My point is that this OS code was written to the rev 4 spec and has > been tested, and we shouldn't change the code or the revision it uses > unless we're fixing something. Well, if the spec says "rev 5 is the same as rev 4 except for function Y which is new in rev 5", then arguably the same OS code is also compliant with rev 5 (it will not use function Y, but that's fine). If the platform firmware follows the same spec, then the rev 5 implementation in it can be expected to work exactly as its rev 4 implementation either. I see no problem with using rev 5 instead of rev 4 in that case and nn new platforms, rev 5 is just as risky as rev 4. > > > > Now, I'm not sure how likely it is for the PCI Firmware spec to > > > > bump up the revision of this interface (I suppose that it will > > > > do so if a new function is defined), but even if it does so, the > > > > kernel will have to check both the new revision and rev 4 > > > > anyway, in case the firmware doesn't know about the new > > > > revision. > > > > > > I guess the reason the OS would check both rev 5 and rev 4 would > > > be that a new platform might implement > > > DSM_PCI_PERST_ASSERTION_DELAY only at rev 5? I think this would > > > be a real mistake in terms of implementing something to the spec. > > > > This is a real possibility, however, because there's nothing to > > prevent this kind of spec interpretation. > > > > I didn't mean this, though. > > > > Say the kernel wants to use a function that is only defined at rev > > 5. It will invoke function 0 at rev 5 to see if this function is > > available, but then it may as well see if the other functions it > > wants to use are available at rev 5 because it will get this > > information from the same _DSM call. If the platform firmware > > responds that they all are implemented, why won't the kernel use > > them all at rev 5? > > This is an unnecessary change, since the OS tested its rev 4 code and > now we're saying the OS should use rev 5 instead, which the OS did not > test. Yes, it did. It is the same code, as per the above. > The reason rev 5 exists at all is probably that some completely > unrelated new function was added, and somebody decided it should be > rev 5. > > I guess function 0 *could* have been defined to answer "yes/no" about > whether a single (function, revision) is implemented. Then we > probably wouldn't be having this conversation. > > But we do get an entire bitmask of implemented functions back from > function 0, which allows the possibility of using the same revision > for all the functions. I suppose we could have a boot-time function > that calls function 0 with rev 0, 1, 2, ..., until it gets an empty > bitmask, and saves the highest rev with a non-empty mask. > > In PCI we don't do that; instead, we call acpi_check_dsm() before > every acpi_evaluate_dsm(). The main reason is that I don't think it's > safe to change the function X rev just because function Y was added > later with a higher revision. So what if there is a platform that only implements revisions greater than X? Presumably, there is another OS targeted by this platform that only uses revisions X+1 and forward. > > > The spec documents DSM_PCI_PERST_ASSERTION_DELAY rev 4. Some > > > platforms implemented and tested DSM_PCI_PERST_ASSERTION_DELAY rev > > > 4. Linux added and tested support for > > > DSM_PCI_PERST_ASSERTION_DELAY rev 4. I propose new platforms > > > should also implement and test DSM_PCI_PERST_ASSERTION_DELAY rev > > > 4. > > > > > > If a new platform implements only DSM_PCI_PERST_ASSERTION_DELAY > > > rev 5, there is no actual documentation for that rev other than > > > the spec assertion that "rev 5 must support all functions defined > > > by previous revs of the UUID." IMO this is nonsense. The > > > platform might have no need to implement all the functions defined > > > for the UUID. > > > > Sure. That's why function 0 should always be used. > > Yes. But the requirement that rev 5 must support all functions ever > previously documented for the UUID still makes no sense to me. I > don't think there's any requirement that a platform implement ALL of > the documented PCI functions. I think that there is some confusion in the spec language that seems to be conflating platforms with interface definitions. It ostensibly wants the definitions of subsequent revisions of the same interface to be consistent with each other, which is not a bad thing in principle IMV, but at the same time it ostensibly allows platforms to choose which functions to implement, and it doesn't say anything about the expected behavior when the OS attempts to use a function that is not implemented for a given rev. You are right that the backward compatibility language in the _DSM definition is not enforceable, but it can be regarded as a guidance: Do not change the interface that you have once defined arbitrarily between revisions. After all, if you don't like this guidance, you can just allocate a new UUID and define a new interface based on it. Anyway, I'm not the author of this piece of the specification, so I don't really know the original intent, and it is not sufficiently clear. > Maybe the intent is that this only applies to a given platform, i.e., > that new firmware for that platform must continue to support all > functions and revisions that were ever supported on that platform? > That seems obvious to me and hardly worth mentioning. > > > > Even if the platform makes all its functions valid for "the lowest > > > valid Revision ID" through the "current Revision ID", there's > > > nothing other than the spec assertion to guarantee that they all > > > behave the same. And dealing with multiple revisions that are > > > "supposed" to be the same just makes work and risk for the OS. > > > > You seem to be questioning the interface versioning at large by > > saying that there is no real guarantee of backwards compatibility > > between consecutive interface revisions on the same platform. > > That's exactly what I'm saying, although I think problems are more > likely across different platforms. There's the requirement in the > spec, but that's just words on a page. There's no way to enforce or > validate it. > > > Presumably, though, the interface is adhering to some specification > > which defines the behavior of the functions (and the set of > > available functions in the first place) for all of the valid > > revisions of it. The OS and the platform firmware both follow the > > same spec and so they should be mutually compatible. If this > > particular spec defines rev 5 to be an exact superset of rev 4, > > there is no reason to expect anything else. > > I don't *expect* rev 5 to be different. But as a user of it, why > would I change working, tested code that is not broken? > > The PCI DPC function 0xc is an example where rev 5 (per ECN) and rev 6 > (per r3.3) are not compatible. > > If the OS implemented rev 5, then learns via function 0 that function > 0xc is also supported at rev 6, and starts using the same OS code with > rev 6, the OS is broken. Yes, in this case the backward compatibility language in the _DSM definition is obviously not followed.
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index ebd49e43457e..04149f037664 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -1499,6 +1499,59 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power) } EXPORT_SYMBOL(pci_acpi_request_d3cold_aux_power); +/** + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM + * @dev: PCI device instance + * @delay_us: Requested delay_us + * + * This function sends a request to the host BIOS via ACPI _DSM to grant the + * required PERST# delay for the specified PCI device. It evaluates the _DSM + * to request the PERST# delay and handles the response accordingly. + * + * Return: returns 0 on success and errno on failure. + */ +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) +{ + union acpi_object in_obj = { + .integer.type = ACPI_TYPE_INTEGER, + .integer.value = delay_us, + }; + + union acpi_object *out_obj; + acpi_handle handle; + int result, ret = -EINVAL; + + if (!dev || !ACPI_HANDLE(&dev->dev)) + return -EINVAL; + + handle = ACPI_HANDLE(&dev->dev); + + out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4, + DSM_PCI_PERST_ASSERTION_DELAY, + &in_obj, ACPI_TYPE_INTEGER); + if (!out_obj) + return -EINVAL; + + result = out_obj->integer.value; + + if (result == delay_us) { + dev_info(&dev->dev, "PERST# Assertion Delay set to" + "%u microseconds\n", delay_us); + ret = 0; + } else if (result == 0) { + dev_warn(&dev->dev, "PERST# Assertion Delay request failed," + "no previous valid request\n"); + } else { + dev_warn(&dev->dev, + "PERST# Assertion Delay request failed" + "Previous valid delay: %u microseconds\n", result); + } + + ACPI_FREE(out_obj); + return ret; +} +EXPORT_SYMBOL(pci_acpi_add_perst_assertion_delay); + static void pci_acpi_set_external_facing(struct pci_dev *dev) { u8 val; diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index dbc4b0ed433c..4b7373f91a9a 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid; #define DSM_PCI_POWER_ON_RESET_DELAY 0x08 #define DSM_PCI_DEVICE_READINESS_DURATIONS 0x09 #define DSM_PCI_D3COLD_AUX_POWER_LIMIT 0x0A +#define DSM_PCI_PERST_ASSERTION_DELAY 0x0B #ifdef CONFIG_PCIE_EDR void pci_acpi_add_edr_notifier(struct pci_dev *pdev); @@ -134,6 +135,7 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { } int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *)); void pci_acpi_clear_companion_lookup_hook(void); int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power); +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us); #else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } @@ -142,6 +144,11 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power) { return -EOPNOTSUPP; } + +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us) +{ + return -EOPNOTSUPP; +} #endif /* CONFIG_ACPI */ #endif /* _PCI_ACPI_H_ */
Implement _DSM Method 11 as per PCI firmware specs section 4.6.11 Rev 3.3. Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/pci/pci-acpi.c | 53 ++++++++++++++++++++++++++++++++++++++++ include/linux/pci-acpi.h | 7 ++++++ 2 files changed, 60 insertions(+)