| Message ID | 20231110185503.46117-2-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add API for missing PCI firmware specification funcs | expand |
On Fri, Nov 10, 2023 at 9:32 PM Mario Limonciello <mario.limonciello@amd.com> 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" > > Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 > PCI Firmware specification 3.3, section 4.6 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> and I haven't seen much activity related to this series, so I'm not sure what's happening to it. Regardless, I think that the remaining two patches are better sent along with the first users of the new APIs. > --- > drivers/acpi/pci_root.c | 3 ++- > drivers/pci/pci-acpi.c | 6 ++++-- > drivers/pci/pci-label.c | 4 ++-- > drivers/pci/pcie/edr.c | 13 +++++++------ > include/linux/pci-acpi.h | 1 + > 5 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 58b89b8d950e..bca2270a93d4 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > * exists and returns 0, we must preserve any PCI resource > * assignments made by firmware for this host bridge. > */ > - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), > + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); > if (obj && obj->integer.value == 0) > host_bridge->preserve_config = 1; > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 004575091596..bea72e807817 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -28,6 +28,7 @@ > const guid_t pci_acpi_dsm_guid = > GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a, > 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d); > +const int pci_acpi_dsm_rev = 5; > > #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) > static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res) > @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus) > if (!pci_is_root_bus(bus)) > return; > > - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3, > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), > + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER); > if (!obj) > return; > @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, > if (bridge->ignore_reset_delay) > pdev->d3cold_delay = 0; > > - obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3, > + obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_DEVICE_READINESS_DURATIONS, NULL, > ACPI_TYPE_PACKAGE); > if (!obj) > diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c > index 0c6446519640..91bdd04029f0 100644 > --- a/drivers/pci/pci-label.c > +++ b/drivers/pci/pci-label.c > @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev) > if (!handle) > return false; > > - return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2, > + return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > 1 << DSM_PCI_DEVICE_NAME); > #else > return false; > @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf, > if (!handle) > return -1; > > - obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2, > + obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_DEVICE_NAME, NULL); > if (!obj) > return -1; > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index 5f4914d313a1..ab6a50201124 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev) > * Behavior when calling unsupported _DSM functions is undefined, > * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. > */ > - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > 1ULL << EDR_PORT_DPC_ENABLE_DSM)) > return 0; > > @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev) > * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is > * optional. Return success if it's not implemented. > */ > - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > - EDR_PORT_DPC_ENABLE_DSM, &argv4); > + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, > + pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM, > + &argv4); > if (!obj) > return 0; > > @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) > * Behavior when calling unsupported _DSM functions is undefined, > * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. > */ > - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > 1ULL << EDR_PORT_LOCATE_DSM)) > return pci_dev_get(pdev); > > - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > - EDR_PORT_LOCATE_DSM, NULL); > + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, > + pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL); > if (!obj) > return pci_dev_get(pdev); > > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h > index 078225b514d4..7966ef8f14b3 100644 > --- a/include/linux/pci-acpi.h > +++ b/include/linux/pci-acpi.h > @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { } > #endif > > extern const guid_t pci_acpi_dsm_guid; > +extern const int pci_acpi_dsm_rev; > > /* _DSM Definitions for PCI */ > #define DSM_PCI_PRESERVE_BOOT_CONFIG 0x05 > -- > 2.34.1 > >
On 11/30/2023 07:34, Rafael J. Wysocki wrote: > On Fri, Nov 10, 2023 at 9:32 PM Mario Limonciello > <mario.limonciello@amd.com> 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" >> >> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 >> PCI Firmware specification 3.3, section 4.6 >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Thanks! > and I haven't seen much activity related to this series, so I'm not > sure what's happening to it. > > Regardless, I think that the remaining two patches are better sent > along with the first users of the new APIs. Bjorn, If you agree, can you please queue up the first patch? I'll shuffle the others into the back of my mind for if/when they're needed. Thanks! > >> --- >> drivers/acpi/pci_root.c | 3 ++- >> drivers/pci/pci-acpi.c | 6 ++++-- >> drivers/pci/pci-label.c | 4 ++-- >> drivers/pci/pcie/edr.c | 13 +++++++------ >> include/linux/pci-acpi.h | 1 + >> 5 files changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index 58b89b8d950e..bca2270a93d4 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> * exists and returns 0, we must preserve any PCI resource >> * assignments made by firmware for this host bridge. >> */ >> - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, >> + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), >> + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); >> if (obj && obj->integer.value == 0) >> host_bridge->preserve_config = 1; >> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >> index 004575091596..bea72e807817 100644 >> --- a/drivers/pci/pci-acpi.c >> +++ b/drivers/pci/pci-acpi.c >> @@ -28,6 +28,7 @@ >> const guid_t pci_acpi_dsm_guid = >> GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a, >> 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d); >> +const int pci_acpi_dsm_rev = 5; >> >> #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) >> static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res) >> @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus) >> if (!pci_is_root_bus(bus)) >> return; >> >> - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3, >> + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), >> + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER); >> if (!obj) >> return; >> @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, >> if (bridge->ignore_reset_delay) >> pdev->d3cold_delay = 0; >> >> - obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3, >> + obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_DEVICE_READINESS_DURATIONS, NULL, >> ACPI_TYPE_PACKAGE); >> if (!obj) >> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c >> index 0c6446519640..91bdd04029f0 100644 >> --- a/drivers/pci/pci-label.c >> +++ b/drivers/pci/pci-label.c >> @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev) >> if (!handle) >> return false; >> >> - return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2, >> + return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> 1 << DSM_PCI_DEVICE_NAME); >> #else >> return false; >> @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf, >> if (!handle) >> return -1; >> >> - obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2, >> + obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_DEVICE_NAME, NULL); >> if (!obj) >> return -1; >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >> index 5f4914d313a1..ab6a50201124 100644 >> --- a/drivers/pci/pcie/edr.c >> +++ b/drivers/pci/pcie/edr.c >> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev) >> * Behavior when calling unsupported _DSM functions is undefined, >> * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. >> */ >> - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> 1ULL << EDR_PORT_DPC_ENABLE_DSM)) >> return 0; >> >> @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev) >> * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is >> * optional. Return success if it's not implemented. >> */ >> - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> - EDR_PORT_DPC_ENABLE_DSM, &argv4); >> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, >> + pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM, >> + &argv4); >> if (!obj) >> return 0; >> >> @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) >> * Behavior when calling unsupported _DSM functions is undefined, >> * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. >> */ >> - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> 1ULL << EDR_PORT_LOCATE_DSM)) >> return pci_dev_get(pdev); >> >> - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> - EDR_PORT_LOCATE_DSM, NULL); >> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, >> + pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL); >> if (!obj) >> return pci_dev_get(pdev); >> >> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h >> index 078225b514d4..7966ef8f14b3 100644 >> --- a/include/linux/pci-acpi.h >> +++ b/include/linux/pci-acpi.h >> @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { } >> #endif >> >> extern const guid_t pci_acpi_dsm_guid; >> +extern const int pci_acpi_dsm_rev; >> >> /* _DSM Definitions for PCI */ >> #define DSM_PCI_PRESERVE_BOOT_CONFIG 0x05 >> -- >> 2.34.1 >> >>
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? 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. 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." Bjorn > Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 > PCI Firmware specification 3.3, section 4.6 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/acpi/pci_root.c | 3 ++- > drivers/pci/pci-acpi.c | 6 ++++-- > drivers/pci/pci-label.c | 4 ++-- > drivers/pci/pcie/edr.c | 13 +++++++------ > include/linux/pci-acpi.h | 1 + > 5 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 58b89b8d950e..bca2270a93d4 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > * exists and returns 0, we must preserve any PCI resource > * assignments made by firmware for this host bridge. > */ > - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), > + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); > if (obj && obj->integer.value == 0) > host_bridge->preserve_config = 1; > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 004575091596..bea72e807817 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -28,6 +28,7 @@ > const guid_t pci_acpi_dsm_guid = > GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a, > 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d); > +const int pci_acpi_dsm_rev = 5; > > #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) > static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res) > @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus) > if (!pci_is_root_bus(bus)) > return; > > - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3, > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), > + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER); > if (!obj) > return; > @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, > if (bridge->ignore_reset_delay) > pdev->d3cold_delay = 0; > > - obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3, > + obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_DEVICE_READINESS_DURATIONS, NULL, > ACPI_TYPE_PACKAGE); > if (!obj) > diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c > index 0c6446519640..91bdd04029f0 100644 > --- a/drivers/pci/pci-label.c > +++ b/drivers/pci/pci-label.c > @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev) > if (!handle) > return false; > > - return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2, > + return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > 1 << DSM_PCI_DEVICE_NAME); > #else > return false; > @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf, > if (!handle) > return -1; > > - obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2, > + obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > DSM_PCI_DEVICE_NAME, NULL); > if (!obj) > return -1; > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index 5f4914d313a1..ab6a50201124 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev) > * Behavior when calling unsupported _DSM functions is undefined, > * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. > */ > - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > 1ULL << EDR_PORT_DPC_ENABLE_DSM)) > return 0; > > @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev) > * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is > * optional. Return success if it's not implemented. > */ > - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > - EDR_PORT_DPC_ENABLE_DSM, &argv4); > + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, > + pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM, > + &argv4); > if (!obj) > return 0; > > @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) > * Behavior when calling unsupported _DSM functions is undefined, > * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. > */ > - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, > 1ULL << EDR_PORT_LOCATE_DSM)) > return pci_dev_get(pdev); > > - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > - EDR_PORT_LOCATE_DSM, NULL); > + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, > + pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL); > if (!obj) > return pci_dev_get(pdev); > > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h > index 078225b514d4..7966ef8f14b3 100644 > --- a/include/linux/pci-acpi.h > +++ b/include/linux/pci-acpi.h > @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { } > #endif > > extern const guid_t pci_acpi_dsm_guid; > +extern const int pci_acpi_dsm_rev; > > /* _DSM Definitions for PCI */ > #define DSM_PCI_PRESERVE_BOOT_CONFIG 0x05 > -- > 2.34.1 >
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"? > 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." > > Bjorn 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. > >> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 >> PCI Firmware specification 3.3, section 4.6 >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/acpi/pci_root.c | 3 ++- >> drivers/pci/pci-acpi.c | 6 ++++-- >> drivers/pci/pci-label.c | 4 ++-- >> drivers/pci/pcie/edr.c | 13 +++++++------ >> include/linux/pci-acpi.h | 1 + >> 5 files changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index 58b89b8d950e..bca2270a93d4 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> * exists and returns 0, we must preserve any PCI resource >> * assignments made by firmware for this host bridge. >> */ >> - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, >> + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), >> + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); >> if (obj && obj->integer.value == 0) >> host_bridge->preserve_config = 1; >> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >> index 004575091596..bea72e807817 100644 >> --- a/drivers/pci/pci-acpi.c >> +++ b/drivers/pci/pci-acpi.c >> @@ -28,6 +28,7 @@ >> const guid_t pci_acpi_dsm_guid = >> GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a, >> 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d); >> +const int pci_acpi_dsm_rev = 5; >> >> #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) >> static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res) >> @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus) >> if (!pci_is_root_bus(bus)) >> return; >> >> - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3, >> + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), >> + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER); >> if (!obj) >> return; >> @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, >> if (bridge->ignore_reset_delay) >> pdev->d3cold_delay = 0; >> >> - obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3, >> + obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_DEVICE_READINESS_DURATIONS, NULL, >> ACPI_TYPE_PACKAGE); >> if (!obj) >> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c >> index 0c6446519640..91bdd04029f0 100644 >> --- a/drivers/pci/pci-label.c >> +++ b/drivers/pci/pci-label.c >> @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev) >> if (!handle) >> return false; >> >> - return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2, >> + return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> 1 << DSM_PCI_DEVICE_NAME); >> #else >> return false; >> @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf, >> if (!handle) >> return -1; >> >> - obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2, >> + obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> DSM_PCI_DEVICE_NAME, NULL); >> if (!obj) >> return -1; >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >> index 5f4914d313a1..ab6a50201124 100644 >> --- a/drivers/pci/pcie/edr.c >> +++ b/drivers/pci/pcie/edr.c >> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev) >> * Behavior when calling unsupported _DSM functions is undefined, >> * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. >> */ >> - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> 1ULL << EDR_PORT_DPC_ENABLE_DSM)) >> return 0; >> >> @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev) >> * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is >> * optional. Return success if it's not implemented. >> */ >> - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> - EDR_PORT_DPC_ENABLE_DSM, &argv4); >> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, >> + pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM, >> + &argv4); >> if (!obj) >> return 0; >> >> @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) >> * Behavior when calling unsupported _DSM functions is undefined, >> * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. >> */ >> - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, >> 1ULL << EDR_PORT_LOCATE_DSM)) >> return pci_dev_get(pdev); >> >> - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> - EDR_PORT_LOCATE_DSM, NULL); >> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, >> + pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL); >> if (!obj) >> return pci_dev_get(pdev); >> >> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h >> index 078225b514d4..7966ef8f14b3 100644 >> --- a/include/linux/pci-acpi.h >> +++ b/include/linux/pci-acpi.h >> @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { } >> #endif >> >> extern const guid_t pci_acpi_dsm_guid; >> +extern const int pci_acpi_dsm_rev; >> >> /* _DSM Definitions for PCI */ >> #define DSM_PCI_PRESERVE_BOOT_CONFIG 0x05 >> -- >> 2.34.1 >>
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
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 58b89b8d950e..bca2270a93d4 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, * exists and returns 0, we must preserve any PCI resource * assignments made by firmware for this host bridge. */ - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER); if (obj && obj->integer.value == 0) host_bridge->preserve_config = 1; diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 004575091596..bea72e807817 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -28,6 +28,7 @@ const guid_t pci_acpi_dsm_guid = GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a, 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d); +const int pci_acpi_dsm_rev = 5; #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res) @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus) if (!pci_is_root_bus(bus)) return; - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3, + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), + &pci_acpi_dsm_guid, pci_acpi_dsm_rev, DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER); if (!obj) return; @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, if (bridge->ignore_reset_delay) pdev->d3cold_delay = 0; - obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3, + obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, DSM_PCI_DEVICE_READINESS_DURATIONS, NULL, ACPI_TYPE_PACKAGE); if (!obj) diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c index 0c6446519640..91bdd04029f0 100644 --- a/drivers/pci/pci-label.c +++ b/drivers/pci/pci-label.c @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev) if (!handle) return false; - return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2, + return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, 1 << DSM_PCI_DEVICE_NAME); #else return false; @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf, if (!handle) return -1; - obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2, + obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, DSM_PCI_DEVICE_NAME, NULL); if (!obj) return -1; diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index 5f4914d313a1..ab6a50201124 100644 --- a/drivers/pci/pcie/edr.c +++ b/drivers/pci/pcie/edr.c @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev) * Behavior when calling unsupported _DSM functions is undefined, * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. */ - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, 1ULL << EDR_PORT_DPC_ENABLE_DSM)) return 0; @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev) * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is * optional. Return success if it's not implemented. */ - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, - EDR_PORT_DPC_ENABLE_DSM, &argv4); + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, + pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM, + &argv4); if (!obj) return 0; @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) * Behavior when calling unsupported _DSM functions is undefined, * so check whether EDR_PORT_DPC_ENABLE_DSM is supported. */ - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev, 1ULL << EDR_PORT_LOCATE_DSM)) return pci_dev_get(pdev); - obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, - EDR_PORT_LOCATE_DSM, NULL); + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, + pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL); if (!obj) return pci_dev_get(pdev); diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 078225b514d4..7966ef8f14b3 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { } #endif extern const guid_t pci_acpi_dsm_guid; +extern const int pci_acpi_dsm_rev; /* _DSM Definitions for PCI */ #define DSM_PCI_PRESERVE_BOOT_CONFIG 0x05
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" Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 PCI Firmware specification 3.3, section 4.6 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/acpi/pci_root.c | 3 ++- drivers/pci/pci-acpi.c | 6 ++++-- drivers/pci/pci-label.c | 4 ++-- drivers/pci/pcie/edr.c | 13 +++++++------ include/linux/pci-acpi.h | 1 + 5 files changed, 16 insertions(+), 11 deletions(-)