| Message ID | 20240501022543.1626025-1-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec | expand |
On Wed, May 01, 2024 at 02:25:43AM +0000, Kuppuswamy Sathyanarayanan wrote: > During the Error Disconnect Recover (EDR) spec transition from r3.2 ECN > to PCI firmware spec r3.3, improvements were made to definitions of > EDR_PORT_DPC_ENABLE_DSM (0x0C) and EDR_PORT_LOCATE_DSM(0x0D) _DSMs. > > Specifically, > > * EDR_PORT_DPC_ENABLE_DSM _DSM version changed from 5 to 6, and > arg4 is now a package type instead of an integer in version 5. > * EDR_PORT_LOCATE_DSM _DSM uses BIT(31) to return the status of the > operation. > > Ensure _DSM implementation aligns with PCI firmware r3.3 spec > recommendation. More details about the EDR_PORT_DPC_ENABLE_DSM and > EDR_PORT_LOCATE_DSM _DSMs can be found in PCI firmware specification, > r3.3, sec 4.6.12 and sec 4.6.13. > > While at it, fix a typo in EDR_PORT_LOCATE_DSM comments. > > Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > drivers/pci/pcie/edr.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index 5f4914d313a1..fea098e22e3e 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, 6, > 1ULL << EDR_PORT_DPC_ENABLE_DSM)) How confident are we that this won't break any existing platforms? Any idea how many platforms implement EDR_PORT_DPC_ENABLE_DSM and what Revision IDs they support? > return 0; > > @@ -47,11 +47,11 @@ static int acpi_enable_dpc(struct pci_dev *pdev) > argv4.package.elements = &req; > > /* > - * Per Downstream Port Containment Related Enhancements ECN to PCI > - * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is > - * optional. Return success if it's not implemented. > + * Per PCI Firmware Specification r3.3, 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, > + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 6, > EDR_PORT_DPC_ENABLE_DSM, &argv4); > if (!obj) > return 0; > @@ -86,7 +86,7 @@ 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. > + * so check whether EDR_PORT_LOCATE_DSM is supported. > */ > if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > 1ULL << EDR_PORT_LOCATE_DSM)) > @@ -103,6 +103,17 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) > return NULL; > } > > + /* > + * Per PCI Firmware Specification r3.3, sec 4.6.13, bit 31 represents > + * the success/failure of the operation. If bit 31 is set, the operation > + * is failed. > + */ > + if (obj->integer.value & BIT(31)) { > + ACPI_FREE(obj); > + pci_err(pdev, "Locate Port _DSM failed\n"); > + return NULL; > + } This changes two _DSMs, and I think it should be two patches. Same question here: we now depend on functionality we didn't depend on before. How confident are we in this? > /* > * Firmware returns DPC port BDF details in following format: > * 15:8 = bus > -- > 2.25.1 >
Hi, On 5/1/24 2:50 PM, Bjorn Helgaas wrote: > On Wed, May 01, 2024 at 02:25:43AM +0000, Kuppuswamy Sathyanarayanan wrote: >> During the Error Disconnect Recover (EDR) spec transition from r3.2 ECN >> to PCI firmware spec r3.3, improvements were made to definitions of >> EDR_PORT_DPC_ENABLE_DSM (0x0C) and EDR_PORT_LOCATE_DSM(0x0D) _DSMs. >> >> Specifically, >> >> * EDR_PORT_DPC_ENABLE_DSM _DSM version changed from 5 to 6, and >> arg4 is now a package type instead of an integer in version 5. >> * EDR_PORT_LOCATE_DSM _DSM uses BIT(31) to return the status of the >> operation. >> >> Ensure _DSM implementation aligns with PCI firmware r3.3 spec >> recommendation. More details about the EDR_PORT_DPC_ENABLE_DSM and >> EDR_PORT_LOCATE_DSM _DSMs can be found in PCI firmware specification, >> r3.3, sec 4.6.12 and sec 4.6.13. >> >> While at it, fix a typo in EDR_PORT_LOCATE_DSM comments. >> >> Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> drivers/pci/pcie/edr.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >> index 5f4914d313a1..fea098e22e3e 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, 6, >> 1ULL << EDR_PORT_DPC_ENABLE_DSM)) > How confident are we that this won't break any existing platforms? Since we are already using arg4 as package, it wont work with platforms that implement version 5. So I think we won't be breaking any existing users of version 5. > Any idea how many platforms implement EDR_PORT_DPC_ENABLE_DSM and what > Revision IDs they support? I am not very sure about it. I think it is being used in some Dell and Nvidia platforms. @Satish from Dell, tested this fix in some Dell server platforms that implements this support and found it working. @Tushar Dave, since you previously submitted some error report related to EDR, I assume you have some platforms that uses these _DSMs. Can you please take a look at this patch and let us know whether it works for you? > >> return 0; >> >> @@ -47,11 +47,11 @@ static int acpi_enable_dpc(struct pci_dev *pdev) >> argv4.package.elements = &req; >> >> /* >> - * Per Downstream Port Containment Related Enhancements ECN to PCI >> - * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is >> - * optional. Return success if it's not implemented. >> + * Per PCI Firmware Specification r3.3, 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, >> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 6, >> EDR_PORT_DPC_ENABLE_DSM, &argv4); >> if (!obj) >> return 0; >> @@ -86,7 +86,7 @@ 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. >> + * so check whether EDR_PORT_LOCATE_DSM is supported. >> */ >> if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> 1ULL << EDR_PORT_LOCATE_DSM)) >> @@ -103,6 +103,17 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) >> return NULL; >> } >> >> + /* >> + * Per PCI Firmware Specification r3.3, sec 4.6.13, bit 31 represents >> + * the success/failure of the operation. If bit 31 is set, the operation >> + * is failed. >> + */ >> + if (obj->integer.value & BIT(31)) { >> + ACPI_FREE(obj); >> + pci_err(pdev, "Locate Port _DSM failed\n"); >> + return NULL; >> + } > This changes two _DSMs, and I think it should be two patches. Ok. I can split it into two patches. > Same question here: we now depend on functionality we didn't depend on > before. How confident are we in this? In some platforms I have tested so far, it seems to work. But I am not sure whether there are other platforms that does not implement this support. IMO, since this change aligns with the spec, it is best to fix it. > >> /* >> * Firmware returns DPC port BDF details in following format: >> * 15:8 = bus >> -- >> 2.25.1 >>
Hi Sathya/Bjorn, Right, from Dell we did perform limited testing on one selected platform with this EDR patch and it did work. But if need to be tested with multiple platforms then we need to test few other configs. Please advise Thanks, Satish Internal Use - Confidential -----Original Message----- From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Sent: Tuesday, April 30, 2024 9:26 PM To: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Thatchanamurthy, Satish <Satish.Thatchanamurt@Dell.com> Subject: [PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec [EXTERNAL EMAIL] During the Error Disconnect Recover (EDR) spec transition from r3.2 ECN to PCI firmware spec r3.3, improvements were made to definitions of EDR_PORT_DPC_ENABLE_DSM (0x0C) and EDR_PORT_LOCATE_DSM(0x0D) _DSMs. Specifically, * EDR_PORT_DPC_ENABLE_DSM _DSM version changed from 5 to 6, and arg4 is now a package type instead of an integer in version 5. * EDR_PORT_LOCATE_DSM _DSM uses BIT(31) to return the status of the operation. Ensure _DSM implementation aligns with PCI firmware r3.3 spec recommendation. More details about the EDR_PORT_DPC_ENABLE_DSM and EDR_PORT_LOCATE_DSM _DSMs can be found in PCI firmware specification, r3.3, sec 4.6.12 and sec 4.6.13. While at it, fix a typo in EDR_PORT_LOCATE_DSM comments. Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- drivers/pci/pcie/edr.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index 5f4914d313a1..fea098e22e3e 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, 6, 1ULL << EDR_PORT_DPC_ENABLE_DSM)) return 0; @@ -47,11 +47,11 @@ static int acpi_enable_dpc(struct pci_dev *pdev) argv4.package.elements = &req; /* - * Per Downstream Port Containment Related Enhancements ECN to PCI - * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is - * optional. Return success if it's not implemented. + * Per PCI Firmware Specification r3.3, 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, + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 6, EDR_PORT_DPC_ENABLE_DSM, &argv4); if (!obj) return 0; @@ -86,7 +86,7 @@ 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. + * so check whether EDR_PORT_LOCATE_DSM is supported. */ if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, 1ULL << EDR_PORT_LOCATE_DSM)) @@ -103,6 +103,17 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) return NULL; } + /* + * Per PCI Firmware Specification r3.3, sec 4.6.13, bit 31 represents + * the success/failure of the operation. If bit 31 is set, the operation + * is failed. + */ + if (obj->integer.value & BIT(31)) { + ACPI_FREE(obj); + pci_err(pdev, "Locate Port _DSM failed\n"); + return NULL; + } + /* * Firmware returns DPC port BDF details in following format: * 15:8 = bus -- 2.25.1
On Wed, May 01, 2024 at 02:25:43AM +0000, Kuppuswamy Sathyanarayanan wrote: > During the Error Disconnect Recover (EDR) spec transition from r3.2 ECN > to PCI firmware spec r3.3, improvements were made to definitions of > EDR_PORT_DPC_ENABLE_DSM (0x0C) and EDR_PORT_LOCATE_DSM(0x0D) _DSMs. > > Specifically, > > * EDR_PORT_DPC_ENABLE_DSM _DSM version changed from 5 to 6, and > arg4 is now a package type instead of an integer in version 5. > * EDR_PORT_LOCATE_DSM _DSM uses BIT(31) to return the status of the > operation. > > Ensure _DSM implementation aligns with PCI firmware r3.3 spec > recommendation. More details about the EDR_PORT_DPC_ENABLE_DSM and > EDR_PORT_LOCATE_DSM _DSMs can be found in PCI firmware specification, > r3.3, sec 4.6.12 and sec 4.6.13. > > While at it, fix a typo in EDR_PORT_LOCATE_DSM comments. > > Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> I split this into two patches and applied them to pci/edr for v6.10, thanks! Take a look here: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=edr and make sure I didn't mess it up (only differences are comments and commit logs). > --- > drivers/pci/pcie/edr.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index 5f4914d313a1..fea098e22e3e 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, 6, > 1ULL << EDR_PORT_DPC_ENABLE_DSM)) > return 0; > > @@ -47,11 +47,11 @@ static int acpi_enable_dpc(struct pci_dev *pdev) > argv4.package.elements = &req; > > /* > - * Per Downstream Port Containment Related Enhancements ECN to PCI > - * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is > - * optional. Return success if it's not implemented. > + * Per PCI Firmware Specification r3.3, 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, > + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 6, > EDR_PORT_DPC_ENABLE_DSM, &argv4); > if (!obj) > return 0; > @@ -86,7 +86,7 @@ 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. > + * so check whether EDR_PORT_LOCATE_DSM is supported. > */ > if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > 1ULL << EDR_PORT_LOCATE_DSM)) > @@ -103,6 +103,17 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) > return NULL; > } > > + /* > + * Per PCI Firmware Specification r3.3, sec 4.6.13, bit 31 represents > + * the success/failure of the operation. If bit 31 is set, the operation > + * is failed. > + */ > + if (obj->integer.value & BIT(31)) { > + ACPI_FREE(obj); > + pci_err(pdev, "Locate Port _DSM failed\n"); > + return NULL; > + } > + > /* > * Firmware returns DPC port BDF details in following format: > * 15:8 = bus > -- > 2.25.1 >
On 5/8/24 1:14 PM, Bjorn Helgaas wrote: > On Wed, May 01, 2024 at 02:25:43AM +0000, Kuppuswamy Sathyanarayanan wrote: >> During the Error Disconnect Recover (EDR) spec transition from r3.2 ECN >> to PCI firmware spec r3.3, improvements were made to definitions of >> EDR_PORT_DPC_ENABLE_DSM (0x0C) and EDR_PORT_LOCATE_DSM(0x0D) _DSMs. >> >> Specifically, >> >> * EDR_PORT_DPC_ENABLE_DSM _DSM version changed from 5 to 6, and >> arg4 is now a package type instead of an integer in version 5. >> * EDR_PORT_LOCATE_DSM _DSM uses BIT(31) to return the status of the >> operation. >> >> Ensure _DSM implementation aligns with PCI firmware r3.3 spec >> recommendation. More details about the EDR_PORT_DPC_ENABLE_DSM and >> EDR_PORT_LOCATE_DSM _DSMs can be found in PCI firmware specification, >> r3.3, sec 4.6.12 and sec 4.6.13. >> >> While at it, fix a typo in EDR_PORT_LOCATE_DSM comments. >> >> Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > I split this into two patches and applied them to pci/edr for v6.10, > thanks! > > Take a look here: > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=edr > and make sure I didn't mess it up (only differences are comments and > commit logs). > Thanks for doing it. It looks good to me. >> --- >> drivers/pci/pcie/edr.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >> index 5f4914d313a1..fea098e22e3e 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, 6, >> 1ULL << EDR_PORT_DPC_ENABLE_DSM)) >> return 0; >> >> @@ -47,11 +47,11 @@ static int acpi_enable_dpc(struct pci_dev *pdev) >> argv4.package.elements = &req; >> >> /* >> - * Per Downstream Port Containment Related Enhancements ECN to PCI >> - * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is >> - * optional. Return success if it's not implemented. >> + * Per PCI Firmware Specification r3.3, 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, >> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 6, >> EDR_PORT_DPC_ENABLE_DSM, &argv4); >> if (!obj) >> return 0; >> @@ -86,7 +86,7 @@ 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. >> + * so check whether EDR_PORT_LOCATE_DSM is supported. >> */ >> if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, >> 1ULL << EDR_PORT_LOCATE_DSM)) >> @@ -103,6 +103,17 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) >> return NULL; >> } >> >> + /* >> + * Per PCI Firmware Specification r3.3, sec 4.6.13, bit 31 represents >> + * the success/failure of the operation. If bit 31 is set, the operation >> + * is failed. >> + */ >> + if (obj->integer.value & BIT(31)) { >> + ACPI_FREE(obj); >> + pci_err(pdev, "Locate Port _DSM failed\n"); >> + return NULL; >> + } >> + >> /* >> * Firmware returns DPC port BDF details in following format: >> * 15:8 = bus >> -- >> 2.25.1 >>
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index 5f4914d313a1..fea098e22e3e 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, 6, 1ULL << EDR_PORT_DPC_ENABLE_DSM)) return 0; @@ -47,11 +47,11 @@ static int acpi_enable_dpc(struct pci_dev *pdev) argv4.package.elements = &req; /* - * Per Downstream Port Containment Related Enhancements ECN to PCI - * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is - * optional. Return success if it's not implemented. + * Per PCI Firmware Specification r3.3, 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, + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 6, EDR_PORT_DPC_ENABLE_DSM, &argv4); if (!obj) return 0; @@ -86,7 +86,7 @@ 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. + * so check whether EDR_PORT_LOCATE_DSM is supported. */ if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, 1ULL << EDR_PORT_LOCATE_DSM)) @@ -103,6 +103,17 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev) return NULL; } + /* + * Per PCI Firmware Specification r3.3, sec 4.6.13, bit 31 represents + * the success/failure of the operation. If bit 31 is set, the operation + * is failed. + */ + if (obj->integer.value & BIT(31)) { + ACPI_FREE(obj); + pci_err(pdev, "Locate Port _DSM failed\n"); + return NULL; + } + /* * Firmware returns DPC port BDF details in following format: * 15:8 = bus
During the Error Disconnect Recover (EDR) spec transition from r3.2 ECN to PCI firmware spec r3.3, improvements were made to definitions of EDR_PORT_DPC_ENABLE_DSM (0x0C) and EDR_PORT_LOCATE_DSM(0x0D) _DSMs. Specifically, * EDR_PORT_DPC_ENABLE_DSM _DSM version changed from 5 to 6, and arg4 is now a package type instead of an integer in version 5. * EDR_PORT_LOCATE_DSM _DSM uses BIT(31) to return the status of the operation. Ensure _DSM implementation aligns with PCI firmware r3.3 spec recommendation. More details about the EDR_PORT_DPC_ENABLE_DSM and EDR_PORT_LOCATE_DSM _DSMs can be found in PCI firmware specification, r3.3, sec 4.6.12 and sec 4.6.13. While at it, fix a typo in EDR_PORT_LOCATE_DSM comments. Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- drivers/pci/pcie/edr.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)